From d383088866bcc2889315f7c8272054557fde5bdb Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Sat, 8 Oct 2022 18:16:09 -0500 Subject: [PATCH 1/2] Make ChecksumMismatch prettier, CLI and __str__ Change: - [checksum] Make ChecksumMismatch carry differences as a class parameter so that when things like the CLI render it, they can use that information. - [checksum] Make ChecksumMismatch implement __str__ and give a pretter error. AWX currently uses str(exc) to render the errors, so this is a UX improvement there as well. - [cli] Make the UI show these errors nicer as well. Now it will truncate the listing of files by default to avoid flooding the user's screen if they forgot (for example) to exclude their .git directory. A new flag, --no-truncate, is added to disable this truncation. Also, now the files are listed out in a readable way, one per line, rather than as a literal Python dictionary representation. Test Plan: - Updated tests for the new __str__ output Signed-off-by: Rick Elrod --- src/ansible_sign/checksum/base.py | 33 ++++++++++++++++++++++++++++--- src/ansible_sign/cli.py | 29 ++++++++++++++++++++++++++- tests/test_checksum.py | 10 +++++----- 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/src/ansible_sign/checksum/base.py b/src/ansible_sign/checksum/base.py index 7c14a9d..cc04df2 100644 --- a/src/ansible_sign/checksum/base.py +++ b/src/ansible_sign/checksum/base.py @@ -11,7 +11,33 @@ class NoDifferException(Exception): class ChecksumMismatch(Exception): - pass + def __init__(self, msg, differences={}): + super().__init__(msg) + self.msg = msg + self.differences = differences + + def __str__(self): + """ + This *must* be something human-readable, it is currently used in the AWX + action plugin on project sync when validation fails. + """ + out = self.msg + extra_info = '' + if 'changes' in self.differences and self.differences['changes']: + extra_info += f"Files changed: {', '.join(self.differences['changes'])}" + + if 'added' in self.differences and self.differences['added']: + sep = "; " if extra_info else "" + extra_info += f"{sep}Files added: {', '.join(self.differences['added'])}" + + if 'removed' in self.differences and self.differences['removed']: + sep = "; " if extra_info else "" + extra_info += f"{sep}Files removed: {', '.join(self.differences['removed'])}" + + if extra_info: + return f"{out}. {extra_info}" + + return out class ChecksumFile: @@ -166,7 +192,7 @@ def verify(self, parsed_manifest_dct, diff=True): # If there are any differences in existing paths, fail the check... differences = self.diff(parsed_manifest_dct.keys()) if differences["added"] or differences["removed"]: - raise ChecksumMismatch(differences) + raise ChecksumMismatch("Files were added or removed", differences) recalculated = self.calculate_checksums_from_root(verifying=True) mismatches = set() @@ -174,6 +200,7 @@ def verify(self, parsed_manifest_dct, diff=True): if recalculated[parsed_path] != parsed_checksum: mismatches.add(parsed_path) if mismatches: - raise ChecksumMismatch(f"Checksum mismatch: {', '.join(mismatches)}") + differences = {'changes': list(mismatches)} + raise ChecksumMismatch("Checksum mismatch", differences) return True diff --git a/src/ansible_sign/cli.py b/src/ansible_sign/cli.py index c4f6a79..f094abd 100644 --- a/src/ansible_sign/cli.py +++ b/src/ansible_sign/cli.py @@ -109,6 +109,14 @@ def parse_args(self, args): dest="gnupg_home", default=None, ) + cmd_gpg_verify.add_argument( + "--no-truncate", + help="Disable truncation of file listings when enumerating file differences", + required=False, + dest="no_truncate", + default=False, + action="store_true", + ) cmd_gpg_verify.add_argument( "project_root", help="The directory containing the files being validated and verified", @@ -227,7 +235,26 @@ def validate_checksum(self): checksum.verify(manifest, diff=True) except ChecksumMismatch as e: self._error("Checksum validation failed.") - self._error(str(e)) + if e.differences: + differences = ( + ('added', 'added'), + ('removed', 'removed'), + ('changes', 'changed'), + ) + for key, verb in differences: + if key in e.differences and e.differences[key]: + self._note(f'Files {verb}:') + num_changes = len(e.differences[key]) + if self.args.no_truncate: + truncate_at = num_changes + truncated = False + else: + truncate_at = 6 + truncated = num_changes > truncate_at + for path in e.differences[key][0:(truncate_at)]: + self._note(f' - {path}') + if truncated: + self._note(f' [{num_changes - truncate_at} lines omitted, use --no-truncate to see all...]') return 2 except FileNotFoundError as e: if os.path.islink(e.filename): diff --git a/tests/test_checksum.py b/tests/test_checksum.py index ccf9120..aebe1fd 100644 --- a/tests/test_checksum.py +++ b/tests/test_checksum.py @@ -116,19 +116,19 @@ def test_parse_manifest_empty(): [ ( "files-added", - "{'added': ['hello2', 'hello3'], 'removed': []}", + "Files were added or removed. Files added: hello2, hello3", ), ( "files-added-removed", - "{'added': ['hello2', 'hello3'], 'removed': ['hello1']}", + "Files were added or removed. Files added: hello2, hello3; Files removed: hello1", ), ( "files-removed", - "{'added': [], 'removed': ['hello1']}", + "Files were added or removed. Files removed: hello1", ), ( "files-changed", - "Checksum mismatch: hello1", + "Checksum mismatch. Files changed: hello1", ), ( "success", @@ -136,7 +136,7 @@ def test_parse_manifest_empty(): ), ], ) -def test_directory_diff( +def test_directory_diff_human_readable_exc( fixture, diff_output, ): From fed2967ac3567ae3edbd820c5e0ec4b8acdb5a37 Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Sat, 8 Oct 2022 18:25:00 -0500 Subject: [PATCH 2/2] run black Signed-off-by: Rick Elrod --- src/ansible_sign/checksum/base.py | 10 +++++----- src/ansible_sign/cli.py | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/ansible_sign/checksum/base.py b/src/ansible_sign/checksum/base.py index cc04df2..48b1edd 100644 --- a/src/ansible_sign/checksum/base.py +++ b/src/ansible_sign/checksum/base.py @@ -22,15 +22,15 @@ def __str__(self): action plugin on project sync when validation fails. """ out = self.msg - extra_info = '' - if 'changes' in self.differences and self.differences['changes']: + extra_info = "" + if "changes" in self.differences and self.differences["changes"]: extra_info += f"Files changed: {', '.join(self.differences['changes'])}" - if 'added' in self.differences and self.differences['added']: + if "added" in self.differences and self.differences["added"]: sep = "; " if extra_info else "" extra_info += f"{sep}Files added: {', '.join(self.differences['added'])}" - if 'removed' in self.differences and self.differences['removed']: + if "removed" in self.differences and self.differences["removed"]: sep = "; " if extra_info else "" extra_info += f"{sep}Files removed: {', '.join(self.differences['removed'])}" @@ -200,7 +200,7 @@ def verify(self, parsed_manifest_dct, diff=True): if recalculated[parsed_path] != parsed_checksum: mismatches.add(parsed_path) if mismatches: - differences = {'changes': list(mismatches)} + differences = {"changes": list(mismatches)} raise ChecksumMismatch("Checksum mismatch", differences) return True diff --git a/src/ansible_sign/cli.py b/src/ansible_sign/cli.py index f094abd..c2d58e2 100644 --- a/src/ansible_sign/cli.py +++ b/src/ansible_sign/cli.py @@ -237,13 +237,13 @@ def validate_checksum(self): self._error("Checksum validation failed.") if e.differences: differences = ( - ('added', 'added'), - ('removed', 'removed'), - ('changes', 'changed'), + ("added", "added"), + ("removed", "removed"), + ("changes", "changed"), ) for key, verb in differences: if key in e.differences and e.differences[key]: - self._note(f'Files {verb}:') + self._note(f"Files {verb}:") num_changes = len(e.differences[key]) if self.args.no_truncate: truncate_at = num_changes @@ -252,9 +252,9 @@ def validate_checksum(self): truncate_at = 6 truncated = num_changes > truncate_at for path in e.differences[key][0:(truncate_at)]: - self._note(f' - {path}') + self._note(f" - {path}") if truncated: - self._note(f' [{num_changes - truncate_at} lines omitted, use --no-truncate to see all...]') + self._note(f" [{num_changes - truncate_at} lines omitted, use --no-truncate to see all...]") return 2 except FileNotFoundError as e: if os.path.islink(e.filename):