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

Added parsing logic if multiple protocols are supplied #632

Merged
merged 10 commits into from
Sep 27, 2023

Conversation

norlandrhagen
Copy link
Contributor

@cisaacstern
Copy link
Member

This is looking good, @norlandrhagen, thanks for jumping on this fix!

FWIW implementing #623 would have caught this bug before it was merged, and would also be useful for testing this PR... but not sure if it's worth the overhead to implement right now, unless you're feeling motivated to do so. 😄

To be clear, happy to test this with a more basic unit test as well (don't need minio).

@norlandrhagen
Copy link
Contributor Author

Totally see how minio would be helpful! For now I just added a simple test, but it would be nice to include that in the future todo.

@cisaacstern
Copy link
Member

Probably need to pass anon=True to workaround the missing credentials error being thrown in the tests. 🙂

@norlandrhagen norlandrhagen marked this pull request as ready for review September 27, 2023 16:54
@cisaacstern cisaacstern added the test-integration Apply this label to run integration tests on a PR. label Sep 27, 2023
@cisaacstern
Copy link
Member

LGTM! Thanks so much @norlandrhagen!

@cisaacstern cisaacstern merged commit 175b522 into pangeo-forge:main Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-integration Apply this label to run integration tests on a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants