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

Merged
merged 5 commits into from
Oct 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions awscli/customizations/s3/results.py
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,12 @@ def _clear_progress_if_no_more_expected_transfers(self, **kwargs):
uni_print(self._adjust_statement_padding(''), self._out_file)


class NoProgressResultPrinter(ResultPrinter):
"""A result printer that doesn't print progress"""
def _print_progress(self, **kwargs):
pass


class OnlyShowErrorsResultPrinter(ResultPrinter):
"""A result printer that only prints out errors"""
def _print_progress(self, **kwargs):
Expand Down
3 changes: 3 additions & 0 deletions awscli/customizations/s3/s3handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from awscli.customizations.s3.results import ResultRecorder
from awscli.customizations.s3.results import ResultPrinter
from awscli.customizations.s3.results import OnlyShowErrorsResultPrinter
from awscli.customizations.s3.results import NoProgressResultPrinter
from awscli.customizations.s3.results import ResultProcessor
from awscli.customizations.s3.results import CommandResultRecorder
from awscli.customizations.s3.utils import RequestParamsMapper
Expand Down Expand Up @@ -110,6 +111,8 @@ def _add_result_printer(self, result_recorder, result_processor_handlers):
result_printer = OnlyShowErrorsResultPrinter(result_recorder)
elif self._cli_params.get('is_stream'):
result_printer = OnlyShowErrorsResultPrinter(result_recorder)
elif not self._cli_params.get('progress'):
result_printer = NoProgressResultPrinter(result_recorder)
else:
result_printer = ResultPrinter(result_recorder)
result_processor_handlers.append(result_printer)
Expand Down
11 changes: 10 additions & 1 deletion awscli/customizations/s3/subcommands.py
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,15 @@
'output is suppressed.')}


NO_PROGRESS = {'name': 'no-progress',
'action': 'store_false',
'dest': 'progress',
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 the default kwarg whose value is True is redundant.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kyleknap kyleknap Oct 12, 2017

Choose a reason for hiding this comment

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

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.

'flags are not provided.')}


EXPECTED_SIZE = {'name': 'expected-size',
'help_text': (
'This argument specifies the expected size of a stream '
Expand Down Expand Up @@ -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]


Expand Down
13 changes: 13 additions & 0 deletions tests/integration/customizations/s3/test_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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')

Expand Down
68 changes: 68 additions & 0 deletions tests/unit/customizations/s3/test_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
from awscli.customizations.s3.results import ResultRecorder
from awscli.customizations.s3.results import ResultPrinter
from awscli.customizations.s3.results import OnlyShowErrorsResultPrinter
from awscli.customizations.s3.results import NoProgressResultPrinter
from awscli.customizations.s3.results import ResultProcessor
from awscli.customizations.s3.results import CommandResultRecorder
from awscli.customizations.s3.utils import relative_path
Expand Down Expand Up @@ -1517,6 +1518,73 @@ def test_print_unicode_error(self):
self.assertEqual(self.error_file.getvalue(), ref_error_statement)


class TestNoProgressResultPrinter(BaseResultPrinterTest):
def setUp(self):
super(TestNoProgressResultPrinter, self).setUp()
self.result_printer = NoProgressResultPrinter(
result_recorder=self.result_recorder,
out_file=self.out_file,
error_file=self.error_file
)

def test_does_not_print_progress_result(self):
progress_result = self.get_progress_result()
self.result_printer(progress_result)
self.assertEqual(self.out_file.getvalue(), '')

def test_does_print_sucess_result(self):
transfer_type = 'upload'
src = 'file'
dest = 's3://mybucket/mykey'
success_result = SuccessResult(
transfer_type=transfer_type, src=src, dest=dest)

self.result_printer(success_result)
expected_message = 'upload: file to s3://mybucket/mykey\n'
self.assertEqual(self.out_file.getvalue(), expected_message)

def test_print_failure_result(self):
transfer_type = 'upload'
src = 'file'
dest = 's3://mybucket/mykey'
failure_result = FailureResult(
transfer_type=transfer_type, src=src, dest=dest,
exception=Exception('my exception'))

self.result_printer(failure_result)

ref_failure_statement = (
'upload failed: file to s3://mybucket/mykey my exception\n'
)
self.assertEqual(self.error_file.getvalue(), ref_failure_statement)

def test_print_warnings_result(self):
self.result_printer(WarningResult('warning: my warning'))
ref_warning_statement = 'warning: my warning\n'
self.assertEqual(self.error_file.getvalue(), ref_warning_statement)

def test_final_total_does_not_try_to_clear_empty_progress(self):
transfer_type = 'upload'
src = 'file'
dest = 's3://mybucket/mykey'

mb = 1024 * 1024
self.result_recorder.expected_files_transferred = 1
self.result_recorder.files_transferred = 1
self.result_recorder.expected_bytes_transferred = mb
self.result_recorder.bytes_transferred = mb

success_result = SuccessResult(
transfer_type=transfer_type, src=src, dest=dest)
self.result_printer(success_result)
ref_statement = 'upload: file to s3://mybucket/mykey\n'
self.assertEqual(self.out_file.getvalue(), ref_statement)

self.result_recorder.final_expected_files_transferred = 1
self.result_printer(FinalTotalSubmissionsResult(1))
self.assertEqual(self.out_file.getvalue(), ref_statement)


class TestOnlyShowErrorsResultPrinter(BaseResultPrinterTest):
def setUp(self):
super(TestOnlyShowErrorsResultPrinter, self).setUp()
Expand Down