tustvold commented on code in PR #3436:
URL: https://github.com/apache/arrow-rs/pull/3436#discussion_r1061363346
##########
object_store/src/aws/mod.rs:
##########
@@ -472,6 +623,50 @@ impl AmazonS3Builder {
self
}
+ /// Set an option on the builder via a key - value pair.
+ pub fn with_option(
+ mut self,
+ key: impl Into<String>,
+ value: impl Into<String>,
+ ) -> Self {
+ match AmazonS3ConfigKey::try_from(key.into()) {
+ Ok(AmazonS3ConfigKey::AccessKeyId) => self.access_key_id =
Some(value.into()),
+ Ok(AmazonS3ConfigKey::SecretAccessKey) => {
+ self.secret_access_key = Some(value.into())
+ }
+ Ok(AmazonS3ConfigKey::Region) => self.region = Some(value.into()),
+ Ok(AmazonS3ConfigKey::Bucket) => self.bucket_name =
Some(value.into()),
+ Ok(AmazonS3ConfigKey::Endpoint) => self.endpoint =
Some(value.into()),
+ Ok(AmazonS3ConfigKey::Token) => self.token = Some(value.into()),
+ Ok(AmazonS3ConfigKey::ImdsV1Fallback) => {
+ self.imdsv1_fallback = str_is_truthy(&value.into())
+ }
+ Ok(AmazonS3ConfigKey::VirtualHostedStyleRequest) => {
+ self.virtual_hosted_style_request =
str_is_truthy(&value.into())
+ }
+ Ok(AmazonS3ConfigKey::DefaultRegion) => {
+ self.region = self.region.or_else(|| Some(value.into()))
+ }
+ Ok(AmazonS3ConfigKey::MetadataEndpoint) => {
+ self.metadata_endpoint = Some(value.into())
+ }
+ Ok(AmazonS3ConfigKey::Profile) => self.profile =
Some(value.into()),
+ Err(_) => (),
+ };
+ self
+ }
+
+ /// Hydrate builder from key value pairs
+ pub fn with_options(
+ mut self,
+ options: &HashMap<impl Into<String> + Clone, String>,
Review Comment:
```suggestion
pub fn with_options<I: IntoIterator<Item=(String, String)>>(
mut self,
options: I,
```
##########
object_store/src/aws/mod.rs:
##########
@@ -472,6 +623,50 @@ impl AmazonS3Builder {
self
}
+ /// Set an option on the builder via a key - value pair.
+ pub fn with_option(
+ mut self,
+ key: impl Into<String>,
+ value: impl Into<String>,
+ ) -> Self {
+ match AmazonS3ConfigKey::try_from(key.into()) {
+ Ok(AmazonS3ConfigKey::AccessKeyId) => self.access_key_id =
Some(value.into()),
+ Ok(AmazonS3ConfigKey::SecretAccessKey) => {
+ self.secret_access_key = Some(value.into())
+ }
+ Ok(AmazonS3ConfigKey::Region) => self.region = Some(value.into()),
+ Ok(AmazonS3ConfigKey::Bucket) => self.bucket_name =
Some(value.into()),
+ Ok(AmazonS3ConfigKey::Endpoint) => self.endpoint =
Some(value.into()),
+ Ok(AmazonS3ConfigKey::Token) => self.token = Some(value.into()),
+ Ok(AmazonS3ConfigKey::ImdsV1Fallback) => {
+ self.imdsv1_fallback = str_is_truthy(&value.into())
+ }
+ Ok(AmazonS3ConfigKey::VirtualHostedStyleRequest) => {
+ self.virtual_hosted_style_request =
str_is_truthy(&value.into())
+ }
+ Ok(AmazonS3ConfigKey::DefaultRegion) => {
+ self.region = self.region.or_else(|| Some(value.into()))
+ }
+ Ok(AmazonS3ConfigKey::MetadataEndpoint) => {
+ self.metadata_endpoint = Some(value.into())
+ }
+ Ok(AmazonS3ConfigKey::Profile) => self.profile =
Some(value.into()),
+ Err(_) => (),
Review Comment:
This feels unfortunate, if I typo a config name I would ideally like to get
an error message
##########
object_store/src/util.rs:
##########
@@ -185,6 +185,15 @@ fn merge_ranges(
ret
}
+#[allow(dead_code)]
+pub(crate) fn str_is_truthy(val: &str) -> bool {
+ val == "1"
+ || val.to_lowercase() == "true"
+ || val.to_lowercase() == "on"
+ || val.to_lowercase() == "yes"
+ || val.to_lowercase() == "y"
Review Comment:
FWIW you can use
https://doc.rust-lang.org/std/primitive.str.html#method.eq_ignore_ascii_case to
avoid an allocation
##########
object_store/src/aws/mod.rs:
##########
@@ -472,6 +623,50 @@ impl AmazonS3Builder {
self
}
+ /// Set an option on the builder via a key - value pair.
+ pub fn with_option(
+ mut self,
+ key: impl Into<String>,
Review Comment:
```suggestion
key: impl AsRef<str>
```
We don't actually need an owned string
##########
object_store/src/aws/mod.rs:
##########
@@ -379,6 +383,170 @@ pub struct AmazonS3Builder {
client_options: ClientOptions,
}
+/// Configuration keys for [`AmazonS3Builder`]
+#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)]
+pub enum AmazonS3ConfigKey {
+ /// AWS Access Key
+ ///
+ /// See [`AmazonS3Builder::with_access_key_id`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_access_key_id`
+ /// - `access_key_id`
+ AccessKeyId,
+
+ /// Secret Access Key
+ ///
+ /// See [`AmazonS3Builder::with_secret_access_key`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_secret_access_key`
+ /// - `secret_access_key`
+ SecretAccessKey,
+
+ /// Region
+ ///
+ /// See [`AmazonS3Builder::with_region`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_region`
+ /// - `region`
+ Region,
+
+ /// Default region
+ ///
+ /// See [`AmazonS3Builder::with_region`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_default_region`
+ /// - `default_region`
+ DefaultRegion,
+
+ /// Bucket name
+ ///
+ /// See [`AmazonS3Builder::with_bucket_name`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_bucket`
+ /// - `aws_bucket_name`
+ /// - `bucket`
+ /// - `bucket_name`
+ Bucket,
+
+ /// Sets custom endpoint for communicating with AWS S3.
+ ///
+ /// See [`AmazonS3Builder::with_endpoint`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_endpoint`
+ /// - `aws_endpoint_url`
+ /// - `endpoint`
+ /// - `endpoint_url`
+ Endpoint,
+
+ /// Token to use for requests (passed to underlying provider)
+ ///
+ /// See [`AmazonS3Builder::with_token`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_session_token`
+ /// - `aws_token`
+ /// - `session_token`
+ /// - `token`
+ Token,
+
+ /// Fall back to ImdsV1
+ ///
+ /// See [`AmazonS3Builder::with_imdsv1_fallback`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_imdsv1_fallback`
+ /// - `imdsv1_fallback`
+ ImdsV1Fallback,
+
+ /// If virtual hosted style request has to be used
+ ///
+ /// See [`AmazonS3Builder::with_virtual_hosted_style_request`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_virtual_hosted_style_request`
+ /// - `virtual_hosted_style_request`
+ VirtualHostedStyleRequest,
+
+ /// Set the instance metadata endpoint
+ ///
+ /// See [`AmazonS3Builder::with_metadata_endpoint`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_metadata_endpoint`
+ /// - `metadata_endpoint`
+ MetadataEndpoint,
+
+ /// AWS profile name
+ ///
+ /// Supported keys:
+ /// - `aws_profile`
+ /// - `profile`
+ Profile,
+}
+
+impl From<AmazonS3ConfigKey> for String {
+ fn from(value: AmazonS3ConfigKey) -> Self {
+ match value {
+ AmazonS3ConfigKey::AccessKeyId => Self::from("aws_access_key_id"),
+ AmazonS3ConfigKey::SecretAccessKey =>
Self::from("aws_secret_access_key"),
+ AmazonS3ConfigKey::Region => Self::from("aws_region"),
+ AmazonS3ConfigKey::Bucket => Self::from("aws_bucket"),
+ AmazonS3ConfigKey::Endpoint => Self::from("aws_endpoint"),
+ AmazonS3ConfigKey::Token => Self::from("aws_session_token"),
+ AmazonS3ConfigKey::ImdsV1Fallback =>
Self::from("aws_imdsv1_fallback"),
+ AmazonS3ConfigKey::VirtualHostedStyleRequest => {
+ Self::from("aws_virtual_hosted_style_request")
+ }
+ AmazonS3ConfigKey::DefaultRegion =>
Self::from("aws_default_region"),
+ AmazonS3ConfigKey::MetadataEndpoint =>
Self::from("aws_metadata_endpoint"),
+ AmazonS3ConfigKey::Profile => Self::from("aws_profile"),
+ }
+ }
+}
+
+impl TryFrom<&str> for AmazonS3ConfigKey {
+ type Error = super::Error;
+
+ fn try_from(value: &str) -> Result<Self> {
+ match value.to_ascii_lowercase().as_str() {
Review Comment:
I don't feel very strongly about this, but IMO this should be case
sensitive, and if the user wishes their config system to be case insensitive,
they can normalize themselves. This covers both cases, whereas as written this
forces all config to be case insensitive.
It will also be marginally faster :smile:
##########
object_store/src/azure/mod.rs:
##########
@@ -409,29 +555,12 @@ impl MicrosoftAzureBuilder {
/// ```
pub fn from_env() -> Self {
let mut builder = Self::default();
-
- if let Ok(account_name) = std::env::var("AZURE_STORAGE_ACCOUNT_NAME") {
- builder.account_name = Some(account_name);
- }
-
- if let Ok(access_key) = std::env::var("AZURE_STORAGE_ACCOUNT_KEY") {
- builder.access_key = Some(access_key);
- } else if let Ok(access_key) =
std::env::var("AZURE_STORAGE_ACCESS_KEY") {
- builder.access_key = Some(access_key);
- }
-
- if let Ok(client_id) = std::env::var("AZURE_STORAGE_CLIENT_ID") {
- builder.client_id = Some(client_id);
- }
-
- if let Ok(client_secret) =
std::env::var("AZURE_STORAGE_CLIENT_SECRET") {
- builder.client_secret = Some(client_secret);
- }
-
- if let Ok(tenant_id) = std::env::var("AZURE_STORAGE_TENANT_ID") {
- builder.tenant_id = Some(tenant_id);
+ for (key, value) in std::env::vars()
Review Comment:
I think we should probably prefer `std::env::var_os` as `vars` panics if ANY
environment variable is not unicode
##########
object_store/src/aws/mod.rs:
##########
@@ -379,6 +383,170 @@ pub struct AmazonS3Builder {
client_options: ClientOptions,
}
+/// Configuration keys for [`AmazonS3Builder`]
+#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)]
+pub enum AmazonS3ConfigKey {
+ /// AWS Access Key
+ ///
+ /// See [`AmazonS3Builder::with_access_key_id`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_access_key_id`
+ /// - `access_key_id`
+ AccessKeyId,
+
+ /// Secret Access Key
+ ///
+ /// See [`AmazonS3Builder::with_secret_access_key`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_secret_access_key`
+ /// - `secret_access_key`
+ SecretAccessKey,
+
+ /// Region
+ ///
+ /// See [`AmazonS3Builder::with_region`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_region`
+ /// - `region`
+ Region,
+
+ /// Default region
+ ///
+ /// See [`AmazonS3Builder::with_region`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_default_region`
+ /// - `default_region`
+ DefaultRegion,
+
+ /// Bucket name
+ ///
+ /// See [`AmazonS3Builder::with_bucket_name`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_bucket`
+ /// - `aws_bucket_name`
+ /// - `bucket`
+ /// - `bucket_name`
+ Bucket,
+
+ /// Sets custom endpoint for communicating with AWS S3.
+ ///
+ /// See [`AmazonS3Builder::with_endpoint`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_endpoint`
+ /// - `aws_endpoint_url`
+ /// - `endpoint`
+ /// - `endpoint_url`
+ Endpoint,
+
+ /// Token to use for requests (passed to underlying provider)
+ ///
+ /// See [`AmazonS3Builder::with_token`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_session_token`
+ /// - `aws_token`
+ /// - `session_token`
+ /// - `token`
+ Token,
+
+ /// Fall back to ImdsV1
+ ///
+ /// See [`AmazonS3Builder::with_imdsv1_fallback`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_imdsv1_fallback`
+ /// - `imdsv1_fallback`
+ ImdsV1Fallback,
+
+ /// If virtual hosted style request has to be used
+ ///
+ /// See [`AmazonS3Builder::with_virtual_hosted_style_request`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_virtual_hosted_style_request`
+ /// - `virtual_hosted_style_request`
+ VirtualHostedStyleRequest,
+
+ /// Set the instance metadata endpoint
+ ///
+ /// See [`AmazonS3Builder::with_metadata_endpoint`] for details.
+ ///
+ /// Supported keys:
+ /// - `aws_metadata_endpoint`
+ /// - `metadata_endpoint`
+ MetadataEndpoint,
+
+ /// AWS profile name
+ ///
+ /// Supported keys:
+ /// - `aws_profile`
+ /// - `profile`
+ Profile,
+}
+
+impl From<AmazonS3ConfigKey> for String {
+ fn from(value: AmazonS3ConfigKey) -> Self {
+ match value {
+ AmazonS3ConfigKey::AccessKeyId => Self::from("aws_access_key_id"),
+ AmazonS3ConfigKey::SecretAccessKey =>
Self::from("aws_secret_access_key"),
+ AmazonS3ConfigKey::Region => Self::from("aws_region"),
+ AmazonS3ConfigKey::Bucket => Self::from("aws_bucket"),
+ AmazonS3ConfigKey::Endpoint => Self::from("aws_endpoint"),
+ AmazonS3ConfigKey::Token => Self::from("aws_session_token"),
+ AmazonS3ConfigKey::ImdsV1Fallback =>
Self::from("aws_imdsv1_fallback"),
+ AmazonS3ConfigKey::VirtualHostedStyleRequest => {
+ Self::from("aws_virtual_hosted_style_request")
+ }
+ AmazonS3ConfigKey::DefaultRegion =>
Self::from("aws_default_region"),
+ AmazonS3ConfigKey::MetadataEndpoint =>
Self::from("aws_metadata_endpoint"),
+ AmazonS3ConfigKey::Profile => Self::from("aws_profile"),
+ }
+ }
+}
Review Comment:
Perhaps we could instead implement `AsRef<str>` to avoid unnecessary
allocations?
##########
object_store/src/azure/mod.rs:
##########
@@ -367,13 +377,149 @@ pub struct MicrosoftAzureBuilder {
client_secret: Option<String>,
tenant_id: Option<String>,
sas_query_pairs: Option<Vec<(String, String)>>,
+ sas_key: Option<String>,
authority_host: Option<String>,
url: Option<String>,
use_emulator: bool,
retry_config: RetryConfig,
client_options: ClientOptions,
}
+/// Configuration keys for [`MicrosoftAzureBuilder`]
+#[derive(PartialEq, Eq, Hash, Clone, Debug, Copy)]
+pub enum AzureConfigKey {
+ /// The name of the azure storage account
+ ///
+ /// Supported keys:
+ /// - `azure_storage_account_name`
+ /// - `account_name`
+ AccountName,
+
+ /// Master key for accessing storage account
+ ///
+ /// Supported keys:
+ /// - `azure_storage_account_key`
+ /// - `azure_storage_access_key`
+ /// - `azure_storage_master_key`
+ /// - `access_key`
+ /// - `account_key`
+ /// - `master_key`
+ AccessKey,
+
+ /// Service principal client id for authorizing requests
+ ///
+ /// Supported keys:
+ /// - `azure_storage_client_id`
+ /// - `azure_client_id`
+ /// - `client_id`
+ ClientId,
+
+ /// Service principal client secret for authorizing requests
+ ///
+ /// Supported keys:
+ /// - `azure_storage_client_secret`
+ /// - `azure_client_secret`
+ /// - `client_secret`
+ ClientSecret,
+
+ /// Tenant id used in oauth flows
+ ///
+ /// Supported keys:
+ /// - `azure_storage_tenant_id`
+ /// - `azure_storage_authority_id`
+ /// - `azure_tenant_id`
+ /// - `azure_authority_id`
+ /// - `tenant_id`
+ /// - `authority_id`
+ AuthorityId,
+
+ /// Shared access signature.
+ ///
+ /// The signature is expected to be percent-encoded, much like they are
provided
+ /// in the azure storage explorer or azure protal.
Review Comment:
```suggestion
/// in the azure storage explorer or azure portal.
```
--
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]
