fix(git): raise error if branch not found #220
Conversation
dclayton-godaddy
requested review from
tarkatronic,
sushantmimani and
mxhenry-godaddy
There was a problem hiding this comment.
I don't like the proposed name of the exception, but if you can sell @jwilhelm-godaddy on it I'll change my vote. ;-)
tartufo/scanner.py
Outdated
| raise BranchNotException( | ||
| f"Branch {self.git_options.branch} was not found." | ||
| ) |
There was a problem hiding this comment.
Well, BranchNotException irks me because it feels like we're missing a word... the branch isn't what? Found? Valid?
Worse, I went and looked to see what tartufo was using for a convention, and they're pretty generic:
class TartufoException(Exception):
class ConfigException(TartufoException):
class ScanException(TartufoException):
class GitException(TartufoException):
class GitLocalException(GitException):
class GitRemoteException(GitException):
This makes me think we probably should be raising GitLocalException for a local repo or GitRemoteException for a remote repo, but I get that we probably don't want to have to screw around trying to pick the right one.
My suggestion here therefore would be to use GitBranchException (which should be a specialization of GitException).
The other option that came to mind would be some sort of UserFootInMouthException for when we are whining about GIGO directives, but there doesn't seem any call for it at this point so I'd stick with something simple.
There was a problem hiding this comment.
I did not want to mark it as a GitException as it wasn't an issue interacting with Git (Raised if there is a problem interacting with git). This is a logic issue unrelated to interacting with git. For example, if someone typed in mybrach and missed the n, this is a user issue where it would say mybrach not found.
There was a problem hiding this comment.
Regardless of all else, I agree that BranchNotException doesn't particularly make sense. As @rbailey-godaddy said, it sounds like it's missing a word. I would recommend renaming it to BranchNotFoundException would be very appropriate.
There was a problem hiding this comment.
lol. You're right. Not sure what I was thinking. I changed to BranchNotFoundException.
tartufo/types.py
Outdated
| class BranchNotException(TartufoException): | ||
| """Raised if a branch was not found""" |
There was a problem hiding this comment.
Per above, I'd make this GitBranchException(GitException) and you might need to move it down if this is ahead of the GitException declaration.
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
What's new?
Currently, if someone runs the tartufo CLI using the
--branchargument and the branch does not exist, the CLI will return no secrets found. This is misleading and should fail the call. This change will fail the call if--branchis specified and the branch was not found.