Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add S3 uploader #244

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add S3 uploader #244

wants to merge 6 commits into from

Conversation

suflors
Copy link

@suflors suflors commented Aug 22, 2024

Found a fork that might be able to offer a solution, however, I have only been able to ensure that the images are being uploaded to Amazon S3 from this, it may be that my custom app that I just made is not properly working yet, or otherwise the actual thing setting the image to be displayed does not work. Will test again later to make sure, but in the meantime, I heavily suggest you test it yourself. If it is in fact broken, I will attempt to fix it in this branch.

@suflors
Copy link
Author

suflors commented Aug 22, 2024

Another thing, I have no idea how you run this project as debug, so I can't really properly look through things. If you could provide instructions for doing so, that would be great.

@suflors
Copy link
Author

suflors commented Aug 22, 2024

Did some digging around, I'm worried Discord might've deprecated support for external images being displayed by Rich Presence activities. I tried testing it by manually handing it one, and it hasn't seemed to work so far.

@sll552
Copy link
Owner

sll552 commented Aug 22, 2024

Another thing, I have no idea how you run this project as debug, so I can't really properly look through things. If you could provide instructions for doing so, that would be great.

This should work with the provided VS project, given that you installed musicbee using the installer (not store) and did not change installation directory. Otherwise check the project configuration in VS.

Did some digging around, I'm worried Discord might've deprecated support for external images being displayed by Rich Presence activities. I tried testing it by manually handing it one, and it hasn't seemed to work so far.

Need to test that myself

In general the idea behind the whole uploader stuff is that it should be possible using a free hosting service. I am currently not aware that there are any free s3 offerings. I would be interested which service you use for hosting the images.

@suflors
Copy link
Author

suflors commented Aug 22, 2024

This should work with the provided VS project, given that you installed musicbee using the installer (not store) and did not change installation directory. Otherwise check the project configuration in VS.

What I did in VS is Debug -> Attach to Process, searched and selected MusicBee, and that's it. Nothing happened, I uninstalled the plugin from MusicBee prior to this, as I assume it doesn't work if you don't. I've never debugged via attaching to a process period, if what I did sounds like it's incorrect, some help would be great.

Need to test that myself

Determined this is not the case as the program is still working for some people, will be investigating more today. My current theory is that some change between version 3.0.2 and 3.1.0 made things more prone to breaking, as someone who is on 3.0.2 was able to show the functionality still works.

In general the idea behind the whole uploader stuff is that it should be possible using a free hosting service. I am currently not aware that there are any free s3 offerings. I would be interested which service you use for hosting the images.

