Update golangci-lint (1.46.2->1.50.0) #1416

oscr · 2022-09-28T12:19:37Z

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

Updates the golangci-lint version from 1.46.2 -> 1.49.0 and fixes all the findings. This is needed due to linters being disabled currently. Also adds make lint target which just makes it easier for developers.

Here is the current main branch linter output:

hack/verify-golint.sh
level=warning msg="[linters context] bodyclose is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters context] contextcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters context] nilerr is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters context] noctx is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters context] rowserrcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters context] sqlclosecheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters context] structcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."
level=warning msg="[linters context] unparam is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649."

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2022-09-28T12:19:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: oscr
Once this PR has been reviewed and has the lgtm label, please assign dcbw for approval by writing /assign @dcbw in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

akankshakumari393 · 2022-09-28T17:27:18Z

Could we rebase the PR? I see that there are other changes as well which I think are not related to this PR

robscott · 2022-09-30T03:07:45Z

@oscr Thanks for filing this PR! I think the presubmit is failing because you need to run make generate to update the generated CRD yamls.

 M config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml
 M config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml
 M config/crd/experimental/gateway.networking.k8s.io_tlsroutes.yaml
 M config/crd/standard/gateway.networking.k8s.io_httproutes.yaml

oscr · 2022-09-30T07:09:05Z

@robscott Thank you! That seems to have solved it.

robscott

Thanks @oscr!

robscott · 2022-09-30T23:44:12Z

apis/v1beta1/httproute_types.go

-//   - name: "version"
-//     value "v1"
+//
+//	path:


Is this additional indentation required to pass the updated linter?

I invoked golangci-lint with fix in in order to resolve the findings. I didn't add a Makefile target for it but it's quite useful. For example cluster-api has it: https://github.com/kubernetes-sigs/cluster-api/blob/1917d52af16a7df70c5e93c9bb0820d5c04f972f/Makefile#L478

I'm not sure how to fix this, but I think this is an example of the linter doing something we want to avoid. It's taking what would otherwise be valid inline YAML example and messing up the indentation to match golang indentation expectations. As far as the make rule that cluster-api has, I think that would actually address #1398. I'd be supportive of that kind of rule if we could figure out how to make it not break any inline examples we have.

Although I know inline yaml is generally not a good idea for go types, it does make our reference documentation much easier to follow: https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io%2fv1alpha2.GRPCRouteRule.

@robscott I can add the lint-fix target. No problem at all.

Given how gofmt insists on the current format is incorrect and want's the code reformatted, maybe we should just disable that specific linter and undo the changes? Whats your opinion on that?

I'm not sure, disabling all of gofmt for this relatively niche use case does seem unfortunate. We could possibly make an exception like this for gofmt that only applied to our API type definitions: https://github.com/kubernetes-sigs/gateway-api/blob/main/.golangci.yml#L42. Interested in what others think here as well.

/cc @shaneutt @youngnick

I think we're stuck between two less than ideal options:

1. Don't run gofmt and goimports on our types files
This can be accomplished pretty easily with the following addition to exclude-rules:

- path: _types\.go linters: - gofmt - goimports

It allows us to keep inline YAML examples in our godocs and therefore generated reference docs.

2. Remove YAML examples from inline docs
Although this may make our generated reference docs slightly harder to follow, it would ensure that our types files were well formatted.

3. Don't upgrade golangci-lint yet
This appears to pass with our current generation of golangci-lint which is only slightly older than this one.

At this point, I don't love any of the options, but am slightly leaning towards 3 here unless we can find a better alternative than 1 or 2.

I think one thing to remember here is that many editors will format on save for Go. I can't remember explicitly enabling it (I might be wrong here) in IDEA but I nevertheless I have it. If the linter expects a certain indentation, but your tooling automatically reformats the code upon save, it's going to be a pain for contributors. I think there is a risk of many prs failing the lint step in the future.

Ultimately it's your call. I can fix 1, 2 or close it for 3.

it's possible to embed yaml, just the entire block needs to be indented to conform to gofmt's notion of a code block

From https://go.dev/doc/comment
indenting all the yaml by a tab should make it a preformatted code block

Awesome find @seankhliao! Thank you so for your help here. I've updated the indents with tabs and at least locally it doesn't reformat or linting fail.

