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 no progress result printer to s3 transfer #2747

Merged
merged 5 commits into from Oct 13, 2017

Conversation

Projects
None yet
7 participants
@joguSD
Contributor

joguSD commented Aug 3, 2017

This seemed relatively easy to implement and a lot of people have been asking for it.
This is basically a copy paste of the OnlyShowErrorsResultPrinter except that it still prints success messages. Just like the --only-show-errors flag you can now also pass --no-progress to omit the progress rendering. Not sure if I hit all the tests we need for this.

Implements: #519

@jamesls

Had some small feedback.

Show outdated Hide outdated awscli/customizations/s3/subcommands.py Outdated
Show outdated Hide outdated awscli/customizations/s3/subcommands.py Outdated
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 21, 2017

Codecov Report

Merging #2747 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2747      +/-   ##
===========================================
+ Coverage    95.58%   95.59%   +<.01%     
===========================================
  Files          151      151              
  Lines        11848    11860      +12     
===========================================
+ Hits         11325    11337      +12     
  Misses         523      523
Impacted Files Coverage Δ
awscli/customizations/s3/results.py 98.55% <100%> (+0.01%) ⬆️
awscli/customizations/s3/subcommands.py 97.16% <100%> (ø) ⬆️
awscli/customizations/s3/s3handler.py 97.79% <100%> (+0.02%) ⬆️
awscli/customizations/preview.py 100% <0%> (ø) ⬆️
awscli/customizations/cloudformation/deploy.py 100% <0%> (ø) ⬆️
awscli/clidocs.py 98.35% <0%> (ø) ⬆️
awscli/customizations/argrename.py 100% <0%> (ø) ⬆️
awscli/customizations/cloudformation/deployer.py 96.29% <0%> (+0.24%) ⬆️

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 7cd785b...cf8c3f3. Read the comment docs.

codecov-io commented Sep 21, 2017

Codecov Report

Merging #2747 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2747      +/-   ##
===========================================
+ Coverage    95.58%   95.59%   +<.01%     
===========================================
  Files          151      151              
  Lines        11848    11860      +12     
===========================================
+ Hits         11325    11337      +12     
  Misses         523      523
Impacted Files Coverage Δ
awscli/customizations/s3/results.py 98.55% <100%> (+0.01%) ⬆️
awscli/customizations/s3/subcommands.py 97.16% <100%> (ø) ⬆️
awscli/customizations/s3/s3handler.py 97.79% <100%> (+0.02%) ⬆️
awscli/customizations/preview.py 100% <0%> (ø) ⬆️
awscli/customizations/cloudformation/deploy.py 100% <0%> (ø) ⬆️
awscli/clidocs.py 98.35% <0%> (ø) ⬆️
awscli/customizations/argrename.py 100% <0%> (ø) ⬆️
awscli/customizations/cloudformation/deployer.py 96.29% <0%> (+0.24%) ⬆️

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 7cd785b...cf8c3f3. Read the comment docs.

@c0reysc0tt

c0reysc0tt approved these changes Sep 28, 2017 edited

I really hope this gets merged into an upcoming version soon. I manually made these changes to the three python files affected, and now the log files created by all my s3 cron scripts are useful and readable. As far as I can tell, it's a complete fix to a problem people have been complaining about for years. The --no progress option is documented in the help files the same as any other function and does what it says it does. Thank you joguSD!

@kyleknap

Looks good. I just had some very small comments. Also make sure you run the integration tests to make sure nothing is broken.

@@ -369,6 +369,15 @@
'output is suppressed.')}
NO_PROGRESS = {'name': 'no-progress',
'action': 'store_false',
'dest': 'progress',

This comment has been minimized.

@kyleknap

kyleknap Oct 12, 2017

Member

Not sure it is needed but all of the parameters like this have a 'default': True in the dictionary. Not sure if it is needed. Do not think it is. But it would not hurt to be consistent.

@kyleknap

kyleknap Oct 12, 2017

Member

Not sure it is needed but all of the parameters like this have a 'default': True in the dictionary. Not sure if it is needed. Do not think it is. But it would not hurt to be consistent.

This comment has been minimized.

@joguSD

joguSD Oct 13, 2017

Contributor

The other two flags like this don't have a default either, I think it's fine to have no default.

@joguSD

joguSD Oct 13, 2017

Contributor

The other two flags like this don't have a default either, I think it's fine to have no default.

This comment has been minimized.

@kyleknap

kyleknap Oct 13, 2017

Member

I was referring to all of the boolean flags that have store_false as the action. I just checked the docs and did it on the command line and its fine. Putting the default kwarg whose value is True is redundant.

@kyleknap

kyleknap Oct 13, 2017

Member

I was referring to all of the boolean flags that have store_false as the action. I just checked the docs and did it on the command line and its fine. Putting the default kwarg whose value is True is redundant.

'dest': 'progress',
'help_text': (
'File transfer progress is not displayed. This flag '
'is only applied when the quiet and only-show-errors '

This comment has been minimized.

@kyleknap

kyleknap Oct 12, 2017

Member

Does the second sentence even matter? Based on the --quiet and --only-show-errors documentation, it will not show progress anyways. I would prefer to remove it in case we do not cause any confusion in the future if we add a new output parameter and forget to update this documentation.

@kyleknap

kyleknap Oct 12, 2017

Member

Does the second sentence even matter? Based on the --quiet and --only-show-errors documentation, it will not show progress anyways. I would prefer to remove it in case we do not cause any confusion in the future if we add a new output parameter and forget to update this documentation.

This comment has been minimized.

@joguSD

joguSD Oct 12, 2017

Contributor

The second sentence was added in response to some of James' feedback.

@joguSD

joguSD Oct 12, 2017

Contributor

The second sentence was added in response to some of James' feedback.

This comment has been minimized.

@kyleknap

kyleknap Oct 12, 2017

Member

Oh its because it is conflicting. Like if --no-progress --quiet is provided which one would win? Yeah I'm fine with keeping it as is.

@kyleknap

kyleknap Oct 12, 2017

Member

Oh its because it is conflicting. Like if --no-progress --quiet is provided which one would win? Yeah I'm fine with keeping it as is.

# Copy file into bucket.
p = aws('s3 cp %s s3://%s/ --no-progress' % (foo_txt, bucket_name))
self.assertEqual(p.rc, 0)
# Ensure success message was printed

This comment has been minimized.

@kyleknap

kyleknap Oct 12, 2017

Member

It might be good to assert there was no progress statement as well in the standard output.

@kyleknap

kyleknap Oct 12, 2017

Member

It might be good to assert there was no progress statement as well in the standard output.

This comment has been minimized.

@joguSD

joguSD Oct 13, 2017

Contributor

Updated.

@joguSD

joguSD Oct 13, 2017

Contributor

Updated.

@kyleknap

Looks good. 🚢

@joguSD joguSD merged commit 2006dc0 into aws:develop Oct 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment