Fix temp dir switching issue and naming in scan repo flow #1016
Conversation
eranturgeman
requested a deployment
to
frogbot
GitHub Actions
Waiting
eranturgeman
added
bug
github-actions
bot
removed
the
safe to test
| } | ||
|
|
||
| func (gm *GitManager) SetLocalRepository() error { | ||
| // Sets the current working dir as the local git repository |
There was a problem hiding this comment.
remove comment
func name is self explanatory
| sr.baseWd = repoDir | ||
| defer func() { | ||
| // On dry run don't delete the folder as we want to validate results | ||
| if sr.dryRun { |
There was a problem hiding this comment.
this is not your code but if you like to(if not stop reading and resolve):
this whole dry run concept is wrong
dry run is used purely for tests
and this is test logic that is leaking into production code
if you want to fix it you can extract a GitClient interface and use dependency injection.
| } | ||
|
|
||
| func (sr *ScanRepositoryCmd) checkoutToBranch() (tempWd string, restoreDir func() error, err error) { | ||
| // This func switches to a newly created or provided temp dir for the the current execution in order to keep the user's working dir clean. |
There was a problem hiding this comment.
typo in comment but remove it anyway
generally we don't want comments
comments are for either one of these 3 things:
-
A temporary situation that will change in the future - Usually we add a TODO for a patch that cant be done.
-
A misguided approach due to a third party source - for example the cli returns a bad object and we need to parse and change it to fit what we do.
-
An unclear business logic - when the logic the code performs contradicts straightforward logic,
We add a comment that explains the why the code is doing what it is doing and not the what.
| if err != nil { | ||
| return | ||
| } | ||
| // Set the current copied local dir as the local git repository we are working with |
There was a problem hiding this comment.
not your code but please remove comments
2 participants
This PR fixes 2 things:
Explanation: Since we can iterate several branches in scan-repo, for each branch we work in a temp dir and return to the original dir at the end. When returning to the original dir we set the cur wd to the original dir and delete the temp one. I noticed that we missed setting the .git repo to the original on. This may cause if some failure happens to leave the gitManager pointing to a .git repository that no longer exists