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

AWS S3 Provider #75

Merged
merged 5 commits into from
Jan 31, 2022
Merged

Conversation

andymac4182
Copy link
Contributor

@andymac4182 andymac4182 commented Sep 8, 2019

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Hi,

We are internally using our own provider based on some of our own libraries. I am looking to port this to the latest version of ImageSharp.Web and remove our custom libraries. Is this something you would be interested in supporting in the library?

For testing internally we use Minio which I think would fit with how you are testing Azure Blob Storage.

Cheers,
Andrew

@CLAassistant
Copy link

CLAassistant commented Sep 8, 2019

CLA assistant check
All committers have signed the CLA.

@andymac4182 andymac4182 changed the title Initial work on adding AWS S3 Provider AWS S3 Provider Sep 8, 2019
@andymac4182 andymac4182 force-pushed the feat/AWSProvider branch 2 times, most recently from f518bd1 to 9ffdf34 Compare September 8, 2019 03:46
@JimBobSquarePants
Copy link
Member

Thanks @andymac4182 I'll have a proper look asap. Will do a cursory review just now.

@andymac4182
Copy link
Contributor Author

Sorry for locking up the build with the mistake in the appveyor.yml I am pretty sure I have fixed it up.

/// Gets or sets the bucket name.
/// Must conform to AWS S3 bucket naming guidelines.
/// </summary>
public string BucketName { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Credentials?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to do some reading. AWS has multiple ways of initialising the client. Their recommendation is normally to use their extension methods. I will see how I go. We register a single S3 client outside of all of our code and I was thinking of applying something similar but I will confirm through their doco.

@andymac4182
Copy link
Contributor Author

I have seen how your testing the Azure provider. I will add some tests this week for the AWS one in a similar manner.

@JimBobSquarePants
Copy link
Member

@andymac4182 Any possibility we can get this up and running again? It would be nice to ship it with V1.

It's a brave new world using Github Actions for CI so I can help get that working.

@breaker05
Copy link

Any update on this being implemented?

@phinett
Copy link

phinett commented Jan 8, 2021

Any update on this being implemented?

Also keen to get this working. I have tried to add this code to my own project, it compiles and runs, but i can't get it to hit any breakpoints in the provider (apart from the constructor on initial load).

I'm not totally sure howi should access the image though, my bucket name is "my-s3" for instance, and im accessing http://localhost:5000/my-s3/myimage.jpg

Am i missing something?

@JimBobSquarePants
Copy link
Member

I really do not appreciate these "Any updates" kind of questions. They paint a picture of consumption without contribution which is not healthy for OSS.

@andymac4182 is doing this work in his own time and is under no obligation to ever complete it. (Please note, neither am I). I suggest that if there is any urgency on your part you should invest the time personally to implement a working solution.

@phinett
Copy link

phinett commented Jan 8, 2021

I'm not asking for an update on when its done.

I am actually trying to have a go at this myself, was just after a point in the right direction of how this pieces together more than anything.

Do i need to implement cache providers, file storage provider etc? Or should it simply work by registering an IImageProvider?

And does this somehow hook into every request? I am presuming yes, it then uses the IsMatch function to determine weather we should process or not?

Sorry if i came across in an incorrect manner.

@breaker05
Copy link

@JimBobSquarePants, my only intention about asking for an update was because this PR on your project is from 16 months ago and is a working solution so was not sure why it was not considered for your v1 release.

You immediately thumbs downed my response of me asking a very simple question if this work by @andymac4182 would be merged and implemented. I have no context as to why you did this since you left no comment. When I saw this earlier I took it as you had no intentions of using the work from @andymac4182. I instead wrote my own implementation and have it working successfully and happy to do my own PR, even though quite redundant to the work already here.

Your library is wonderful and I am quite thankful for it with the ability to extend it with other providers. I only ask you to not rush to judgment that everyone is only out to consume. I am happy to contribute to this project as well.

@JimBobSquarePants
Copy link
Member

@phinett @breaker05

Thanks both for your interest and also for expressing the interest in contributing.

Please both be aware though that you initial comments do not, (from a maintainers perspective), match the intent you have both since expressed and that there is no way, (from a maintainers perspective), to interpret the intent in a positive manner.

We all have something to learn from the exchange.

That aside...

@breaker05 If you have a working solution that you are able to share that would be wonderful!

In order to get something that is mergeable though we need a simple way to unit test your code (locally and in GitHub Actions) without having to set up an AWS account. That was the main reason for dropping the initial work from the V1 build.

Azure makes this very simple and need to be able to do the same for AWS.

If that is the case then I will happily work with you to integrate the solution into the repository here.

@JimBobSquarePants JimBobSquarePants merged commit 7f2c18e into SixLabors:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants