Fix failing scan-remote-repo command
#241
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The recent live-output & pygit2 changes broke existing remote repo scans because `scanner.scan` was being called from the subcommand and the cloned directory was being deleted before the scan could actually happen. The original process_issues callback has now been moved into util and is called from the sub-commands to fix this issue. The main callback only handles exit status now.
sushantmimani
requested review from
rscottbailey,
mxhenry-godaddy,
rbailey-godaddy and
tarkatronic
and removed request for
a team
tarkatronic
suggested changes
Oct 27, 2021
tartufo/cli.py
Outdated
| @@ -282,26 +280,9 @@ def main(ctx: click.Context, **kwargs: config.OptionTypes) -> None: | |||
| @click.pass_context | |||
| def process_issues( | |||
There was a problem hiding this comment.
Since this is no longer doing any actual processing of issues, let's rename it to something more appropriate.
There was a problem hiding this comment.
I keep coming up with stupid-sounding redundant names. "return_exit_status" or "compute_return_code" or something oxymoronic that covers the 2 lines of code that's all that's left...
rbailey-godaddy
requested changes
Oct 27, 2021
tartufo/cli.py
Outdated
| @@ -282,26 +280,9 @@ def main(ctx: click.Context, **kwargs: config.OptionTypes) -> None: | |||
| @click.pass_context | |||
| def process_issues( | |||
There was a problem hiding this comment.
I keep coming up with stupid-sounding redundant names. "return_exit_status" or "compute_return_code" or something oxymoronic that covers the 2 lines of code that's all that's left...
This addresses godaddy#243. We had two problems: 1. Diff was generated in backwards order; it is clear from: https://pygit2.readthedocs.io/en/latest/recipes/git-show.html that if "show diff" is implemented as: ``` >>> diff = repo.diff(commit.parents[0], commit) ``` then the previous commit must be the first argument (and in fact you can see just above where prev_commit is precisely commit.parents[0]). 2. Commit metadata was retrieved for the previous commit I verified running tartufo against itself that the reported commit hashes and diffs now match what "git show" actually displays. Unit test changes were restricted to swapping the order of the commits in expected call signatures for repo.diff(). I resisted the temptation to collapse the special-case logic for the lead commit into the earlier block that looks for it, in order to minimize the size of this commit.
tarkatronic
approved these changes
Oct 27, 2021
rbailey-godaddy
approved these changes
Oct 27, 2021
This was referenced Oct 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To help us get this pull request reviewed and merged quickly, please be sure to include the following items:
PR Type
What kind of change does this PR introduce?
Backward Compatibility
Is this change backward compatible with the most recently released version? Does it introduce changes which might change the user experience in any way? Does it alter the API in any way?
What's new?
The recent live-output & pygit2 changes broke existing remote repo scans because
scanner.scanwas beingcalled from the subcommand and the cloned directory was being deleted before the scan could actually happen.
The original process_issues callback has now been moved into util and is called from the sub-commands to
fix this issue. The main callback only handles exit status now.