-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from all commits
dd03751
8b564db
0f9fb95
42496d9
cf8c3f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -369,6 +369,15 @@ | |
'output is suppressed.')} | ||
|
||
|
||
NO_PROGRESS = {'name': 'no-progress', | ||
'action': 'store_false', | ||
'dest': 'progress', | ||
'help_text': ( | ||
'File transfer progress is not displayed. This flag ' | ||
'is only applied when the quiet and only-show-errors ' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the second sentence even matter? Based on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second sentence was added in response to some of James' feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh its because it is conflicting. Like if |
||
'flags are not provided.')} | ||
|
||
|
||
EXPECTED_SIZE = {'name': 'expected-size', | ||
'help_text': ( | ||
'This argument specifies the expected size of a stream ' | ||
|
@@ -424,7 +433,7 @@ | |
SSE_C_COPY_SOURCE_KEY, STORAGE_CLASS, GRANTS, | ||
WEBSITE_REDIRECT, CONTENT_TYPE, CACHE_CONTROL, | ||
CONTENT_DISPOSITION, CONTENT_ENCODING, CONTENT_LANGUAGE, | ||
EXPIRES, SOURCE_REGION, ONLY_SHOW_ERRORS, | ||
EXPIRES, SOURCE_REGION, ONLY_SHOW_ERRORS, NO_PROGRESS, | ||
PAGE_SIZE, IGNORE_GLACIER_WARNINGS, FORCE_GLACIER_TRANSFER] | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1255,6 +1255,19 @@ def test_normal_output_only_show_errors(self): | |
# Check that nothing was printed to stdout. | ||
self.assertEqual('', p.stdout) | ||
|
||
def test_normal_output_no_progress(self): | ||
bucket_name = _SHARED_BUCKET | ||
foo_txt = self.files.create_file('foo.txt', 'foo contents') | ||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be good to assert there was no progress statement as well in the standard output. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
self.assertIn('upload', p.stdout) | ||
self.assertIn('s3://%s/foo.txt' % bucket_name, p.stdout) | ||
self.assertNotIn('Completed ', p.stdout) | ||
self.assertNotIn('calculating', p.stdout) | ||
|
||
def test_error_output(self): | ||
foo_txt = self.files.create_file('foo.txt', 'foo contents') | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other two flags like this don't have a default either, I think it's fine to have no default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thedefault
kwarg whose value isTrue
is redundant.