Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly translate supported confi values to TransferConfig #2146

Merged
merged 1 commit into from
Sep 1, 2016

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Sep 1, 2016

The previous implementation made the open-ended assumption that
runtime config translates 1-1 to transfer configs.
This also had the (unintended) side effect that everything in
transfer config was implicitly supported in the ~/.aws/config
file because the config is passed directly to TransferConfig.

This also generates errors if you pass an unknown option to the
TransferConfig, both for nonsensical values as well as valid
values supported in ~/.aws/config but not in TransferConfig.
I noticed this because I had addressing_style=path
in my ~/.aws/config file:

$ aws s3 cp foo s3://bucket/foo

__init__() got an unexpected keyword argument 'addressing_style'

I've updated the code to have a fixed list of supported config values.
This means that we need to explicitly update this list as we decide
to support for transfer config options.

cc @kyleknap @JordonPhillips

@jamesls jamesls added the pr:needs-review This PR needs a review from a Member. label Sep 1, 2016
@kyleknap
Copy link
Contributor

kyleknap commented Sep 1, 2016

Looks good. 🚢 assuming test pass.

The previous implementation made the open-ended assumption that
runtime config translates 1-1 to transfer configs.
This also had the (unintended) side effect that everything in
transfer config was implicitly supported in the ~/.aws/config
file because the config is passed directly to `TransferConfig`.

I've updated the code to have a fixed list of supported config values.
This means that we need to explicitly update this list as we decide
to support for transfer config options.
@jamesls jamesls changed the base branch from integrate-s3transfer to develop September 1, 2016 17:42
@jamesls
Copy link
Member Author

jamesls commented Sep 1, 2016

Ok I've branched off of develop and set develop as the base branch.

@kyleknap
Copy link
Contributor

kyleknap commented Sep 1, 2016

Still 🚢

@jamesls jamesls merged commit 777b0aa into aws:develop Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants