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

Add --request-payer to transfer commands #3147

Merged
merged 3 commits into from
Feb 19, 2018

Conversation

kyleknap
Copy link
Contributor

This builds on this previous PR: #3016

Compared to the original PR, there was a lot more lines added. For the most part, the logic from the original PR stayed intact. Just I added a lot of functional tests for this as practically all API requests used in a command will require the request payer header or the command will fail, and so I wanted to be sure that I caught all of the different scenarios (good thing I did too because I found a bug in s3transfer as well related to this). I also tested it locally on a bucket enabled for requester pays using an account that does not own the bucket and it was working well there. For anyone following/reviewing this PR, feel free to try it out locally as well and that will help double check I did not miss anything.

Also, note tests won't pass until this PR gets merged from s3transfer: boto/s3transfer#103

@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #3147 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3147      +/-   ##
===========================================
+ Coverage    97.09%   97.11%   +0.01%     
===========================================
  Files          406      407       +1     
  Lines        33362    33515     +153     
===========================================
+ Hits         32394    32547     +153     
  Misses         968      968
Impacted Files Coverage Δ
awscli/customizations/s3/subcommands.py 97.21% <100%> (+0.05%) ⬆️
tests/functional/s3/test_mv_command.py 100% <100%> (+2.17%) ⬆️
awscli/customizations/s3/filegenerator.py 98.91% <100%> (ø) ⬆️
tests/functional/s3/test_cp_command.py 100% <100%> (ø) ⬆️
awscli/customizations/s3/s3handler.py 97.8% <100%> (ø) ⬆️
tests/functional/s3/test_sync_command.py 100% <100%> (ø) ⬆️
tests/unit/customizations/s3/test_utils.py 99.58% <100%> (+0.05%) ⬆️
tests/functional/s3/test_rm_command.py 100% <100%> (ø) ⬆️
tests/functional/s3/__init__.py 100% <100%> (ø)
awscli/customizations/s3/utils.py 97.97% <100%> (+0.11%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 017279e...fb231df. Read the comment docs.

Copy link
Member

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I played around a little bit with it and it seems to work.

@kyleknap kyleknap merged commit 5d34910 into aws:develop Feb 19, 2018
@kyleknap kyleknap deleted the add-requester-pays-to-sync branch February 19, 2018 20:03
@aquanyc
Copy link

aquanyc commented Feb 23, 2018

@kyleknap

When do you think there will be a version we can pull for testing ?

@kyleknap
Copy link
Contributor Author

@aquanyc It was recently released in version 1.14.42. So I would recommend upgrading to the latest version of the CLI to start using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants