Enhance the static-analysis workflow

[ahrtr]

Some issues were found using revive, such as,

• https://github.com/etcd-io/etcd/issues/14162
• https://github.com/etcd-io/etcd/pull/14160
• https://github.com/etcd-io/etcd/pull/14163

We should enhance the static-analysis workflow. The revive has already been integrated into golangci-lint. So I think we can enhance the static-analysis using the golangci-lint ?

cc @serathius @ptabor @spzala @chavacava @ldez @jirfag @ernado

[chavacava]

revive has more than 65 rules and, of course, there is some overlap with those of the other linters golangci-lint can run.

When looking for potential bugs I use the following revive configuration:

ignoreGeneratedHeader = false
severity = "warning"
confidence = 0.8
errorCode = 1
warningCode = 0

[rule.constant-logical-expr]
[rule.time-equal]
[rule.unreachable-code]
[rule.struct-tag]
[rule.modifies-value-receiver]
[rule.bool-literal-in-expr]
[rule.range-val-in-closure]
[rule.range-val-address]
[rule.datarace]
[rule.string-of-int]
[rule.unconditional-recursion]
[rule.identical-branches]

[cenkalti]

I am willing to work on this task is no one else has started yet. I can integrate golangci-lint. After integration is done we can talk about which linter to turn on/off and rules.

[ramses]

Yesterday I finished exactly this for another project — i.e., golangci-lint for the project, then added a github action.

I've just noticed that we do not have the following:

• .golangci.yml configuration.
• make target for linting — this and the above bullet would enable repeatable local linting.

Also, the static analysis workflow calls the scripts/test.sh. Instead, we could use the golangci-lint github-action for static analysis. Note, that this should simplify the test.sh script.

It seems that it would be better to do a pull request for the bullets above, and then after that enable the github action.

Your thoughts?

[cenkalti]

It's better to use Github actions instead of writing more script code.

[ramses]

It's better to use Github actions instead of writing more script code.

I'm not recommending more script code.

I am recommending LESS script code.

A configuration file for golangci-lint would enable consistent local linting.

Using the golangci-lint-github-action would simplify scripts/test.sh. Note that this action would take advantage of the configuration file.

Your thoughts?

[cenkalti]

Agreed. That's what I was trying to say.

[ahrtr]

Thanks both @cenkalti and @ramses for the feedback. Actually I tried the golangci-lint as well about 9 months ago in project gocontainer, and I believe it's definitely useful, but we also need to avoid some false alarms.

@serathius @spzala @ptabor do you have any immediate concern on this approach? If no any objection, @cenkalti and @ramses , please any of you feel free to deliver a PR on this.

[ernado]

Don't be shy to ignore false positives. Here is example from one of my projects:

issues:
  exclude-use-default: false
  exclude-rules:
    # TODO(ernado): Should we refactor Parse() functions?
    - path: exchange
      linters: [ gocyclo, gocognit ]
      text: "Run"

    - linters: [ gocritic ]
      text: "commentedOutCode"
      source: "SHA1"

    # Exclude go:generate from lll
    - source: "//go:generate"
      linters: [ lll ]

    # Disable linters that are annoying in tests.
    - path: _test\.go
      linters:
        - gocyclo
        - errcheck
        - dupl
        - gosec
        - funlen
        - goconst
        - gocognit
        - scopelint
        - lll

    # Ignore shadowing of err.
    - linters: [ govet ]
      text: 'declaration of "(err|ctx|log)"'

    # Ignore linters in main packages.
    - path: cmd\/.+\/main\.go
      linters: [ goconst, funlen, gocognit, gocyclo ]

    # Intended in commands:
    # G307: Deferring unsafe method "Close" on type "*os.File"
    # G304: Potential file inclusion via variable
    - path: cmd\/.+\/main\.go
      text: G(307|304)

    - linters: [ goconst ]
      text: 'string `(bool|int64)`'

    - path: tgtest
      linters: [ revive, golint ]
      text: "should have comment or be unexported"

    - linters: [ staticcheck ]
      text: "SA1019: telegram.+ is deprecated:"

    - linters: [ staticcheck ]
      text: 'SA1019: strings\.Title has been deprecated'

    - linters: [ revive ]
      text: "if-return: redundant if ...; err != nil check, just return error instead."

    - linters: [ unparam ]
      text: "Builder.+is never used"

The exclude-rules section is very flexible and it is ok to disable some checks fully or partially.

[serathius]

+1 to adding .golangci.yml configuration. I would start with Kubernetes one. We should avoid having too strict rules that would discourage new contributors.

+1 for direct target for linting. We have PASSES='fmt bom dep' ./scripts/test.sh however I find it very unnatural. Cleaning up makefile was always on my list of things that I want to do, but don't have time.

One note about adding more strict static analysis. We should watch out for alienating contributors that don't know/use same linters. The stricter the linting, the more automation for fixing we should have. I would recommend also adding a makefile target for fixing (just calls golangci-lint run --fix).

+1 To less scripting. We should unify/simplify developer and CI interface. Commands like build, test, lint should be simple to access, but also there should not be big difference in how CI does it. Would recommend to start from wrapping commonly used ./scripts/tests.sh invocations with makefile targets and switching CI to use them.

My favorite example for how static analysis commands should work is https://github.com/kubernetes-sigs/metrics-server/blob/master/Makefile#L184-L289, for every verify-X command you have update-X command that should fix the code to pass verify-X. You also have commands verify and update that just run everything. However I'm biased because I wrote this :)

[ernado]

Also ruleguard is interesting for advanced linting, you can write custom linting rules with DSL.

[ramses]

Initial PR, adding the configuration file: https://github.com/etcd-io/etcd/pull/14255

Please carefully read the commit message.

Thank you, @serathius for the sensible suggestion to base our config from Kubernetes' golangci-lint configuration.

[chavacava]

The chosen configuration of golangci-lint does not include revive thus there is still the risk of not detecting bugs as those mentioned in the initial message of this discussion.

[ahrtr]

The chosen configuration of golangci-lint does not include revive thus there is still the risk of not detecting bugs as those mentioned in the initial message of this discussion.

We will add revive in separate PR. https://github.com/etcd-io/etcd/pull/14255#discussion_r930459460

[ahrtr]

This task isn't finished yet. https://github.com/etcd-io/etcd/pull/14255 is just the very first step. Please anyone feel free to continue to work on this task.

[cenkalti]

I added the GitHub action: https://github.com/etcd-io/etcd/pull/14392 I added revive to the list of linters: https://github.com/etcd-io/etcd/pull/14391

Can you review these please?

[ahrtr]

I added the GitHub action: #14392 I added revive to the list of linters: #14391

Can you review these please?

Thank you @cenkalti . Approved and merged.

[ahrtr]

Please consider to resolve https://github.com/etcd-io/etcd/pull/14479#issuecomment-1249195534

[ahrtr]

Looks this issue has already been resolved. Thanks all.