-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
update golangci-lint to 1.60.1 #23614
Conversation
2b12bdf
to
b6cdb97
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
Well this was painful but it is done now and we can keep updating our linter |
@@ -105,9 +104,6 @@ func commit(cmd *cobra.Command, args []string) error { | |||
if !commitOptions.Quiet { | |||
commitOptions.Writer = os.Stderr | |||
} | |||
if len(commitOptions.Changes) > 0 { | |||
commitOptions.Changes = handlers.DecodeChanges(commitOptions.Changes) |
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 is this being removed? It looks like it ought to do something
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.
This thing is called several times in the same code path so I dropped one instance
|
||
declare -a to_lint | ||
to_lint=(ABI TUNNEL REMOTE) |
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.
I assume this isn't needed because remote == tunnel?
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.
yep, on linux we just lint the same things twice because of that and the linter isn't exactly fast so I dropped it.
@@ -137,13 +139,6 @@ var _ = Describe("Podman privileged container tests", func() { | |||
}) | |||
|
|||
It("run no-new-privileges test", func() { | |||
// Check if our kernel is new enough | |||
k, err := IsKernelNewerThan("4.14") |
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.
Dropping because kernel 4.14 and lower is ancient?
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.
yes
LGTM |
Fixes new spotted issues around printf() formats and using os.Setenv() in tests. Signed-off-by: Paul Holzinger <[email protected]>
The new golangci-lint version 1.60.1 has problems with typecheck when linting remote files. We have certain pakcages that should never be inlcuded in remote but the typecheck tries to compile all of them but this never works and it seems to ignore the exclude files we gave it. To fix this the proper way is to mark all packages we only use locally with !remote tags. This is a bit ugly but more correct. I also moved the DecodeChanges() code around as it is called from the client so the handles package which should only be remote doesn't really fit anyway. Signed-off-by: Paul Holzinger <[email protected]>
Now that we have propert !remote tags set everywhere we can just rely on that and do not need to skip any dirs. Also on linux do not lint three times, one remote run is enough. We still have to skip the test dir for windows/macos though or we need to add linux build tags there everywhere as well. This seems simpler. Signed-off-by: Paul Holzinger <[email protected]>
We need something newer than 4.14 anyway now for most Podman functions. This is breaking liniting on windows as the function doesn't work there. Signed-off-by: Paul Holzinger <[email protected]>
It qemu cannot be compiled anyway so make sure we do not try to compile parts where the typechecker complains about on windows. Also all the e2e test files are only used on linux as well. pkg/machine/wsl also reports some error but to many for me to fix them now. One minor problem was fixed in pkg/machine/machine_windows.go. Signed-off-by: Paul Holzinger <[email protected]>
Fix one minor issue with vfkit error handling. First checking if err != nil OR errors.Is() is pointless as the err != is already true. Second nilerr complains because we return nil when we hit an error branch, in this case this is correct because an error means VM is stopped. Signed-off-by: Paul Holzinger <[email protected]>
Rebased, I see the macos linter failing on all mains PRs currently so I wonder if this here now also fails or if this magically fixes it. |
Ok this seems to indeed work while other PRs are failing: https://cirrus-ci.com/task/5484764008808448 @containers/podman-maintainers PTAL let's get this in to unblock CI. |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes new spotted issues around printf() formats and using os.Setenv() in tests.
Does this PR introduce a user-facing change?