🏃 Update linter and lint fixes #773

mcristina422 · 2020-01-22T20:14:37Z

This updates golangci-lint, moves the configuration all to .golangci.yml and then fixes the linter errors

k8s-ci-robot · 2020-01-22T20:14:45Z

Hi @mcristina422. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

DirectXMan12

couple of questions, but mostly looks awesome.

Thanks so much!

DirectXMan12 · 2020-01-22T20:26:36Z

pkg/internal/testing/integration/etcd.go

@@ -86,7 +86,7 @@ func (e *Etcd) setProcessState() error {
 		return err
 	}

-	e.processState.StartMessage = internal.GetEtcdStartMessage(e.processState.URL)
+	e.processState.HealthCheckEndpoint = "/health"


StartMessage is deprecated. So staticcheck is failing. I was iffy on this change too, we can add //nolint to revert this change

https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/testing/integration/internal/process.go#L40

DirectXMan12 · 2020-01-22T20:27:12Z

pkg/manager/signals/signal_test.go

@@ -36,7 +36,7 @@ var _ = Describe("runtime signal", func() {
 			task := &Task{
 				ticker: time.NewTicker(time.Second * 2),
 			}
-			c := make(chan os.Signal)
+			c := make(chan os.Signal, 1)


context on this one?

SA1017: the channel used with signal.Notify should be buffered
https://staticcheck.io/docs/checks#SA1017

You can see elsewhere in the codebase

controller-runtime/pkg/manager/signals/signal.go

Line 33 in 45d6520

c := make(chan os.Signal, 2)

ah, prevents missing signals, ok.

kensipe

I'm a big fan of most of the code changes... they make sense.

The exception is etcd.go L89 change for which I don't at the moment understand the impact.

I'm most concern with the dropping of a number of enabled linters. Is the situation such that this PR would be too large to handle the update for all of them? or is it realized that these are dropped? or ? I think we need clear intention defined here.

kensipe · 2020-01-23T15:28:39Z

.golangci.yml

@@ -3,3 +3,5 @@ linters-settings:
    line-length: 170
  dupl:
    threshold: 400
+run:
+  deadline: 5m


deadline is deprecated... we should switch to timeout

details: golangci/golangci-lint#822

kensipe · 2020-01-23T15:54:23Z

hack/verify.sh

-#    --enable=gosec \
-#    --enable=maligned \
-#    --enable=safesql \
+golangci-lint run ./pkg/... ./examples/... .


The following linters have been removed and are NOT enabled by default

--enable=misspell \ --enable=golint \ --enable=goconst \ --enable=unparam \ --enable=nakedret \ --enable=interfacer \ --enable=gocyclo \ --enable=lll \ --enable=dupl \ --enable=goimports \

Is this intended? If we plan to keep them... it would be great to add them to the .golangci.yml file.

https://github.com/golangci/golangci-lint#enabled-by-default-linters

No, this was not my intention. I misinterpreted how dropping the --disable-all flag worked. I'll make sure to explicitly enable the same ones in the config file

vincepri · 2020-01-23T16:03:55Z

/ok-to-test

kensipe

@mcristina422 might I suggest that you change the one contested change around e.processState.HealthCheckEndpoint = "/health" adding a //nolint: there and we can have that debate separately?

The rest of this lgtm

mcristina422 · 2020-01-23T19:27:34Z

@kensipe I've reverted the etcd change. Since staticcheck isn't enabled currently this will pass linting

In a follow up we can debate enabling more like staticcheck, stylecheck, whitespace and others found in the more strict configuration that golangci uses themselves https://github.com/golangci/golangci-lint/blob/master/.golangci.yml

kensipe

/lgtm

kensipe · 2020-01-24T15:54:16Z

@DirectXMan12 could we get a 2nd look on this?

gerred · 2020-01-27T15:35:18Z

hack/check-everything.sh

@@ -74,7 +74,7 @@ function fetch_go_tools {
  header_text "Checking for gometalinter.v2"
  if ! is_installed golangci-lint; then
    header_text "Installing golangci-lint"
-    curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.21.0
+    curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.23.1


long term maybe we introduce a tools.go to manage these dependencies with go modules - a few other tools do this.

kensipe · 2020-02-11T17:05:22Z

@mcristina422 looks like it needs a rebase... lets see if we can get some attention on this after the rebase... thx

camilamacedo86 · 2020-02-11T17:34:03Z

pkg/client/fake/client_test.go

@@ -235,7 +236,7 @@ var _ = Describe("Fake client", func() {
 					Namespace: "ns2",
 				}
 				obj := &corev1.ConfigMap{}
-				err = cl.Get(nil, namespacedName, obj)


Why this change is required?

When running the linter with staticcheck enabled (not enforced by this PR) it fails with

SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck)

https://staticcheck.io/docs/checks#SA1012

camilamacedo86 · 2020-02-11T17:34:25Z

pkg/controller/controller_integration_test.go

@@ -152,7 +152,7 @@ var _ = Describe("controller", func() {
 			By("Invoking Reconciling for an OwnedObject when it is updated")
 			newReplicaset := replicaset.DeepCopy()
 			newReplicaset.Labels = map[string]string{"foo": "bar"}
-			newReplicaset, err = clientset.AppsV1().ReplicaSets("default").Update(newReplicaset)
+			_, err = clientset.AppsV1().ReplicaSets("default").Update(newReplicaset)


camilamacedo86 · 2020-02-11T17:34:32Z

hack/verify.sh

@@ -24,29 +24,6 @@ go vet ${MOD_OPT} ./...

 header_text "running golangci-lint"

-golangci-lint run --disable-all \


camilamacedo86 · 2020-02-11T17:34:38Z

.golangci.yml

 linters-settings:
  lll:
    line-length: 170
  dupl:
    threshold: 400
+
+run:
+  timeout: 5m


vincepri · 2020-02-11T17:49:26Z

.golangci.yml

@@ -1,5 +1,27 @@
+linters:
+  disable-all: true


I'd suggest to flip these. Enable all linters and explicitly disable the others, so we know which ones we are disabling and folks can pick up the task of removing a disabled linter from this list if they wanted to.

This is explicitly called out here because enable-all is deprecated

https://github.com/golangci/golangci-lint/blob/b9eef79121fff235d0d794c176ffa2b3d9bd422f/.golangci.yml#L59-L60

Oh well, it seems a bit counter intuitive to me, but nothing major

camilamacedo86

It shows great for me 👍
/lgtm

vincepri · 2020-02-21T18:05:56Z

/approve

k8s-ci-robot · 2020-02-21T18:06:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mcristina422, vincepri

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:

~~OWNERS~~ [vincepri]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 22, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 22, 2020

k8s-ci-robot requested review from DirectXMan12 and droot January 22, 2020 20:16

DirectXMan12 suggested changes Jan 22, 2020

View reviewed changes

gerred mentioned this pull request Jan 23, 2020

Add 'kensipe' as reviewer #775

Closed

kensipe reviewed Jan 23, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 23, 2020

kensipe reviewed Jan 23, 2020

View reviewed changes

kensipe approved these changes Jan 23, 2020

View reviewed changes

k8s-ci-robot assigned kensipe Jan 23, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2020

gerred reviewed Jan 27, 2020

View reviewed changes

DirectXMan12 self-assigned this Feb 5, 2020

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

camilamacedo86 reviewed Feb 11, 2020

View reviewed changes

.golangci.yml

linters-settings:

lll:

line-length: 170

dupl:

threshold: 400

run:

timeout: 5m

Copy link

Member

camilamacedo86 Feb 11, 2020

Choose a reason for hiding this comment

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

👍 cool

Fix linter errors, update and move lint to config file

fd19c6a

mcristina422 force-pushed the moreLinters branch from c17d688 to fd19c6a Compare February 11, 2020 17:45

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 11, 2020

vincepri reviewed Feb 11, 2020

View reviewed changes

camilamacedo86 approved these changes Feb 11, 2020

View reviewed changes

k8s-ci-robot assigned camilamacedo86 Feb 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2020

vincepri added this to the v0.5.x milestone Feb 21, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2020

k8s-ci-robot merged commit e00985b into kubernetes-sigs:master Feb 21, 2020

kensipe mentioned this pull request Feb 21, 2020

re-enable golangci-lint's godoc comment checking #501

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏃 Update linter and lint fixes #773

🏃 Update linter and lint fixes #773

mcristina422 commented Jan 22, 2020 •

edited

k8s-ci-robot commented Jan 22, 2020

DirectXMan12 left a comment

DirectXMan12 Jan 22, 2020

mcristina422 Jan 22, 2020

DirectXMan12 Jan 22, 2020

mcristina422 Jan 22, 2020 •

edited

gerred Jan 27, 2020

kensipe left a comment

kensipe Jan 23, 2020

kensipe Jan 23, 2020

mcristina422 Jan 23, 2020

vincepri commented Jan 23, 2020

kensipe left a comment

mcristina422 commented Jan 23, 2020

kensipe left a comment

kensipe commented Jan 24, 2020

gerred Jan 27, 2020

kensipe commented Feb 11, 2020

camilamacedo86 Feb 11, 2020

mcristina422 Feb 11, 2020

camilamacedo86 Feb 11, 2020

camilamacedo86 Feb 11, 2020

camilamacedo86 Feb 11, 2020

vincepri Feb 11, 2020

mcristina422 Feb 11, 2020

vincepri Feb 11, 2020

camilamacedo86 left a comment

vincepri commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

		@@ -24,29 +24,6 @@ go vet ${MOD_OPT} ./...

		header_text "running golangci-lint"

		golangci-lint run --disable-all \

🏃 Update linter and lint fixes #773

🏃 Update linter and lint fixes #773

Conversation

mcristina422 commented Jan 22, 2020 • edited

k8s-ci-robot commented Jan 22, 2020

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcristina422 Jan 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jan 23, 2020

kensipe left a comment

Choose a reason for hiding this comment

mcristina422 commented Jan 23, 2020

kensipe left a comment

Choose a reason for hiding this comment

kensipe commented Jan 24, 2020

Choose a reason for hiding this comment

kensipe commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

vincepri commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

mcristina422 commented Jan 22, 2020 •

edited

mcristina422 Jan 22, 2020 •

edited