-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: golangci-lint error #170
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, LGTM. Just a few comments.
Just one feedback about when dealing an error but it's not critical and unhandled or it can be recovered eventually, suggest logging with warn level instead of error.
fsops/fsops.go
Outdated
@@ -127,7 +127,9 @@ func (f *FileSystemOperator) List(path string) ([]string, error) { | |||
func (f *FileSystemOperator) Upload(src, dst string) error { | |||
tmpDst := dst + ".tmp" + "." + strconv.FormatInt(time.Now().UTC().UnixNano(), 10) | |||
if f.FileExists(tmpDst) { | |||
f.Remove(tmpDst) | |||
if err := f.Remove(tmpDst); err != nil { | |||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we handle this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we return error and for the caller to handle it. Or do you mean log it before return the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the experience, usually removing files would be best efforts, so the error could not be handled. Or, we can just have a warn log, good enough? (of course, those temp files could be leftover.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, update to log a warning message. Thanks.
Thank you. I change the log level to warning. |
Signed-off-by: PoAn Yang <[email protected]>
Which issue(s) this PR fixes:
longhorn/longhorn#7448
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context