Fix 607 Use s3ForcePathStyle Appropriately #650
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 10, 2022
This was referenced May 19, 2022
Collaborator
|
Please rebase. |
- Modified function signature. Instead of augmenting an object (side effect) the function now gets an options object and returns a configuration object. - Cleaned up of code auto used to extract bucket and region from s3 url. - Added comments. - Increased test coverage. - All tests pass.
- Modified versioning to add bucket name to hosted_path when s3ForcePathStyle is true. - Modified s3_setup to remove bucket name from prefix when s3ForcePathStyle is true. - Added test coverage for s3ForcePathStyle cases.
554be5f to
2555ef7
Compare
Contributor
Author
Yes sir 😄 |
cclauss
approved these changes
Jun 29, 2024
Collaborator
cclauss
left a comment
There was a problem hiding this comment.
AWESOME WORK! Thanks for righting the ship.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This pull request fixes #607 and additional issues. It also refactors S3 Setup detect functionality and increases test coverage of existing features.
The fix to #607 can be considered a patch for #576 (by @pive). Solution is based on #608 (by @jared-duo). The latter could be merged prior to merging this pull request (for well deserved contributor karma 👍).
Change
s3_setupdetectfunction signature. Instead of augmenting an object (side effect) the function now gets an options object and returns a configuration object.versioningto add bucket name to hosted_path whens3ForcePathStyleistrue.s3_setupto remove bucket name fromprefixwhens3ForcePathStyleistrue.publish,unpublishandinfoto be stylistically similar and report paths actually used.Tests
s3ForcePathStyleoptions performed against play.min.io. All as expected.