jdockerty commented on code in PR #520:
URL: https://github.com/apache/iceberg-rust/pull/520#discussion_r1704182016
##########
crates/iceberg/src/io/storage.rs:
##########
@@ -117,7 +125,24 @@ impl Storage {
))
}
}
- #[cfg(all(not(feature = "storage-s3"), not(feature =
"storage-fs")))]
+ #[cfg(feature = "storage-gcs")]
+ Storage::Gcs { config } => {
+ let operator = super::gcs_config_build(config)?;
+ let prefix = format!("gs://{}/", operator.info().name());
+ if path.starts_with(&prefix) {
+ Ok((operator, &path[prefix.len()..]))
+ } else {
+ Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!("Invalid gcs url: {}, should start with {}",
path, prefix),
+ ))
+ }
+ }
+ #[cfg(all(
+ not(feature = "storage-s3"),
+ not(feature = "storage-fs"),
+ not(feature = "storage-gcs")
+ ))]
Review Comment:
This is an extension to the prior check which returns the `Error` when there
are no features enabled.
I believe this needs to be altered for each new `storage-$TYPE` which is
introduced? For example, it was previously set to this:
```rust
#[cfg(all(not(feature = "storage-s3"), not(feature = "storage-fs")))]
// ...
```
Once I added the `not(feature = "storage-gcs")` too, the formatting moves
this onto separate lines for readability.
**Are you saying I do _not_ need the addition of `not(feature =
"storage-gcs")` here?**
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
