Remove temp sshKeyFile after use#5769
Conversation
hiep-tk
requested review from
Warashi,
ffjlabo,
khanhtc1202 and
t-kikuc
as code owners
| if cfg.Git.ShouldConfigureSSHConfig() { | ||
| if err := git.AddSSHConfig(cfg.Git); err != nil { | ||
| tempFile, err := git.AddSSHConfig(cfg.Git) | ||
| if err != nil { |
There was a problem hiding this comment.
This still leaks in some cases: if the error is not nil, the AddSSHConfig function returns tempFile, but this block will immediately close this run function, thus defer at L184 is not triggered 👀
There was a problem hiding this comment.
I agree with @khanhtc1202 , the code might be like this:
tempFile, err := git.AddSSHConfig(cfg.Git)
if len(tempFile) > 0 { // HERE
defer os.Remove(tempFile)
}
if err != nil {
...There was a problem hiding this comment.
Updated as suggested. @khanhtc1202 @t-kikuc
| // TODO: Remove this key file when Piped terminating. | ||
| if _, err := sshKeyFile.Write(sshKey); err != nil { | ||
| return err | ||
| return sshKeyFile.Name(), err |
There was a problem hiding this comment.
How about closing this file in this function if err is not nil, then we can have convention like: return "", err if err occurred, while the file is only returned in case there is no issue, and the caller only need to defer close file in case of no error (after err check)? @hiep-tk
There was a problem hiding this comment.
Updated for consistent return format @khanhtc1202
Signed-off-by: hiep-tk <hiep.trinhkhanh0466@gmail.com>
khanhtc1202
changed the title
khanhtc1202
changed the title
* Do treeless clone for git clone to improve performance (#5722) Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * remove temp sshKeyFile after use(#2215) (#5769) Signed-off-by: hiep-tk <hiep.trinhkhanh0466@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> --------- Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> Signed-off-by: hiep-tk <hiep.trinhkhanh0466@gmail.com> Co-authored-by: Shinnosuke Sawada-Dazai <shin@warashi.dev> Co-authored-by: Hiep Trinh <hiep.trinhkhanh0466@gmail.com>
4 participants
What this PR does: remove the temporary ssh key file after using it for the config
Why we need it: clean up the ssh key folder
Which issue(s) this PR fixes:
Fixes #2215
Does this PR introduce a user-facing change?: