Fix Azure URL parsing#604
Conversation
| /// - `s3://bucket/path` -> [`AmazonS3`](crate::aws::AmazonS3) (also supports `s3a`) | ||
| /// - `gs://bucket/path` -> [`GoogleCloudStorage`](crate::gcp::GoogleCloudStorage) | ||
| /// - `az://account/container/path` -> [`MicrosoftAzure`](crate::azure::MicrosoftAzure) (also supports `adl`, `azure`, `abfs`, `abfss`) | ||
| /// - `[az|abfs[s]]://container[@<account>.<host>]/path` -> [`MicrosoftAzure`](crate::azure::MicrosoftAzure) |
There was a problem hiding this comment.
I opted to drop azure as it is a custom format AFAICT, and adl as it is deprecated by azure.
Whilst still supported for compatibility, people shouldn't be using them.
There was a problem hiding this comment.
to be clear, you stopped documenting that azure is supported, but the code still supports it
| "az://account/container", | ||
| (ObjectStoreScheme::MicrosoftAzure, ""), | ||
| "az://container/path", | ||
| (ObjectStoreScheme::MicrosoftAzure, "path"), |
There was a problem hiding this comment.
8516db8 to
45759bd
Compare
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @tustvold . I just had one question about a test
FYI @kylebarron
| /// - `s3://bucket/path` -> [`AmazonS3`](crate::aws::AmazonS3) (also supports `s3a`) | ||
| /// - `gs://bucket/path` -> [`GoogleCloudStorage`](crate::gcp::GoogleCloudStorage) | ||
| /// - `az://account/container/path` -> [`MicrosoftAzure`](crate::azure::MicrosoftAzure) (also supports `adl`, `azure`, `abfs`, `abfss`) | ||
| /// - `[az|abfs[s]]://container[@<account>.<host>]/path` -> [`MicrosoftAzure`](crate::azure::MicrosoftAzure) |
There was a problem hiding this comment.
to be clear, you stopped documenting that azure is supported, but the code still supports it
| (ObjectStoreScheme::MicrosoftAzure, "path"), | ||
| ), | ||
| ( | ||
| "az://account/container/path", |
There was a problem hiding this comment.
Can we please keep this test so it is clearer what is the difference in behavior? I am not sure what this URL will parse to now
kylebarron
left a comment
kylebarron
left a comment
There was a problem hiding this comment.
This seems fine to me; I apologize for the breakage
3 participants
Which issue does this PR close?
Rationale for this change
#399 introduced a breaking change to the way that we parse
az://URLs. This IMO reflects a more sensible URL encoding, but this doesn't match the URL encoding expected by the rest of this crate, which in turn is borrowed from fsspec.What changes are included in this PR?
Reverts the URL parsing change in #399
Are there any user-facing changes?
Technically this is a bug fix, but might make sense to wait for a breaking release.