Skip to content
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

✨ Upgrade golangci from 1.54 to 1.57 #3846

Merged

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Apr 5, 2024

  • Upgrade golangci from 1.54 to 1.57
  • Fix lint issues after upgrade by:

a) Ensure that in all places we are ignoring the exception equally

         // nolint:revive
	. "github.com/onsi/ginkgo/v2"
	// nolint:revive
	. "github.com/onsi/gomega"

b) Ensure that we use _ for args not explicit called in the funcs. Example

PreRunE: func(_ *cobra.Command, _ []string) error {

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 5, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2024
@camilamacedo86 camilamacedo86 changed the title ❇️ Upgrade golangci from 1.54 to 1.57 ✨ Upgrade golangci from 1.54 to 1.57 Apr 5, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 5, 2024
Copy link
Contributor

@Kavinjsir Kavinjsir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Kavinjsir

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -20,7 +20,9 @@ import (
"fmt"
"testing"

// nolint:revive
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having all of these nolint:revive lines, I think we are safe to assume dot imports will be used for intentional reasons. I think it would be cleaner to just remove the dot-imports rule from line 28 in .golangci.yml.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. One way maybe:

[rule.dot-imports]
  arguments = [{ allowedPackages = ["github.com/onsi/ginkgo/v2","github.com/onsi/gomega"] }]

Copy link
Member Author

@camilamacedo86 camilamacedo86 Apr 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use dot imports
Just the case of ginkgo and gomega which are an exception.
So, we still want the check

If a config like

[rule.dot-imports]
  arguments = [{ allowedPackages = ["github.com/onsi/ginkgo/v2","github.com/onsi/gomega"] }]

Works then, It is totally fine.
We can use it

Copy link
Member Author

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stmcginnis

Rather than having all of these nolint:revive lines, I think we are safe to assume dot imports will be used for intentional reasons. I think it would be cleaner to just remove the dot-imports rule from line 28 in .golangci.yml.

We should still not using alias with .. However, yes we have one exception which we have been using // nolint to ignore those which are only. Then, if we do not check it probably we will end up using it without a good reason too.

	// nolint:revive
	. "github.com/onsi/ginkgo/v2"
	// nolint:revive
	. "github.com/onsi/gomega"

The lint fix of this PR just ensure that we have those applied equally in all places
Note that seems that some files we were not checking and because of this they were without the //nolint check.

However, to ignore this case I totally agree that would be nice if we could find a way to configure it as @Kavinjsir suggested. He also open an issue: #3847 (which I understand would have this goal)

Therefore, IMO it seems fine we try to replace those in a follow up with the suggestion if we be able to check that can works. +1 💯

The other rule that we need to fix here is for we use _ instead of args name when those are not used which is also a valid rule see: (i.e. https://github.com/kubernetes-sigs/kubebuilder/pull/3846/files#diff-e5f2449f45b05bf525a27e22a412864f1d479ac4548d5ffe3c90da55fdfee3f6R34-R39)

In this way, since:

  • @Kavinjsir /lgtm this one
  • It only do the bump and ensure that we are ignoring equally in all place the ginkgo and gomega exception regards the dots imports
  • Do the fix of the _ for args not used in the missing places
  • And we have a issue to see if we can improve in a follow up
  • By last it is too silly small/nit one

I think it is safe we get this one merged and I am going forward with this one.
I hope you both has no objections.

But please feel free to contribute within for we improve this in a follow up as you see that it would fit.

Thank you a lot for your collaboration @Kavinjsir and @stmcginnis 🎉

@camilamacedo86 camilamacedo86 merged commit a7f6e03 into kubernetes-sigs:master Apr 6, 2024
16 of 18 checks passed
@camilamacedo86 camilamacedo86 deleted the upgrade-golangci-lint branch April 6, 2024 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants