[Merged by Bors] - Add pathStyleAccess to S3ConnectionSpec

[siegfriedweber]

Description

pathStyleAccess added to commons::s3::S3ConnectionSpec

Review Checklist

• Code contains useful comments
• (Integration-)Test cases added (or not applicable)
• Documentation added (or not applicable)
• Changelog updated (or not applicable)
• Cargo.toml only contains references to git tags (not specific commits or branches)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

[sbernauer]

I think the main question is: What is the default value? ;)
Pros of default to true:

• It should work anywhere (vs. lots of users having problems and need to figure out their S3 doesn't support host style access)
  Pros of default to false:
• Maybe better performance for AWS S3? Or lower costs from AWS S3?

So i guess we should default to true for maximum portability

[sbernauer]

And i still can't get around that the bool is optional and not just simply defaulting to true (or false).
But i guess that's the way it is :D

[maltesander]

I think the main question is: What is the default value? ;) Pros of default to true:

• It should work anywhere (vs. lots of users having problems and need to figure out their S3 doesn't support host style access)
  Pros of default to false:
• Maybe better performance for AWS S3? Or lower costs from AWS S3?

So i guess we should default to true for maximum portability

I think pathstyle access will be deprecated at some point? Not sure for every provider, at least for AWS. So for now i would default to true, but still use it in every example and set it explicitly.

[sbernauer]

Sounds good 👍
The Option actually is really bad IMHO here as we cannot simply set the default to true but instead every individual operator has to enforce the default by itself. Is there any chance of using a bool and default to true?

[maltesander]

Sounds good +1 The Option actually is really bad IMHO here as we cannot simply set the default to true but instead every individual operator has to enforce the default by itself. Is there any chance of using a bool and default to true?

I think it makes sense that each operator does it on its own. When setting that you can just have an unwrap_or() with the default best suited for the operator.

[sbernauer]

We agreeded on:

• The S3ConnectionSpec handles the default value.
• The default value is false because we then align with other tools out there like Trino or hive MS

[sbernauer]

We agreed (second time) on

#[serde(default, skip_serializing_if = "Option::is_none")]
pub access_style: Option<S3AccessStyle>,
# [...]

pub enum S3AccessStyle {
    Path,
    VirtualHosted,
}

impl Default for S3AccessStyle {
    fn default() -> Self {
        S3AccessStyle::VirtualHosted
    }
}

[maltesander]

LGTM.

[maltesander]

bors merge

[bors]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[sbernauer]

bors retry

[bors]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[sbernauer]

bors cancel

[maltesander]

bors merge

[bors]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[maltesander]

bors merge

[bors]

Stopped waiting for PR status (Github check) without running due to duplicate requests to run. You may check Bors to see that this PR is included in a batch by one of the other requests.

[bors]

Pull request successfully merged into main.

Build succeeded:

• All tests passed