🌱 Enable excluded lint checks #4624

stmcginnis · 2021-05-14T21:33:55Z

What this PR does / why we need it:

There are a set of lint checks that are disabled by default with
golangci-lint. This enables them by setting exclude-use-default to
false.

This by itself triggers a set of lint failures due to these additional
checks. To by systematic about addressing these potential issues, this
adds those checks back as individual exclusions so they can be addressed
on by one.

This eliminates one of those individual checks by addressing cases of
"Potential file inclusion via variable" where passed in variables are
used in os.OpenFile calls.

Which issue(s) this PR fixes:
Related #4622

k8s-ci-robot · 2021-05-14T21:34:03Z

Hi @stmcginnis. 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.

fabriziopandini · 2021-05-16T20:10:17Z

.golangci.yml

@@ -38,10 +38,17 @@ linters:
 issues:
  max-same-issues: 0
  max-issues-per-linter: 0
+  exclude-use-default: false


What about adding a comment here:

We are disabling default golangci exclusions because we want to help reviewers to focus on reviewing the most relevant changes in PRs and not too much on nitpicking.

fabriziopandini · 2021-05-16T20:14:57Z

.golangci.yml

+    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
+    - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)
+    - Subprocess launch(ed with variable|ing should be audited)
+    - (G104|G307)
+    - (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)
+    - package comment should be of the form "Package (.+) ..."


I would suggest to restructure this a bit so we group

Default golangci exclusions that we would like to remove as soon as possible

Default golangci exclusions that we preserve (possibily explaining why)

Additional exclusions that we added in CAPI (possibily explaining why)

Also, in the golangci doc I see exclusions being numbered; what about preserving this number inner comments ?

fabriziopandini · 2021-05-16T20:15:19Z

/ok-to-test

.golangci.yml

There are a set of lint checks that are disabled by default with golangci-lint. This enables them by setting exclude-use-default to false. This by itself triggers a set of lint failures due to these additional checks. To by systematic about addressing these potential issues, this adds those checks back as individual exclusions so they can be addressed on by one. This eliminates one of those individual checks by addressing cases of "Potential file inclusion via variable" where passed in variables are used in os.OpenFile calls. Signed-off-by: Sean McGinnis <[email protected]>

vincepri

/lgtm

sbueringer · 2021-05-17T16:26:35Z

.golangci.yml

  # List of regexps of issue texts to exclude, empty list by default.
  exclude:
    - Using the variable on range scope `(tc)|(rt)|(tt)|(test)|(testcase)|(testCase)` in function literal
    - "G108: Profiling endpoint is automatically exposed on /debug/pprof"
+    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked


I wonder if we want to get rid of a few of them. not the ones about fmt.Fprintf, but e.g. File.Close()

As mentioned above, file.Close() in a defer is actually useful. While ignoring the error can be odd, if a file cannot be closed (because it's a syscall) an error is usually thrown way earlier.

Okay, missed your comment above. Then it's fine for me.

Just had a few of those when close failed when writing to S3/GCS and it's pretty hard to debug. But we also have "manual" reviews, so we should be good

sbueringer · 2021-05-17T16:27:55Z

.golangci.yml

+    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
+    # The following are being worked on to remove their exclusion. This list should be reduced or go away all together over time.
+    # If it is decided they will not be addressed they should be moved above this comment.
+    - (comment on exported (method|function|type|const)|should have( a package)? comment|comment should be of the form)


@stmcginnis That's the one which would detect these kinds of error right? https://github.com/kubernetes-sigs/cluster-api/pull/4607/files

Yep, I believe so. Next in the list to tackle. ;)

sbueringer · 2021-05-17T16:41:34Z

.golangci.yml

@@ -38,10 +38,21 @@ linters:
 issues:


Some additional ideas for linters:

