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

fix: trim trailing slash in url storage options (#2656) #2775

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

omkar-foss
Copy link
Contributor

Description

This trims trailing slash if present in a storage option's value
if it's key ends with _URL (e.g. HOST_URL) or if the value
itself seems to be a url (i.e. starts with http:// or https://).

This also adds supporting test for this fix.

Related Issue(s)

Documentation

N/A

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Aug 15, 2024
@omkar-foss omkar-foss marked this pull request as ready for review August 15, 2024 02:55
@omkar-foss
Copy link
Contributor Author

omkar-foss commented Aug 15, 2024

Tried closing PR #2770 and re-opening the same here in this PR, but the workflows are still stuck at Expected — Waiting for status to be reported. It'll be great if someone can help resolve this, thanks.

@ion-elgreco
Copy link
Collaborator

Tried closing PR #2770 and re-opening the same here in this PR, but the workflows are still stuck at Expected — Waiting for status to be reported. It'll be great if someone can help resolve this, thanks.

Workflows require approval each time

@omkar-foss
Copy link
Contributor Author

Workflows require approval each time

Ohhh got it, thanks! 👍🏽

.map(|(k, v)| {
let needs_trim = v.starts_with("http://")
|| v.starts_with("https://")
|| k.to_lowercase().ends_with("_url");
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are we checking here? i.e. what does a _url suffix signify?

I'm probably just missing something obvious 😆.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah - from the tests i gather this is a convention we are introducing?

Should we document this behavior in a docstring somewhere?

This trims trailing slash if present in a storage option's value
if it's key ends with `_URL` (e.g. `HOST_URL`) or if the value
itself seems to be a url (i.e. starts with `http://` or `https://`).

This also adds supporting test for this fix.
Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!

@roeap roeap added this pull request to the merge queue Aug 19, 2024
Merged via the queue into delta-io:main with commit 673f8be Aug 19, 2024
18 checks passed
@omkar-foss omkar-foss deleted the trim-slash-storage-opts branch August 20, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trailing slash on AWS_ENDPOINT raises S3 Error
3 participants