Fix various golangci linter issues #977
Conversation
|
@kevpar PTAL when you get the chance |
|
Any chance for a brief overview of what these are? e.g. are 90% of these "remove unused code"? |
|
The fact that our old linter complained about this PR made me giggle |
|
And that's why I didn't want to change linter in #970 but instead make it a separate step :)
|
katiewasnothere
force-pushed
the
linter_fixes
branch
2 times, most recently
from
7dd1134
to
b91db64
Compare
| @@ -460,7 +455,7 @@ func (s *service) DiagPid(ctx context.Context, req *shimdiag.PidRequest) (*shimd | |||
| if s == nil { | |||
| return nil, nil | |||
| } | |||
| ctx, span := trace.StartSpan(ctx, "DiagPid") | |||
| ctx, span := trace.StartSpan(ctx, "DiagPid") //nolint:ineffassign,staticcheck | |||
There was a problem hiding this comment.
curious; what was the linter complaining about here?
There was a problem hiding this comment.
It complained because the call to StartSpan returns a new ctx, which isn't used again in the function.
There was a problem hiding this comment.
Ah!
would something like this work for those as well?
| ctx, span := trace.StartSpan(ctx, "DiagPid") //nolint:ineffassign,staticcheck | |
| _, span := trace.StartSpan(ctx, "DiagPid") |
There was a problem hiding this comment.
Can we do _, span := ... and remove the nolint?
There was a problem hiding this comment.
Oh, @thaJeztah commented right before me :)
There was a problem hiding this comment.
we can't add new functions on the context struct type since it's defined in a different imported package that we don't own.
There was a problem hiding this comment.
If we keep the nolint directives in, and then later change the code such that they are no longer needed (e.g. start using ctx), will the linter tell us "hey, this directive isn't doing anything"?
There was a problem hiding this comment.
Not from what I've seen
There was a problem hiding this comment.
I don't have a strong preference here. I would probably go with _, span := ..., and rely on re-creating the ctx variable later if we end up needing it, but leaving the nolint directives in seems okay to me as well.
There was a problem hiding this comment.
I think I'm going to keep the nolint directives in for now and if it becomes annoying we can take them out.
| } | ||
| <-containerExitCh | ||
| err = containerExitErr |
There was a problem hiding this comment.
do you know why this was here? (or was this just copy/pasta from another block?)
There was a problem hiding this comment.
Looks like that was something @jstarks wrote two years ago, so maybe he can comment haha
@katiewasnothere just wanted to make sure you saw this. |
@kevpar I updated the PR description with some info. Is there something else that I should add that would make it clearer? |
| Spec: s, | ||
| HostingSystem: parent, | ||
| NetworkNamespace: netNS, | ||
| ScaleCPULimitsToSandbox: shimOpts.ScaleCpuLimitsToSandbox, |
There was a problem hiding this comment.
Why did we remove this assignment?
There was a problem hiding this comment.
The linter was complaining because below we check if shimOpts is nil, so it didn't like this direct access to it. But also, we set it additionally right below this struct creation, so we only need one :D
There was a problem hiding this comment.
looks like it was noticed just before / after merge 🙈 #915 (comment)
There was a problem hiding this comment.
I thought I force pushed to fix before checking in 😭 😭 😭. Well I've gotta give it to myself, I both fixed it and then broke it again. This deserves a medal
There was a problem hiding this comment.
💩 happens; you did that to test if the linters work, correct? 😂
There was a problem hiding this comment.
100%, don't let anyone tell you otherwise 😉
| @@ -572,10 +566,12 @@ func startContainerShim(c *container, pidFile, logFile string) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| defer shim.Release() | |||
| defer func() { | |||
| _ = shim.Release() | |||
There was a problem hiding this comment.
This is obnoxious. Maybe we need to think about turning off the error check lint in the future.
There was a problem hiding this comment.
I think it's valuable to have on. I think we should just find a better way to handle cases where we don't really care about the error. Maybe log a warning? Probably depends on the case though.
Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
katiewasnothere
force-pushed
the
linter_fixes
branch
from
b91db64
to
58f7ef4
Compare
|
Thanks @katiewasnothere !! |
This PR updates our internal ADO repo to the github commit [d9474d2](microsoft@d9474d2). Related work items: microsoft#964, microsoft#965, microsoft#966, microsoft#967, microsoft#968, microsoft#969, microsoft#970, microsoft#971, microsoft#972, microsoft#974, microsoft#975, microsoft#976, microsoft#977, microsoft#978, microsoft#979, microsoft#980, microsoft#981, microsoft#982, microsoft#983, microsoft#984, microsoft#987, microsoft#990, microsoft#991, microsoft#992, microsoft#993, microsoft#995, microsoft#996, microsoft#997, microsoft#1000
This PR fixes the various linter issues discovered in #975.
Many of the fixes involved adding a comment to exclude code from specific linters, see No Lint.
For warnings relating to deadcode, the related code was removed. Warnings that related to ineffectual value assignment for context variables, I excluded the line from the
ineffassignandstaticchecklinters. This was to help ensure future log statements that may be made in these functions use the correctctxvalue. For warnings relating to not checking the error code of a function call, I excluded theerrchecklinter for that line, since it was likely intentional to ignore the error. If this is not the case, we can fix the relevant areas.To see a list of the default checks made with golangci, see here.
Signed-off-by: Kathryn Baldauf kabaldau@microsoft.com