exportloopref: checks for pointers to enclosing loop variables (https://github.com/kyoh86/exportloopref)

gci: Gci control golang package import order and make it always deterministic. (https://github.com/daixiang0/gci)

would help to enforce imports in 3 groups: sdk, external, local (i.e. goimports --local) (if we want that)

gofumpt: Gofumpt checks whether code was gofumpt-ed. (https://github.com/mvdan/gofumpt)

slightly more opinionated than gofmt (over time I found it pretty much matches my own preferences :))

ifshort: Checks that your code uses short syntax for if-statements whenever possible (https://github.com/esimonov/ifshort)

nilerr: Finds the code that returns nil even if it checks that the error is not nil. (https://github.com/gostaticanalysis/nilerr)

ref: https://golangci-lint.run/usage/linters/

Just some ideas. I don't want to start a lengthy discussion like tab vs spaces :). But if we get consensus on one / a few of them, that would be nice.

If we want them, they should probably be added commented out here and then introduced one at a time (like with the exclude below)

+1 on exportloopref, ifshort, and nilerr

The others require new tooling that are more stylistic than not, which I'm generally fine to leave as is

fabriziopandini · 2021-05-18T10:25:15Z

/lgtm

vincepri

/approve

k8s-ci-robot · 2021-05-18T15:14:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

stmcginnis · 2021-05-18T15:21:49Z

/retest

Timeouts that shouldn't be related.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 14, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 14, 2021

k8s-ci-robot requested review from fabriziopandini and justinsb May 14, 2021 21:34

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 14, 2021

stmcginnis mentioned this pull request May 14, 2021

Investigate if we should enable additional linters (through golangci-lint) #4622

Closed

9 tasks

fabriziopandini reviewed May 16, 2021

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 May 16, 2021

stmcginnis force-pushed the enablelinting branch from 84b1ac3 to 0385c30 Compare May 17, 2021 11:56

vincepri reviewed May 17, 2021

View reviewed changes

.golangci.yml Show resolved Hide resolved

.golangci.yml Show resolved Hide resolved

.golangci.yml Outdated Show resolved Hide resolved

stmcginnis force-pushed the enablelinting branch from 0385c30 to 6a24e15 Compare May 17, 2021 16:02

vincepri approved these changes May 17, 2021

View reviewed changes

k8s-ci-robot assigned vincepri May 17, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2021

sbueringer reviewed May 17, 2021

View reviewed changes

k8s-ci-robot assigned fabriziopandini May 18, 2021

vincepri approved these changes May 18, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2021

k8s-ci-robot merged commit a78914c into kubernetes-sigs:master May 18, 2021

k8s-ci-robot added this to the v0.4 milestone May 18, 2021

stmcginnis deleted the enablelinting branch May 18, 2021 16:07

sbueringer mentioned this pull request May 19, 2021

WIP: 🌱 Enable gci linter #4633

Closed

sedefsavas mentioned this pull request May 26, 2021

ISSUE-2414: Add additional linters kubernetes-sigs/cluster-api-provider-aws#2431

Merged

4 tasks

sedefsavas mentioned this pull request Jun 4, 2021

Enable additional linters kubernetes-sigs/cluster-api-provider-aws#2471

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Enable excluded lint checks #4624

🌱 Enable excluded lint checks #4624

stmcginnis commented May 14, 2021

k8s-ci-robot commented May 14, 2021

fabriziopandini May 16, 2021

fabriziopandini May 16, 2021

fabriziopandini commented May 16, 2021

vincepri left a comment

sbueringer May 17, 2021 •

edited

Loading

vincepri May 17, 2021

sbueringer May 17, 2021 •

edited

Loading

sbueringer May 17, 2021

stmcginnis May 17, 2021

sbueringer May 17, 2021 •

edited

Loading

vincepri May 17, 2021 •

edited

Loading

fabriziopandini commented May 18, 2021

vincepri left a comment

k8s-ci-robot commented May 18, 2021

stmcginnis commented May 18, 2021

🌱 Enable excluded lint checks #4624

🌱 Enable excluded lint checks #4624

Conversation

stmcginnis commented May 14, 2021

k8s-ci-robot commented May 14, 2021

fabriziopandini May 16, 2021

Choose a reason for hiding this comment

fabriziopandini May 16, 2021

Choose a reason for hiding this comment

fabriziopandini commented May 16, 2021

vincepri left a comment

Choose a reason for hiding this comment

sbueringer May 17, 2021 • edited Loading

Choose a reason for hiding this comment

vincepri May 17, 2021

Choose a reason for hiding this comment

sbueringer May 17, 2021 • edited Loading

Choose a reason for hiding this comment

sbueringer May 17, 2021

Choose a reason for hiding this comment

stmcginnis May 17, 2021

Choose a reason for hiding this comment

sbueringer May 17, 2021 • edited Loading

Choose a reason for hiding this comment

vincepri May 17, 2021 • edited Loading

Choose a reason for hiding this comment

fabriziopandini commented May 18, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 18, 2021

stmcginnis commented May 18, 2021

sbueringer May 17, 2021 •

edited

Loading

sbueringer May 17, 2021 •

edited

Loading

sbueringer May 17, 2021 •

edited

Loading

vincepri May 17, 2021 •

edited

Loading