apis/v1beta1/validation/gateway.go

cmd/admission/main.go

robscott

Thanks for the work on this @oscr!

apis/v1alpha2/validation/gateway.go

apis/v1alpha2/grpcroute_types.go

robscott · 2022-10-03T19:13:35Z

apis/v1beta1/httproute_types.go

-//   - name: "version"
-//     value "v1"
+//
+//	path:


I'm not sure how to fix this, but I think this is an example of the linter doing something we want to avoid. It's taking what would otherwise be valid inline YAML example and messing up the indentation to match golang indentation expectations. As far as the make rule that cluster-api has, I think that would actually address #1398. I'd be supportive of that kind of rule if we could figure out how to make it not break any inline examples we have.

Although I know inline yaml is generally not a good idea for go types, it does make our reference documentation much easier to follow: https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io%2fv1alpha2.GRPCRouteRule.

cmd/admission/main.go

oscr · 2022-10-04T20:57:08Z

Newer version released. Updating pr with latest and greatest.

robscott · 2022-10-07T18:56:20Z

apis/v1alpha2/grpcroute_types.go

+//	```
+//	matches:
+//	- method:
+//	   type: Exact
+//	   service: "foo"
+//	  headers:
+//	  - name: "version"
+//	    value "v1"
+//	```


Unfortunately this didn't translate well to the generated reference docs 😢. https://deploy-preview-1416--kubernetes-sigs-gateway-api.netlify.app/references/spec/#gateway.networking.k8s.io%2fv1alpha2.GRPCRouteMatch. Not sure what the solution is, there likely is some combination of indentation that works well for most. I have noticed that inline yaml examples only seem to fail gofmt on top level structs/comments were the actual // is not indented.

Unfortunate, but a good find with the top level comments. Is that a viable way forward? Otherwise I suggest closing the pr because it doesn't seem to be possible to move forward as is.

k8s-ci-robot · 2022-11-05T12:34:32Z

@oscr: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 28, 2022

k8s-ci-robot requested review from bowei and mikemorris September 28, 2022 12:19

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 28, 2022

robscott mentioned this pull request Sep 30, 2022

Fix verify-codegen failure #1417

Closed

oscr force-pushed the update-golangci-lint branch from c3acfaa to 50de140 Compare September 30, 2022 06:59

robscott reviewed Sep 30, 2022

View reviewed changes

oscr force-pushed the update-golangci-lint branch from 50de140 to da48aee Compare October 1, 2022 08:14

robscott reviewed Oct 3, 2022

View reviewed changes

k8s-ci-robot requested review from shaneutt and youngnick October 4, 2022 01:29

oscr force-pushed the update-golangci-lint branch from da48aee to d4b188b Compare October 4, 2022 06:55

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2022

oscr force-pushed the update-golangci-lint branch from d4b188b to 2380e6c Compare October 4, 2022 20:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2022

oscr force-pushed the update-golangci-lint branch from 2380e6c to ce7a939 Compare October 4, 2022 20:56

oscr changed the title ~~Update golangci-lint (1.46.2->1.49.0)~~ Update golangci-lint (1.46.2->1.50.0) Oct 4, 2022

oscr force-pushed the update-golangci-lint branch 3 times, most recently from e698894 to 039e8f4 Compare October 7, 2022 10:33

Update golangci-lint (1.46.2->1.50.0)

71a6835

oscr force-pushed the update-golangci-lint branch from 039e8f4 to 71a6835 Compare October 7, 2022 10:52

robscott reviewed Oct 7, 2022

View reviewed changes

robscott mentioned this pull request Oct 15, 2022

https://gateway-api.sigs.k8s.io/references/spec renders code snippets improperly #1458

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2022

oscr closed this Dec 21, 2022

Update golangci-lint (1.46.2->1.50.0) #1416

Update golangci-lint (1.46.2->1.50.0) #1416

Uh oh!

Conversation

oscr commented Sep 28, 2022

Uh oh!

k8s-ci-robot commented Sep 28, 2022

Uh oh!

akankshakumari393 commented Sep 28, 2022

Uh oh!

robscott commented Sep 30, 2022

Uh oh!

oscr commented Sep 30, 2022

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oscr commented Oct 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants