Feature/support multi os baseline files #586
Conversation
…tect-secrets into feature/support-multi-os-path
|
@lorenzodb1 @jpdakran Keen to understand any feedback you might have. |
|
@lorenzodb1 @jpdakran bump |
|
Hi @eddiez9, we haven't missed your PR and I'm happy to see that all tests pass this time. Thank you for your work, it's definitely a great improvement. This PR is on our TODO list and we will review this PR as soon as we can. To give you a high-level timeline, our goal of including these changes in our next release (possibly happening by the end of September). We appreciate your patience in the meantime. |
|
Thanks for the update @lorenzodb1! |
|
@eddiez9 How will this affect writes to a baseline file? Will it update the baseline in a consistent manner or will it update in an OS specific manner? If the later is the case will we have mixed formatting? |
|
@lorenzodb1 @jpdakran bumping this PR as I would also like this feature to be added (we have devs using Windows and others Unix systems, and this causes conflicts when running a CI on Unix). |
|
@eddiez9 could you please merge the latest |
|
@lorenzodb1 Why has this not been merged yet? What is missing for this to be merged? All checks are passing. Because the repo states that it is for enterprise, we have started implementing it, however, we cannot use this library right now as we can't check in the |
|
@jsdevtom in order to get tests to run I needed the author of this PR to merge the master branch into their branch, and that happened merely two weeks ago. Now that all checks are passing, I'll find some time to review the changes. Apologies for any inconvenience 😄 |
There was a problem hiding this comment.
@eddiez9 thank you for your contribution! I'll go ahead and merge this so it can be included in the next release, coming up at the end of the month 😄
Attempt 2 at #573
Only change from here and the last attempt is that I've been more careful with just replacing what I need to in terms of updating the test assertions.
And instead of using os.path.join in the test assertions, I've used the same approach as in #528 (pathlib path)
Problem
Currently, detect-secrets doesn't support multi OS environments very well.
If a Windows developer generates a baseline file, and a Linux developer uses the same baseline file, they will always be alerted on new secrets and vice versa.
This is because baseline files have OS specific paths
Windows:
"filename": "src\blah\appsettings
Linux:
"filename": "src/blah/appsettings
When performing new_secrets = secrets - args.baseline you will always get new secrets because the filenames dont match.
Solution
The best solution would probably be to generate baseline files/read files in a consistent manner to always use Unix paths with something like os.path.normpath Doing this however will mean people will need to update their baseline files.
The solution implemented in the PR instead will using os.sep to determine if we are on UNIX or Windows, then replace slashes in filenames for the loaded baseline file (without changing the baseline file). The comparison will then work regardless of if you are on Linux comparing against a windows generated baseline file and vice versa.