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

feat: add s3 OptionsFromConfig func #1241

Closed
wants to merge 1 commit into from
Closed

Conversation

renanferr
Copy link

This PR adds OptionsFromConfig function which does almost the same as NewFromConfig func but returns standalone Options.

I also made a change in NewFromConfig to use OptionsFromConfig so it's more maintainable.

@jasdel jasdel added the feature-request A feature should be added or improved. label Apr 27, 2021
@jasdel
Copy link
Contributor

jasdel commented Apr 27, 2021

Thanks for submitting this PR @renanferr. We won't be able to merge this change as it is, since the chance is being made to the generated files. But this is a change that could be made to the SDK's code generation, to generate the OptionsFromConfig helper for each API client.

@jasdel
Copy link
Contributor

jasdel commented Apr 27, 2021

HI @renanferr could you provide some background on your usecase for this feature, and the reason it would be helpful for your aplication?

@renanferr
Copy link
Author

Hi @jasdel, thanks for the feedback! I can try to make the change in the codegen, then.
The use case is an app with multiple custom s3 clients that i am migrating the sdk version from v1 to v2. It uses fx to manage dependecy injection and constructs the clients from aws config read by a private framework of ours.
I managed to make it work by using the NewFromConfig constructor and by wrapping the sdk client with our clients, but I figured this feature could be useful and decided to try and contribute.

@renanferr
Copy link
Author

@jasdel do you think this feature is relevant? I could work on it or just close the PR.

@jasdel
Copy link
Contributor

jasdel commented May 7, 2021

Thanks for the update @renanferr. Based on this information. Sounds like using and wrapping the NewFromConfig is the best approach for this. Mainly because the config.LoadDefaultConfig is the best way to source generic configuration across multiple API clients with the SDK's aws.Config value. The API client's Options type is specific to the API client, and could only be used to create additional instances of it. whereas the aws.Config can be used to initialize all of the API clients.

Using the NewFromConfig at a higher level in your application, and passing the value down into your code is a great way to ensure your code does not need to be aware how the API client is created. This pattern also allows you to unit test your code without needing to be concerned about the internal details of the API client.

Given this I think we can close this PR, as alternative design fits better. Let us know if you have additional feedback or issues.

@jasdel jasdel added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 7, 2021
@renanferr renanferr closed this May 14, 2021
@renanferr
Copy link
Author

Alright, it's closed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closing-soon This issue will automatically close in 4 days unless further comments are made. feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants