Feature/secureli-435: Implement custom PII scan #496
Conversation
…cureli-435-pii-from-scratch
…cureli-435-pii-from-scratch
| @@ -27,15 +27,15 @@ def __init__( | |||
| echo: EchoAbstraction, | |||
| language_analyzer: language_analyzer.LanguageAnalyzerService, | |||
| language_support: language_support.LanguageSupportService, | |||
| scanner: ScannerService, | |||
| hooks_scanner: HooksScannerService, | |||
There was a problem hiding this comment.
Just to leave context from a prior conversation during standup, I decided to rename this service so that it's more explicitly clear what it's used for, in contrast to any custom scans that are implemented (e.g., the PII scan). This scanner solely scans using external pre-commit hooks. Calling it a generic ScannerService didn't quite capture that nuance IMO.
I'd also like to rename this file in a follow-up PR to match this change. I didn't do this as part of this PR because it wouldn't be as clear which in-file changes I made.
| @@ -18,15 +22,7 @@ class OutputParseErrors(str, Enum): | |||
| REPO_NOT_FOUND = "repo-not-found" | |||
|
|
|||
|
|
|||
| class ScanOuput(pydantic.BaseModel): | |||
There was a problem hiding this comment.
I moved this to the models folder along with ScanFailure, ScanMode, and ScanResult. It felt more appropriate there.
| :raises ValueError: The specified path does not exist or is not a git repo | ||
| :return: The list of staged file names | ||
| """ | ||
| self._confirm_is_git_repo(folder_path) |
There was a problem hiding this comment.
I wrote similar logic for #436. I added an abstraction for GitPython to get committed files. I'm wondering if we can use one or the other implementations or combine them. Adding it as an abstraction allowed for easier unit testing.
There was a problem hiding this comment.
Good call out. I am planning on having a follow-up PR to optimize some things. Would you be OK with me addressing this in that follow-up PR? That way we can get the meat of the functionality in today while we're all still here, and then I can have a smaller PR that won't be so difficult to get reviews on from new Secureli folks.
There was a problem hiding this comment.
Captured here: #497
| """ | ||
| git_path = folder_path / ".git" | ||
|
|
||
| if not git_path.exists() or not git_path.is_dir(): |
There was a problem hiding this comment.
Same here. Can probably add a similar function for this in the GitRepo abstraction.
kathleen-hogan-slalom
changed the title
kathleen-hogan-slalom
merged commit 9c43ac7
into
refactor/secureli-000-modular-refactor
secureli-435
This PR implements a new, custom PII scan. This scan occurs separately from our existing scans which utilize pre-commit hooks external to the secureli codebase.
Changes
Testing
Clean Code Checklist