mbutrovich commented on code in PR #2976: URL: https://github.com/apache/iceberg-rust/pull/2976#discussion_r4198816056
########## Cargo.lock: ########## @@ -6193,10 +6212,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index"; checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.4.3", + "getrandom 0.3.4", "once_cell", "rustix", - "windows-sys 0.61.2", + "windows-sys 0.52.0", Review Comment: The lockfile also moves several crates to older versions that the new dependencies don't need. `windows-sys` goes from 0.61.2 to 0.48.0 or 0.52.0 for `errno`, `rustix`, `tempfile` and others, and `getrandom` goes from 0.4.3 to 0.3.4 for `tempfile`. When I start from `main`'s `Cargo.lock` with this PR's manifests and run `cargo metadata`, the only version changes are `reqsign-aws-core`, `reqsign-aws-v4`, `reqsign-core`, a second `quick-xml`, `asyncband`, and `socket2`. Could you regenerate the lockfile that way, so the PR doesn't carry the unrelated downgrades? ########## crates/storage/opendal/src/gcs.rs: ########## @@ -45,7 +51,9 @@ pub(crate) fn gcs_config_parse(mut m: HashMap<String, String>) -> Result<GcsConf cfg.endpoint = Some(endpoint); } - if m.remove(GCS_NO_AUTH).is_some() { + if let Some(no_auth) = m.remove(GCS_NO_AUTH) + && is_truthy(&no_auth) + { Review Comment: Could the `gcs.no-auth` parsing fix move to its own PR, along with the other fixes that don't depend on credential refresh? I count three of them in this PR: - `gcs.no-auth` now enables anonymous access only for a truthy value. On `main`, any value, including `false`, turns it on. - GCS now accepts `gcs://` paths in [`create_operator`](https://github.com/apache/iceberg-rust/blob/f631771198df4b98b251c391b43aadde060579cb/crates/storage/opendal/src/lib.rs#L557-L563), [`relativize_path`](https://github.com/apache/iceberg-rust/blob/f631771198df4b98b251c391b43aadde060579cb/crates/storage/opendal/src/lib.rs#L760-L772), and [`gcs_config_build`](https://github.com/apache/iceberg-rust/blob/f631771198df4b98b251c391b43aadde060579cb/crates/storage/opendal/src/gcs.rs#L89-L94). On `main`, `OpenDalResolvingStorage` routes `gcs://` to the GCS backend, but the backend then rejects the path. - [`OpenDalResolvingStorage`'s `Debug`](https://github.com/apache/iceberg-rust/blob/f631771198df4b98b251c391b43aadde060579cb/crates/storage/opendal/src/resolving.rs#L263-L270) now prints property keys only. On `main` it prints the property values, including secrets. Each one changes behavior for users who never set a credential provider, so it deserves its own changelog entry, and a reviewer can check it without reading the refresh code. They line up with the first piece of the split discussed on #2932. ########## crates/storage/opendal/src/lib.rs: ########## @@ -278,6 +306,7 @@ fn default_memory_operator() -> Operator { /// /// The serialized representation is not a stable format and may change between crate versions. #[derive(Clone, Debug, Serialize, Deserialize)] +#[non_exhaustive] Review Comment: Could you add an entry for this change under `[Unreleased]` "Breaking Changes" in [`CHANGELOG.md`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/CHANGELOG.md?plain=1#L27-L35)? Adding `#[non_exhaustive]` to `OpenDalStorage` and its variants, plus the new `credential_provider` fields on `S3` and `Gcs`, breaks downstream code that builds a variant with a struct literal or matches the enum exhaustively (`crates/storage/opendal/public-api.txt` shows the change). An entry tells those users what to change, the way #3286 did for `BoxedCatalogBuilder::with_runtime`. We discussed flagging this break in the split PR that carries it on #2932 ([thread](https://github.com/apache/iceberg-rust/pull/2932#discussion_r4137009802)). ########## crates/storage/opendal/src/utils.rs: ########## @@ -27,3 +28,296 @@ pub(crate) fn from_opendal_error(e: opendal::Error) -> iceberg::Error { ) .with_source(e) } + +/// The non-empty value of `key` in a vended credential's config. +#[cfg(any(feature = "opendal-s3", feature = "opendal-gcs"))] +pub(crate) fn required_credential_property<'a>( + config: &'a std::collections::HashMap<String, String>, Review Comment: The new code in `utils.rs` spells out `std::collections::HashMap`, `std::sync::Arc`, `iceberg::io::StorageCredential`, `iceberg::io::StorageCredentialProvider`, and `reqsign_core::...` inline throughout. If I'm reading it right, that's because `reqsign_core` is only available with the `opendal-s3` or `opendal-gcs` feature. Could these be `use` items gated with `#[cfg(any(feature = "opendal-s3", feature = "opendal-gcs"))]`, like the rest of the crate imports its names? The same goes for the three `std::sync::Once` uses in [`file_io.rs`](https://github.com/apache/iceberg-rust/blob/f631771198df4b98b251c391b43aadde060579cb/crates/iceberg/src/io/file_io.rs#L104), where `Once` can join the existing `use std::sync::{Arc, OnceLock};`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
