-
Notifications
You must be signed in to change notification settings - Fork 74
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
Handle the case where exclude-signatures is a list of strings #372
Handle the case where exclude-signatures is a list of strings #372
Conversation
Add error handling when tartufo config is not found Only open the file once when writing the config Add a couple more tests
9265877
to
86d755f
Compare
Turns out the docker build was failing because poetry decided to delete half the hashes from the lockfile while updating tomlkit. |
There is a new issue that was experienced relating to trying to pass bytes to click.echo. Here is the relevant part of the traceback:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Any particular motivation for modifying the configuration file in place instead of just replacing it with a new version like you were doing before? I'm just interested.
Are you referring to the If so, I did it to prevent opening the file twice, which felt inefficient. |
Ok this PR now addresses both the issue where exclude-signatures is a list of strings, and also where click.echo fails as described in #373 Am I missing anything? I feel like it might be best to try and prevent a traceback from hitting the end user, but it may obscure potentially useful information in the traceback. I'll defer to your feedback on whether that should be added. |
I have also noticed an issue where if duplicates are removed from the config, the final closing bracket is placed on the preceding line which can cause issues if that preceding line is only a comment after removing duplicates. It's a pretty niche bug, but I know of at least 1 repository personally that would be affected. A tracking issue has been created on the tomlkit repo here. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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?
Issue Linking
Closes #371
Closes #373
What's new?
I added an upwrap function to handle the case where
exclude-signatures
is a list of strings.I also added additional error handling for the case when there is no config file/no
tool.tartufo
section.tomlkit
0.7.2, the current version in use bytartufo
, had a bug where values would not update properly.I created a minimal reproducible example of that:
Result:
Expected result:
Based on this I updated
tomlkit
to the latest version 0.11.1 and that issue is now resolved.I added a couple more tests to handle the new cases.