S3 is not free, and would thus be up to the user to configure and provide for (though, the charge is .023 USD for 1 GB, so it's fairly inexpensive). However, at the very least it is an extra option. I wanted to put CatBox.moe functionality in, however, the package does not target an old enough version. I will see what I can do about that, but I'd like to see if I can get what currently exists to work first off, as it should be possible, given others have it working.

@suflors
Copy link
Author

suflors commented Aug 22, 2024

If you have a better place I can contact you, that would be great. I'm digging into the Imgur errors now and trying to make sense of them and what versions they happen on, would really love some pointers or at least to properly get the debugging set up so I can see it happen in real time and not just through error logs.

@suflors
Copy link
Author

suflors commented Aug 22, 2024

Update: figured out how to debug it now. Will see what I can do.

@suflors
Copy link
Author

suflors commented Aug 22, 2024

image
I couldn't figure out getting Imgur to work again, however, I could figure out getting this branch to work. I would heavily recommend merging it as it offers at least a chance for those who can't get Imgur to work another option. There is no default AW3 Bucket, so if one wants to use it, they will have to set up one themselves. I will be looking into other replacements for Imgur for the time being, but I do believe this should be provided as an option. If you intend to accept it, I could likely write up a tutorial on how to set it up, as well.

@sll552
Copy link
Owner

sll552 commented Aug 23, 2024

I will take a look at it, but pls be patient I guess it will take more than a few days 😞
Also I don't have an S3 setup anywhere, so if you could provide me some bucket for testing that would be great. You can contact me on Discord for the credentials etc.

@suflors
Copy link
Author

suflors commented Aug 23, 2024

I will take a look at it, but pls be patient I guess it will take more than a few days 😞 Also I don't have an S3 setup anywhere, so if you could provide me some bucket for testing that would be great. You can contact me on Discord for the credentials etc.

Absolutely, I can help in that, and take all the time you need!

@kramerc
Copy link

kramerc commented Aug 28, 2024

Hey, thanks for creating the PR for this feature. I believe the fork you're referring to was mine, seeing how the changes are mostly nearly identical. I intended to get a PR created, but if I recall correctly, there were some bugs and polish (believe it was quirks with the settings UI, feeling like the S3 health check could be improved, and readme updates) that I didn't get around to wrapping it up as I had other priorities going on. I would at least appreciate some credit for my work 😅

That said, I've been using my fork for a while and it works quite well.

Example usage

Album cover hosted on AWS S3 served with CloudFront endpoint:
https://x.mer.vg/DiscordBee/ca2000dc6535f25da67d8134a5bc088b
firefox_c1O3CZpWgX
MusicBee_hQ1Nz0ZwkU

@sll552
Copy link
Owner

sll552 commented Aug 28, 2024

Hey, thanks for creating the PR for this feature. I believe the fork you're referring to was mine, seeing how the changes are mostly nearly identical. I intended to get a PR created, but if I recall correctly, there were some bugs and polish (believe it was quirks with the settings UI, feeling like the S3 health check could be improved, and readme updates) that I didn't get around to wrapping it up as I had other priorities going on. I would at least appreciate some credit for my work 😅

I think the original commits (before the last force-push) actually included your authorship, so please restore the author @suflors

@suflors
Copy link
Author

suflors commented Aug 28, 2024

Hey, thanks for creating the PR for this feature. I believe the fork you're referring to was mine, seeing how the changes are mostly nearly identical. I intended to get a PR created, but if I recall correctly, there were some bugs and polish (believe it was quirks with the settings UI, feeling like the S3 health check could be improved, and readme updates) that I didn't get around to wrapping it up as I had other priorities going on. I would at least appreciate some credit for my work 😅

I think the original commits (before the last force-push) actually included your authorship, so please restore the author @suflors

You're going to have to tell me exactly what you mean by that, do you want me to just change the commits back to the original commits? I can do this, if so. Did not mean to step on any feet, merely thought it'd be better if the original commit was split into two, as I felt that adding the uploader switcher and the new uploader in one commit was a lot. Say the words and I'll go change it back to the original commit + my changes, though.

Edit: Decided to just go ahead and do it, hope it's all fine now.

Copy link

@kramerc kramerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries @suflors, thanks for fixing the commit history. It looks good now. 👍

I believe I see the issues you were having with the GetBaseUrl() method. I hadn't adequately tested this without a CustomDomain value, as I use AWS CloudFront to serve assets from my S3 bucket.

I left some feedback on your changes, wrote a suggestion that better handles building the base URL from Endpoint without having to query the bucket's region, and suggestions on other things I noticed.

S3Client/S3Client.cs Outdated Show resolved Hide resolved
S3Client/S3Client.cs Outdated Show resolved Hide resolved
S3Client/S3Client.cs Outdated Show resolved Hide resolved
S3Client/S3Client.cs Outdated Show resolved Hide resolved
UI/SettingsWindow.cs Outdated Show resolved Hide resolved
UI/SettingsWindow.cs Outdated Show resolved Hide resolved
@HeartbeatingForCenturies

theres also https://imgbb.com, it has an API and its free - used here https://github.com/VZCE/mbrpc

@suflors
Copy link
Author

suflors commented Aug 29, 2024

theres also https://imgbb.com, it has an API and its free - used here https://github.com/VZCE/mbrpc

Will consider it for when I start writing other image uploaders. Thanks!

suflors and others added 2 commits August 29, 2024 17:05
Co-authored-by: Kramer Campbell <[email protected]>
Co-authored-by: Kramer Campbell <[email protected]>
@suflors
Copy link
Author

suflors commented Aug 30, 2024

No worries @suflors, thanks for fixing the commit history. It looks good now. 👍

I believe I see the issues you were having with the GetBaseUrl() method. I hadn't adequately tested this without a CustomDomain value, as I use AWS CloudFront to serve assets from my S3 bucket.

I left some feedback on your changes, wrote a suggestion that better handles building the base URL from Endpoint without having to query the bucket's region, and suggestions on other things I noticed.

Handled everything, I think. I left comments on each one for the most part, as well. By the way, do I have to worry about most of your commits being marked as "Unverified"? I'm not quite sure if it's something I'm supposed to do anything about.

@HeartbeatingForCenturies

thanks again for working on this, is there any documentation I should follow to set this up myself?

I've just made a bucket for s3 and given my credentials in the plugin but its not uploading anything

@suflors
Copy link
Author

suflors commented Sep 2, 2024

thanks again for working on this, is there any documentation I should follow to set this up myself?

I've just made a bucket for s3 and given my credentials in the plugin but its not uploading anything

Not yet, might be missing something but you should just have to turn off "Block all public access" and make your bucket policy look something like this:
{ "Version": "2012-10-17", "Statement": [ { "Sid": "PublicReadGetObject", "Effect": "Allow", "Principal": "*", "Action": "s3:GetObject", "Resource": "arn:aws:s3:::BUCKETNAMEHERE/*" }, { "Effect": "Allow", "Principal": { "AWS": "arn:aws:iam::308446412268:user/YOURUSERNAMEHERE" }, "Action": "s3:PutObject", "Resource": "arn:aws:s3:::BUCKETNAMEHERE/*" } ] }
After that it should work.

Feel free to harass me in the DiscordBee server if it still doesn't work.

@HeartbeatingForCenturies

thanks again for working on this, is there any documentation I should follow to set this up myself?
I've just made a bucket for s3 and given my credentials in the plugin but its not uploading anything

Not yet, might be missing something but you should just have to turn off "Block all public access" and make your bucket policy look something like this: { "Version": "2012-10-17", "Statement": [ { "Sid": "PublicReadGetObject", "Effect": "Allow", "Principal": "*", "Action": "s3:GetObject", "Resource": "arn:aws:s3:::BUCKETNAMEHERE/*" }, { "Effect": "Allow", "Principal": { "AWS": "arn:aws:iam::308446412268:user/YOURUSERNAMEHERE" }, "Action": "s3:PutObject", "Resource": "arn:aws:s3:::BUCKETNAMEHERE/*" } ] } After that it should work.

Feel free to harass me in the DiscordBee server if it still doesn't work.

thankyou, it worked using the policy you sent

@kramerc
Copy link

kramerc commented Sep 5, 2024

Hey I've been meaning to follow up on this, just got a lot of stuff going right now. Sorry to keep you all waiting.

@suflors
Copy link
Author

suflors commented Sep 5, 2024

Hey I've been meaning to follow up on this, just got a lot of stuff going right now. Sorry to keep you all waiting.

No problem, I've been busy on my own stuff anyway. Though I do wonder if @sll552 has looked at the branch yet at all.

@sll552
Copy link
Owner

sll552 commented Sep 6, 2024

I did take a look at the code, which looks fine, unfortunately I haven't had time to test it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants