Add support for results that do not print newlines via new SkipFileResult (fix printing bugs when using no-overwrite flag) (#9717)

Fixed bug when using `--no-overwrite` parameter where the last character printed to CLI would be a carriage return, which would cause incompatibility with some terminal program.
  * Keep track of files skipped.
  * Update files remaining progress computation to take into account files skipped.
  * Add support for results that do not print newlines via SkipFileResult and updates to ResultPrinter.

Author: aemous

awscli/customizations/s3/results.py

@@ -57,6 +57,7 @@ def _create_new_result_cls(name, extra_fields=None, base_cls=BaseResult):
 FailureResult = _create_new_result_cls('FailureResult', ['exception'])
 
 DryRunResult = _create_new_result_cls('DryRunResult')
+SkipFileResult = _create_new_result_cls('SkipFileResult')
 
 ErrorResult = namedtuple('ErrorResult', ['exception'])
 
@@ -132,6 +133,13 @@ def _on_failure(self, future, e):
                 self._src,
                 self._dest,
             )
+            self._result_queue.put(
+                SkipFileResult(
+                    transfer_type=self._transfer_type,
+                    src=self._src,
+                    dest=self._dest,
+                )
+            )
         else:
             self._result_queue.put(
                 FailureResult(
@@ -167,6 +175,7 @@ def __init__(self):
         self.files_transferred = 0
         self.files_failed = 0
         self.files_warned = 0
+        self.files_skipped = 0
         self.errors = 0
         self.expected_bytes_transferred = 0
         self.expected_files_transferred = 0
@@ -184,6 +193,7 @@ def __init__(self):
             SuccessResult: self._record_success_result,
             FailureResult: self._record_failure_result,
             WarningResult: self._record_warning_result,
+            SkipFileResult: self._record_skipped_file_result,
             ErrorResult: self._record_error_result,
             CtrlCResult: self._record_error_result,
             FinalTotalSubmissionsResult: self._record_final_expected_files,
@@ -299,6 +309,9 @@ def _record_failure_result(self, result, **kwargs):
         self.files_failed += 1
         self.files_transferred += 1
 
+    def _record_skipped_file_result(self, result, **kwargs):
+        self.files_skipped += 1
+
     def _record_warning_result(self, **kwargs):
         self.files_warned += 1
 
@@ -359,6 +372,7 @@ def __init__(self, result_recorder, out_file=None, error_file=None):
             SuccessResult: self._print_success,
             FailureResult: self._print_failure,
             WarningResult: self._print_warning,
+            SkipFileResult: self._print_skip,
             ErrorResult: self._print_error,
             CtrlCResult: self._print_ctrl_c,
             DryRunResult: self._print_dry_run,
@@ -375,6 +389,10 @@ def _print_noop(self, **kwargs):
         # If the result does not have a handler, then do nothing with it.
         pass
 
+    def _print_skip(self, **kwargs):
+        # Don't reset progress length since this result printer doesn't print a newline
+        self._redisplay_progress(reset_progress_length=False)
+
     def _print_dry_run(self, result, **kwargs):
         statement = self.DRY_RUN_FORMAT.format(
             transfer_type=result.transfer_type,
@@ -427,23 +445,26 @@ def _get_transfer_location(self, result):
             src=result.src, dest=result.dest
         )
 
-    def _redisplay_progress(self):
+    def _redisplay_progress(self, reset_progress_length=True):
         # Reset to zero because done statements are printed with new lines
         # meaning there are no carriage returns to take into account when
         # printing the next line.
-        self._progress_length = 0
+        if reset_progress_length:
+            self._progress_length = 0
         self._add_progress_if_needed()
 
     def _add_progress_if_needed(self):
         if self._has_remaining_progress():
             self._print_progress()
+        else:
+            self._clear_progress_if_no_more_expected_transfers(ending_char='\r')
 
     def _print_progress(self, **kwargs):
-        # Get all of the statistics in the correct form.
+        # Get all the statistics in the correct form.
         remaining_files = self._get_expected_total(
             str(
                 self._result_recorder.expected_files_transferred
-                - self._result_recorder.files_transferred
+                - (self._result_recorder.files_transferred + self._result_recorder.files_skipped)
             )
         )
 
@@ -506,7 +527,7 @@ def _adjust_statement_padding(self, print_statement, ending_char='\n'):
     def _has_remaining_progress(self):
         if not self._result_recorder.expected_totals_are_final():
             return True
-        actual = self._result_recorder.files_transferred
+        actual = self._result_recorder.files_transferred + self._result_recorder.files_skipped
         expected = self._result_recorder.expected_files_transferred
         return actual != expected
 
@@ -516,9 +537,9 @@ def _print_to_out_file(self, statement):
     def _print_to_error_file(self, statement):
         uni_print(statement, self._error_file)
 
-    def _clear_progress_if_no_more_expected_transfers(self, **kwargs):
+    def _clear_progress_if_no_more_expected_transfers(self, ending_char='\n', **kwargs):
         if self._progress_length and not self._has_remaining_progress():
-            uni_print(self._adjust_statement_padding(''), self._out_file)
+            uni_print(self._adjust_statement_padding('', ending_char=ending_char), self._out_file)
 
 
 class NoProgressResultPrinter(ResultPrinter):

tests/unit/customizations/s3/test_results.py

@@ -10,6 +10,7 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
 # ANY KIND, either express or implied. See the License for the specific
 # language governing permissions and limitations under the License.
+from botocore.exceptions import HTTPClientError
 from s3transfer.exceptions import CancelledError, FatalError
 
 from awscli.compat import StringIO, queue
@@ -31,6 +32,7 @@
     ResultProcessor,
     ResultRecorder,
     ShutdownThreadRequest,
+    SkipFileResult,
     SuccessResult,
 )
 from awscli.customizations.s3.utils import WarningResult
@@ -159,6 +161,27 @@ def test_on_done_failure(self):
             ),
         )
 
+    def test_on_done_precondition_failed(self):
+        subscriber = self.get_result_subscriber(DoneResultSubscriber)
+        precondition_failed = HTTPClientError(
+            request=mock.Mock(),
+            response={
+                'Error': {
+                    'Code': 'PreconditionFailed'
+                }
+            },
+            error='PreconditionFailed',
+        )
+        precondition_failed_future = self.get_failed_transfer_future(precondition_failed)
+        subscriber.on_done(precondition_failed_future)
+        result = self.get_queued_result()
+        self.assert_result_queue_is_empty()
+        self.assertEqual(
+            result,
+            SkipFileResult(transfer_type=mock.ANY, src=mock.ANY, dest=mock.ANY)
+        )
+
+
     def test_on_done_unexpected_cancelled(self):
         subscriber = self.get_result_subscriber(DoneResultSubscriber)
         cancelled_exception = FatalError('some error')
@@ -1493,6 +1516,23 @@ def test_print_unicode_error(self):
         ref_error_statement = 'fatal error: unicode exists \u2713\n'
         self.assertEqual(self.error_file.getvalue(), ref_error_statement)
 
+    def test_does_not_print_skipped_file(self):
+        transfer_type = 'upload'
+        src = 'file'
+        dest = 's3://mybucket/mykey'
+        skip_file_result = SkipFileResult(
+            transfer_type=transfer_type, src=src, dest=dest
+        )
+
+        # Pretend that this is the final result in the result queue that
+        # is processed.
+        self.result_recorder.final_expected_files_transferred = 1
+        self.result_recorder.expected_files_transferred = 1
+        self.result_recorder.files_skipped = 1
+
+        self.result_printer(skip_file_result)
+        self.assertEqual(self.out_file.getvalue(), '')
+
 
 class TestNoProgressResultPrinter(BaseResultPrinterTest):
     def setUp(self):