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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix "pre-commit hook removes audited secrets" and verbosity overflow #65

Merged
merged 4 commits into from
Jul 31, 2018
Merged

Fix "pre-commit hook removes audited secrets" and verbosity overflow #65

merged 4 commits into from
Jul 31, 2018

Conversation

KevinHock
Copy link
Collaborator

Fixes #60 and #61

Aspiring to be more like @bcaller, I've made my git commit history squeaky clean 馃榿

@KevinHock
Add min(debug_level, 2)
0b27002 
So that 3 v's e.g. `-vvv` would not cause a KeyError, and default to logging.DEBUG
@KevinHock
Replace 5 spaces preceding comment with 2 spaces
a9973a7
@KevinHock
Change Eg. to e.g., delete commented out code
06db4b0
@KevinHock
Make is_secret an attribute of PotentialSecret
710d96e 
And make _load_baseline_from_dict pick it up, so that it does not get removed when e.g. lines move around.
Add is_secret attribute to _create_baseline() in pre_commit_hook_test.py so that TestPreCommitHook.test_writes_new_baseline_if_modified covers the change.
Fixes #60
@KevinHock KevinHock requested a review from domanchi July 31, 2018 20:32
domanchi
domanchi reviewed Jul 31, 2018
View reviewed changes
Copy link
Contributor

@domanchi domanchi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please create a test case that tests for this bug that you fixed (the one where different lines removes the labelling of data).

@KevinHock
Copy link
Collaborator Author

KevinHock commented Jul 31, 2018 •
edited
Loading

I'm sorry that it was a little tucked away in my commit message, but
TestPreCommitHook.test_writes_new_baseline_if_modified covers the change, since line 136 modifies the line to be 0, the pre-commit code is given a baseline with an "out-of-date" lineno and is_secret=True, and it outputs the correct lineno with is_secret=True.

domanchi
domanchi approved these changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@domanchi domanchi left a comment

Choose a reason for hiding this comment

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

To be thorough, we can probably write a test case without that label, to make sure behavior is expected.

But I think this should be sufficient. Thanks for pointing out how existing tests check for it!

@KevinHock KevinHock merged commit 7cbb139 into master Jul 31, 2018
@KevinHock
Copy link
Collaborator Author

Note to self: what seems like the cleanest way of doing the above is to add a secret to _create_baseline w/out the is_secret attribute, however I'm a little lazy to do that at the moment, as it may (or may not) require changing a lot of tests.

@KevinHock KevinHock deleted the 60_pre_commit_removes_is_secret branch July 31, 2018 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domanchi domanchi domanchi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@KevinHock @domanchi