-
Notifications
You must be signed in to change notification settings - Fork 582
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 linter warning message #1057
Fix linter warning message #1057
Conversation
915f004
to
f3af119
Compare
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.
Thank you!
if err := os.Setenv("CNI_ARGS", cniArgs); err != nil { | ||
return err | ||
} | ||
return nil | ||
return os.Setenv("CNI_ARGS", cniArgs) |
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.
Is CNI_ARGS going to be the new default instead of returning nil? And what is the reason for this change?
if err := os.Unsetenv("CNI_ARGS"); err != nil { | ||
return err | ||
} | ||
return nil | ||
return os.Unsetenv("CNI_ARGS") |
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.
ditto
func (fe *fakeExec) FindInPath(plugin string, paths []string) (string, error) { | ||
func (fe *fakeExec) FindInPath(_ string, _ []string) (string, 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.
Why have argument names been removed for many functions? What is the need for this?
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.
Make peace with the linter ? It is complaining these attributes are not being used.
Thus we tell the compiler we don't care enough to name them.
... this needs to happen because this is a method of an interface.
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.
Thanks Miguel, also talked to Tomo about the changes. I think we're good to merge here
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.
Changes looks good to me, and thanks Tomo/Miguel for explaining the need for these changes.
Fix linter/revive warning message.