Make region and bucket as optional and always set the endpoint #2
Conversation
This file contains 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 reverts commit eb4f39f.
These are changes necessary to support the usage when only the bucket url is known.
philss
changed the title
Also make that attribute required in the typespecs.
philss
added a commit
to elixir-explorer/explorer
that referenced
this pull request
Aug 19, 2023
This makes Explorer treat the "endpoint" as already containing the bucket name, and force the virtual host style in order to make Polars ignore the bucket name. Since FSS is always building the correct endpoint in case the user does not provide, we changed the endpoint to be required in the Rust struct. This work is related to elixir-explorer/fss#2
josevalim
reviewed
Aug 19, 2023
This is going to treat the endpoint differently depending on bucket name. It is also going to make `:bucket` nil if we can build a virtual-host style endpoint. The rules are more or less the following: * Endpoint is given * And bucket is an empty string (s3:///path): keep bucket nil * Otherwise keep both endpoint and bucket in config * Endpoint is nil * Bucket has dots: keep the endpoint _without_ the bucket, fill in bucket field * Bucket does not have dots: keep the endpoint with bucket, bucket is nil
josevalim
approved these changes
Aug 22, 2023
philss
added a commit
to elixir-explorer/explorer
that referenced
this pull request
Aug 22, 2023
philss
added a commit
to elixir-explorer/explorer
that referenced
this pull request
Aug 23, 2023
This makes Explorer treat the "endpoint" as already containing the bucket name, and force the virtual host style in order to make Polars ignore the bucket name. Since FSS is always building the correct endpoint in case the user does not provide, we changed the endpoint to be required in the Rust struct. This work is related to elixir-explorer/fss#2
philss
added a commit
to elixir-explorer/explorer
that referenced
this pull request
Aug 23, 2023
philss
added a commit
to elixir-explorer/explorer
that referenced
this pull request
Aug 23, 2023
* Fix FSS.S3 support when bucket is not provided This makes Explorer treat the "endpoint" as already containing the bucket name, and force the virtual host style in order to make Polars ignore the bucket name. Since FSS is always building the correct endpoint in case the user does not provide, we changed the endpoint to be required in the Rust struct. This work is related to elixir-explorer/fss#2 * Fix with suggestions from PR It addresses José's suggestions to simplify the endpoint. * Update FSS after merging PR PR merged: elixir-explorer/fss#2 * Update FSS to the latest version
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We need to relax a little bit the region configuration, because it's not required for every service that is S3 compatible.
We also relax the requirement of the bucket, because in some situations we cannot know the bucket name from a "bucket URL". We assume that the bucket name is in the endpoint, in case bucket is not provided.
This is also moving the
:bucketfield from theFSS.S3.EntrytoFSS.S3.Configstruct.