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

User Provided S3 Config #240

Merged
merged 3 commits into from
Jul 11, 2023
Merged

User Provided S3 Config #240

merged 3 commits into from
Jul 11, 2023

Conversation

chrisowebb
Copy link
Contributor

Allow user to provide configuration for S3 on api init

User may want to change the config of the client for s3. One example of this is during local dev, to change endpoint etc.

I think the only way to currently do this, is to import the client from lambda-api/lib/s3-service.js and override it?

I think it might be a nicer user experience to be able to provide the config on api init. 😄

@naorpeled
Copy link
Collaborator

Hey @chrisowebb

Thanks for opening this PR, you're awesome! Great feature!

I'm currently on vacation until Tuesday+-

Will be able to check this more thoroughly then.
In the meantime, could you please add some tests? that'd be much appreciated❤️

@chrisowebb
Copy link
Contributor Author

Hey @chrisowebb

Thanks for opening this PR, you're awesome! Great feature!

I'm currently on vacation until Tuesday+-

Will be able to check this more thoroughly then. In the meantime, could you please add some tests? that'd be much appreciated❤️

hey, @naorpeled thanks for the response. Have you had a chance to look at this yet ? 😄

@naorpeled
Copy link
Collaborator

Hey @chrisowebb

Thanks for opening this PR, you're awesome! Great feature!

I'm currently on vacation until Tuesday+-

Will be able to check this more thoroughly then. In the meantime, could you please add some tests? that'd be much appreciated❤️

hey, @naorpeled thanks for the response. Have you had a chance to look at this yet ? 😄

My plan is to try it out today 😎
Don't worry, didn't forget

Comment on lines 14 to 15
exports.setClient = (config) => (exports.client = new S3Client(config));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Imo, in order to not making a breaking change,
we should not remove the ``exports.client = new S3Client()``` above.

Wdyt?

Also, regarding naming of setClient,
maybe we should rename to setConfig or overrideConfig because we're not passing a client, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points, totally agree. I have made updates to reflect this 😄

@chrisowebb chrisowebb requested a review from naorpeled July 10, 2023 20:49
Copy link
Collaborator

@naorpeled naorpeled left a comment

Choose a reason for hiding this comment

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

LGTM 😎

I wonder whether we can implement setConfig without creating a new instance of the client.
Honestly it's probably not super crucial but just a thought.

Anyways, thanks for this, you rock!

@naorpeled
Copy link
Collaborator

Will merge and release this tomorrow 🙏

@naorpeled naorpeled merged commit 732f2a7 into jeremydaly:main Jul 11, 2023
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.

2 participants