Forem: Oleg Kovalov

Go linters configuration, the right version.

Oleg Kovalov — Tue, 28 Mar 2023 07:06:33 +0000

Originally posted here: https://olegk.dev/go-linters-configuration-the-right-version

TLDR: See the golangci-lint config that I find useful Github Gist

Intro

Ah, these mighty linters. Tools that intend to make our life better but might hurt our software engineering egos.

Somewhere in 2018, I was super eager to make my code and others' code better. Thankfully, I met @quasilyte on Gophers Slack and we made "The most opinionated Go source code linter" go-critic.

But we were not alone, golangci-lint started as a gometalinter replacement and became a multi-linter in the Go ecosystem. If you have a linter and it makes code better for everyone and not only for your specific use case, consider adding it to golangci-lint so everyone can benefit from that.

(a funny thing that I found during writing this post: both repositories start their git commit history Apr 29, 2018, LOL)

As for today golangci-lint contains 120+ linters and what I often notice:

not everyone is aware of how to configure it
what are the pros and cons of some fields
what is The Best™ golangci-lint config
etc.

In this post, I'm going to share my config and some thoughts on why I prefer this or that. The post might look long due to the big YAML sections (hah, YAML), but most of the stuff is quite simple.

Note: this post is based on golangci-lint version v1.52.1.

Linters environment

When you're planning to start using or running a linter you need to understand the environment. I see 2 cases:

personal projects
- where you decide how to write the code.
team projects
- where there are other members with their opinion.

For personal projects everything is cool - just do the stuff, run a linter with any config you want, no problems.

But with a team situation is different. If you are going to introduce a linter, change its config or disable a check - consider discussing it first, to have a team-wide agreement. This will cause less conflicts later (both code and personal).

Everything that is about style is subjective. 1 newline in a pull request can start a 10+ comments thread because everyone has something to say. I would like to say this was a joke but it's not, true story.

Running a linter

Software engineering or coding in general is like riding a bike. When you're young and not that experienced you gonna use these small additional wheels to move stably. But when you're experienced enough these wheels will distract you more and more. As you can guess wheels are linters.

After 5 years of writing Go daily I found that I don't see linter's suggestions because there is nothing to suggest. So, I switched from automatic runs to the manual, when I know/feel/think that something might be missed. There is a big chance that someone might find this idea over-optimistic but I know I'm not alone, consider this as a come out.

A short disclaimer: formatting or styling suggestions are fine, only bugs are unacceptable.

As for junior and middle engineers I strongly suggest to go 1st option and don't rush to disable the linter. Instead, I heavily recommend enabling more linters, to see more suggestions and keep learning from them. Even if you will not find yourself in a situation when the linter distracts you - it's fine, this doesn't impact your professional skills.

My handy command

I have a shell alias in my ~/.zshrc:

alias lnt="golangci-lint run --config=~/.golangci.yaml ./..."

Just type lnt in a specific directory and see the results. It's very handy when I do git clone of a repository and try to understand how good it is for me.

The config

It consists of 5 parts (as for golangci-lint v1.52.1):

run
- general config of the linter
output
- how the result should be presented
issues & severity
- how to treat the results
linters & linters-settings
- what linters to run and how to configure them

Let's discuss each of them.

run

The run object is responsible for the golangci-lint run. Here is what I found useful:

run:
  # Depends on your hardware, my laptop can survive 8 threads.
  concurrency: 8

  # I really care about the result, so I'm fine to wait for it.
  timeout: 30m

  # Fail if the error was met.
  issues-exit-code: 1

  # This is very important, bugs in tests are not acceptable either.
  tests: true

  # In most cases this can be empty but there is a popular pattern
  # to keep integration tests under this tag. Such tests often require
  # additional setups like Postgres, Redis etc and are run separately.
  # (to be honest I don't find this useful but I have such tags)
  build-tags:
    - integration 

  # Up to you, good for a big enough repo with no-Go code.
  skip-dirs:
    # - src/external_libs

  # When enabled linter will skip directories: vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
  # Skipping `examples` sounds scary to me but skipping `testdata` sounds ok.
  skip-dirs-use-default: false

  # Autogenerated files can be skipped (I'm looking at you gRPC).
  # AFAIK autogen files are skipped but skipping the whole directory should be somewhat faster.
  #skip-files:
  #  - "protobuf/.*.go"

  # With the read-only mode linter will fail if go.mod file is outdated.
  modules-download-mode: readonly

  # Till today I didn't know this param exists, never ran 2 golangci-lint at once.
  allow-parallel-runners: false

  # Keep this empty to use the Go version from the go.mod file.
  go: ""

output

Let's briefly talk about output. As you can guess it's all about how results will be presented.

As I have shared my command (in My Handy command section) I'm using a bash alias. The results are saved in a lint.txt file that I can process in the editor.

output:
  # I prefer the simplest one: `line-number` and saving to `lint.txt`
  #
  # The `tab` also looks good and with the next release I will switch to it
  # (ref: https://github.com/golangci/golangci-lint/issues/3728)
  #
  # There are more formats which can be used on CI or by your IDE.
  format: line-number:lint.txt

  # I do not find this useful, parameter above already enables filepath
  # with a line and column. For me, it's easier to follow the path and
  # see the line in an IDE where I see more code and understand it better.
  print-issued-lines: false

  # Must have. Easier to understand the output.
  print-linter-name: true

  # No, no skips, everything should be reported.
  uniq-by-line: false

  # To be honest no idea when this can be needed, maybe a multi-module setup?
  path-prefix: ""

  # Slightly easier to follow the results + getting deterministic output.
  sort-results: true

issues & severity

Configuring the issues:

issues:
  # I found it strange to skip the errors, setting 0 to have all the results.
  max-issues-per-linter: 0

  # Same here, nothing should be skipped to not miss errors.
  max-same-issues: 0

  # When set to `true` linter will analyze only new code which are
  # not committed or after some specific revision. This is a cool 
  # feature when you're going to introduce linter into a big project.
  # But I prefer going gradually package by package. 
  # So, it's set to `false` to scan all code.
  new: false

  # 2 other params regarding git integration

  # Even with a recent GPT-4 release I still believe that
  # I know better how to do my job and fix the suggestions.
  fix: false

To be honest, I have never configured the severity part, it was always empty in my configuration, have nothing to recommend for you.

If you are using non-default parameters, please share your setup in the comments or DM me, happy to hear.

Linters to run

The core idea of the article: linters to enable or disable. Let me make a few disclaimers:

1) The following config is based on my experience and my preferences.

2) The linters that I keep disabled should not be treated as "this linter is bad" or "linter's author did a bad thing". Every linter makes sense, it just depends on the context and codebase.

How to find your best config: I recommend you enable all available linters, run on your codebase (or a few of them), analyze the results and decide what is important for you and what is not. Again: if this config will be used in a team - discuss it before adopting it, and be friendly.

(to get all available linters run: golangci-lint help linters)

In other words: YMMV (your mileage might vary). Keep that in mind. Let's go:

linters:
  # Set to true runs only fast linters.
  # Good option for 'lint on save', pre-commit hook or CI.
  fast: false

  enable:
    # Check for pass []any as any in variadic func(...any).
    # Rare case but saved me from debugging a few times.
    - asasalint

    # I prefer plane ASCII identifiers.
    # Symbol `∆` instead of `delta` looks cool but no thanks.
    - asciicheck

    # Checks for dangerous unicode character sequences.
    # Super rare but why not to be a bit paranoid?
    - bidichk

    # Checks whether HTTP response body is closed successfully.
    - bodyclose

    # Check whether the function uses a non-inherited context.
    - contextcheck

    # Check for two durations multiplied together.
    - durationcheck

    # Forces to not skip error check.
    - errcheck 

    # Checks `Err-` prefix for var and `-Error` suffix for error type.
    - errname

    # Suggests to use `%w` for error-wrapping.
    - errorlint

    # Checks for pointers to enclosing loop variables.
    - exportloopref

    # As you already know I'm a co-author. It would be strange to not use
    # one of my warmly loved projects.
    - gocritic

    # Forces to put `.` at the end of the comment. Code is poetry.
    - godot

    # Might not be that important but I prefer to keep all of them.
    # `gofumpt` is amazing, kudos to Daniel Marti https://github.com/mvdan/gofumpt
    - gofmt
    - gofumpt
    - goimports

    # Allow or ban replace directives in go.mod
    # or force explanation for retract directives.
    - gomoddirectives

    # Powerful security-oriented linter. But requires some time to
    # configure it properly, see https://github.com/securego/gosec#available-rules
    - gosec

    # Linter that specializes in simplifying code.
    - gosimple

    # Official Go tool. Must have. 
    - govet

    # Detects when assignments to existing variables are not used
    # Last week I caught a bug with it.
    - ineffassign

    # Even with deprecation notice I find it useful.
    # There are situations when instead of io.ReaderCloser 
    # I can use io.Reader. A small but good improvement.
    - interfacer

    # Fix all the misspells, amazing thing.
    - misspell

    # Finds naked/bare returns and requires change them.
    - nakedret

    # Both require a bit more explicit returns.
    - nilerr
    - nilnil

    # Finds sending HTTP request without context.Context.
    - noctx

    # Forces comment why another check is disabled.
    # Better not to have //nolint: at all ;)
    - nolintlint

    # Finds slices that could potentially be pre-allocated.
    # Small performance win + cleaner code.
    - prealloc

    # Finds shadowing of Go's predeclared identifiers.
    # I hear a lot of complaints from junior developers.
    # But after some time they find it very useful.
    - predeclared

    # Lint your Prometheus metrics name.
    - promlinter

    # Checks that package variables are not reassigned.
    # Super rare case but can catch bad things (like `io.EOF = nil`)
    - reassign

    # Drop-in replacement of `golint`.
    - revive

    # Somewhat similar to `bodyclose` but for `database/sql` package.
    - rowserrcheck
    - sqlclosecheck

    # I have found that it's not the same as staticcheck binary :\
    - staticcheck

    # Is a replacement for `golint`, similar to `revive`.
    - stylecheck

    # Check struct tags.
    - tagliatelle

    # Test-related checks. All of them are good.
    - tenv
    - testableexamples
    - thelper
    - tparallel

    # Remove unnecessary type conversions, make code cleaner
    - unconvert

    # Might be noisy but better to know what is unused
    - unparam

    # Must have. Finds unused declarations.
    - unused

    # Detect the possibility to use variables/constants from stdlib.
    - usestdlibvars

    # Finds wasted assignment statements.
    - wastedassign

And know some linters that I prefer to keep disabled. Again, my use case, yours might be different:

  disable:
    # Detects struct contained context.Context field. Not a problem.
    - containedctx

    # Checks function and package cyclomatic complexity.
    # I can have a long but trivial switch-case.
    #
    # Cyclomatic complexity is a measurement, not a goal.
    # (c) Bryan C. Mills / https://github.com/bcmills
    - cyclop

    # Abandoned, replaced by `unused`.
    - deadcode

    # Check declaration order of types, consts, vars and funcs.
    # I like it but I don't use it.
    - decorder

    # Checks if package imports are in a list of acceptable packages.
    # I'm very picky about what I import, so no automation.
    - depguard

    # Checks assignments with too many blank identifiers. Very rare.
    - dogsled

    # Tool for code clone detection.
    - dupl

    # Find duplicate words, rare.
    - dupword

    # I'm fine to check the error from json.Marshal ¯\_(ツ)_/¯
    - errchkjson

    # All SQL queries MUST BE covered with tests.
    - execinquery

    # Forces to handle more cases. Cool but noisy.
    - exhaustive
    - exhaustivestruct # Deprecated, replaced by check below.
    - exhaustruct

    # Forbids some identifiers. I don't have a case for it.
    - forbidigo

    # Finds forced type assertions, very good for juniors.
    - forcetypeassert

    # I might have long but a simple function.
    - funlen

    # Imports order. I do this manually ¯\_(ツ)_/¯
    - gci

    # I'm not a fan of ginkgo and gomega packages.
    - ginkgolinter

    # Checks that compiler directive comments (//go:) are valid. Rare.
    - gocheckcompilerdirectives

    # Globals and init() are ok.
    - gochecknoglobals
    - gochecknoinits

    # Same as `cyclop` linter (see above)
    - gocognit
    - goconst
    - gocyclo

    # TODO and friends are ok.
    - godox

    # Check the error handling expressions. Too noisy.
    - goerr113

    # I don't use file headers.
    - goheader

    # 1st Go linter, deprecated :( use `revive`.
    - golint

    # Reports magic consts. Might be noisy but still good.
    - gomnd

    # Allowed/blocked packages to import. I prefer to do it manually.
    - gomodguard

    # Printf-like functions must have -f.
    - goprintffuncname

    # Groupt declarations, I prefer manually.
    - grouper

    # Deprecated.
    - ifshort

    # Checks imports aliases, rare.
    - importas

    # Forces tiny interfaces, very subjective.
    - interfacebloat

    # Accept interfaces, return types. Not always.
    - ireturn

    # I don't set line length. 120 is fine by the way ;)
    - lll

    # Some log checkers, might be useful.
    - loggercheck

    # Maintainability index of each function, subjective.
    - maintidx

    # Slice declarations with non-zero initial length. Not my case.
    - makezero

    # Deprecated. Use govet `fieldalignment`.
    - maligned

    # Enforce tags in un/marshaled structs. Cool but not my case.
    - musttag

    # Deeply nested if statements, subjective.
    - nestif

    # Forces newlines in some places.
    - nlreturn

    # Reports all named returns, not that bad.
    - nonamedreturns

    # Deprecated. Replaced by `revive`.
    - nosnakecase

    # Finds misuse of Sprintf with host:port in a URL. Cool but rare.
    - nosprintfhostport

    # I don't use t.Parallel() that much.
    - paralleltest

    # Often non-`_test` package is ok.
    - testpackage

    # Compiler can do it too :)
    - typecheck

    # I'm fine with long variable names with a small scope.
    - varnamelen

    # gofmt,gofumpt covers that (from what I know).
    - whitespace

    # Don't find it useful to wrap all errors from external packages.
    - wrapcheck

    # Forces you to use empty lines. Great if configured correctly.
    # I mean there is an agreement in a team.
    - wsl

Linters config

Many of these linters can be configured more precisely via linters-settings config. Initially I planned to write about all of them but I think it will be just a waste of time. See Linters page and adapt to your own needs.

linters-settings:
  # I'm biased and I'm enabling more than 100 checks
  # Might be too much for you. See https://go-critic.com/overview.html
  gocritic:
    enabled-tags:
      - diagnostic
      - experimental
      - opinionated
      - performance
      - style
    disabled-checks:
      # These 3 will detect many cases, but they do sense
      # if it's performance oriented code
      - hugeParam
      - rangeExprCopy
      - rangeValCopy

  errcheck:
    # Report `a := b.(MyStruct)` when `a, ok := ...` should be.
    check-type-assertions: true # Default: false

    # Report skipped checks:`num, _ := strconv.Atoi(numStr)`.
    check-blank: true # Default: false

    # Function to skip.
    exclude-functions:
      - io/ioutil.ReadFile
      - io.Copy(*bytes.Buffer)
      - io.Copy(os.Stdout)

  govet:
    disable:
      - fieldalignment # I'm ok to waste some bytes

  nakedret:
    # No naked returns, ever.
    max-func-lines: 1 # Default: 30

  tagliatelle:
    case:
      rules:
        json: snake # why it's not a `snake` by default?!
        yaml: snake # why it's not a `snake` by default?!
        xml: camel
        bson: camel
        avro: snake
        mapstructure: kebab

Conclusions

I hope I haven't missed some linters. If so - don't forget to mention this in the comments. Here is the config as a Github gist. Feel free to copy and adapt it. There is no ~~silver bullet~~ perfect config, find yours.

Aaaand a huge kudos to Ludovic Fernandez who is currently keeping golangci-lint alive. Great job!

Thanks.

Links

DEV / Lobsters / Reddit / Twitter

Github Actions and Go

Oleg Kovalov — Tue, 21 Mar 2023 08:26:13 +0000

Originally posted here: https://olegk.dev/github-actions-and-go

TLDR: See cristalhq/.github build workflow and how it can be used cristalhq/jsn

Intro

I love open source, and also I love Go. So, a few months ago I decided to build the best CI for Go that I could easily reuse across my projects.

This post shares the results.

Note: Post is based on version v0.5.0 of cristalhq/.github repository.

Github Actions

You probably know what GitHub Actions are, and because they are well-integrated with GitHub, I am too lazy to look at alternatives.

The only thing that I recommend is to look at Workflow syntax which explains what syntax is available for us.

Dependabot

I've noticed that not all developers are happy with Dependabot notifications (pull requests), and they find them noisy. For me, they do more good than bad. Consider just making the interval parameter bigger (a week or longer).

In almost all my projects I use the following Dependabot configuration:

version: 2
updates:
  - package-ecosystem: gomod
    directory: "/"
    schedule:
      interval: daily
  - package-ecosystem: GitHub-actions
    directory: "/"
    schedule:
      interval: daily

Save the YAML above in .github/dependabot.yml and it's done.

Basic workflow

The default Github Actions for Go looks like this:

name: Go

on:
  push:
    branches: ["main"]
  pull_request:
    branches: ["main"]

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v3

    - name: Set up Go
      uses: actions/setup-go@v4
      with:
        go-version: 1.19

    - name: Build
      run: go build -v ./...

    - name: Test
      run: go test -v ./...

In many cases it's ok but we can do better. Let's add more good things to it.

Regular runs

A few times I got stuck with a failing CI because I missed some 3rd party changes and found them too late, running CI regularly prevents that:

on:
  schedule:
  - cron: '0 10 * * 1' # run "At 10:00 on Monday"

A cron expression is an example, play on crontab guru to find yours.

Runs on and Go version

Most of my projects do not interact with the operating system, so running on Ubuntu is enough.

It's a good practice to keep your code working on the current and previous Go versions. It's not a strict rule, but it might simplify life for you or your users when a new Go version is released.

The actions/setup-go@v4 action names them as stable and oldstable:

jobs:
  run:
    name: Build
    runs-on: ubuntu-latest
    timeout-minutes: 5 # just in case ¯\_(ツ)_/¯
    strategy:
      matrix:
        go: ['stable', 'oldstable']

First steps:

Just check out the code and install Go. No magic:

    steps:
    - name: Check out code
      uses: actions/checkout@v3

    - name: Install Go
      uses: actions/setup-go@v4
      with:
        go-version: ${{ matrix.go }}
        check-latest: true

check-latest set to true will pick the latest Go release for the given Go version. For example, for Go 1.20, it will download Go 1.20.2 (which is the latest at the moment of writing this post).

actions/setup-go@v4 also supports caching out of the box. Please see README section. I don't have this configured because most of my projects are dependency-free or I don't care that much.

Go steps

Enough to setup the CI job, time to do real work!

Formatting and go vet

If code isn't formatted there is no sense to build it (or even to review). Same regarding go vet. It's the smallest linter but reports good things:

      - name: Go Format
        run: gofmt -s -w . && git diff --exit-code

      - name: Go Vet
        run: go vet ./...

Thanks to Daniel Martí gofmt is super fast now Go #43566.

Check dependencies

I love to write dependency-free packages but sometimes I need something to import. So, the dependencies must be verified.

First, we need to check that go.mod is correct, download dependencies and verify go.sum:

      - name: Go Tidy
        run: go mod tidy && git diff --exit-code

      - name: Go Mod
        run: go mod download

      - name: Go Mod Verify
        run: go mod verify

You might think that go mod verify is an extra step, but this will help you to catch force pushes (or even supply chain attacks) in your dependencies.

I heavily recommend adding this to all your workflows.

Codegen & build

Maybe it's not the most popular feature in Go and there are different ways to manage this but I prefer and recommend you to commit generated files and always treat them as your code (well, it's yours anyway).

And let's build all packages and ignore the executables (if any, anyway we are not going to use them on CI):

      - name: Go Generate
        run: go generate ./... && git diff --exit-code

      - name: Go Build
        run: go build -o /dev/null ./...

Test

Okay, this part might sound overengineered but give me a moment to explain. Most of my projects are quite simple and don't require a database or any separate service to run, it's just a Go package that I use in other projects.

Doing just go test is exactly what I'm doing (ignore the flags and if: ..., I will explain them in a second):

      - name: Go Test
        if: ${{ !inputs.skipTests }}
        run: go test -v -count=1 -race -shuffle=on -coverprofile=coverage.txt ./...

But as for Redis client that I'm writing (cristalhq/redis), I need to setup Redis to test everything properly which requires additional changes to the workflow file. And I don't want that, I want to reuse as much as possible!

What I did: I added a boolean parameter to the GitHub Actions workflow named skipTests which is false by default (because most of the projects are simple).

But for the Redis client repository, I change it to true and disable tests. I'm running them separately in another workflow file called e2e.yml.

And when tests are disabled I'm only compiling them:

      - name: Go Compile Tests
        if: ${{ inputs.skipTests }}
        run: go test -exec /bin/true ./...

It's a hacky way to compile all packages and delete executables, see Go #15513.

To add a parameter to the workflow just do:

on:
  workflow_call:
    inputs:
      skipTests:
        description: 'Skip tests, useful when there is a dedicated CI job for tests'
        default: false
        required: false
        type: boolean

Test flags

Let's take a closer look at the go test flags:

-v
- I prefer verbose output where I can see everything
-count=1
- prevents caching go test results, which might be irrelevant for CI but I prefer to keep it
-race
- enable race detector, this increases build and run time but it helps to find concurrency bugs earlier, priceless.
-shuffle=on
- run tests in a different order to prevent implicit test order dependencies
- shameless plug: it was my proposal
-coverprofile=coverage.txt
- collect the coverage to upload it later
./...
- just test all the packages

Benchmark

Benchmarking on the CI in many cases is a bad idea due to noisy neighbours (read other processes on the machine) that will skew your results.

But a rare and bad thing: your benchmark might be broken and you might miss that. Well, this happened to me a few times.

To fix that we can run benchmarks once just to verify that they're still passing:

      - name: Go Benchmark
        run: go test -v -shuffle=on -run=- -bench=. -benchtime=1x ./...

-v & -shuffle=on
- you know that already
-run=-
- means do not run tests
-bench=.
- means run every benchmark
-benchtime=1x
- tells to run every benchmark once
./...
- do this in every package

I took this suggestion from a good Go issue "Go CI best practices" Go #42119. Again, thanks to Daniel Marti.

Coverage

In the previous step, we collected code coverage, better to keep it somewhere. I prefer Codecov but you might use any other:

      - name: Upload Coverage
        if: ${{ !inputs.skipTests }}  # upload when we really run our tests
        uses: codecov/codecov-action@v3
        continue-on-error: true  # we don't care if it fails
        with:
          token: ${{secrets.CODECOV_TOKEN}}  # set in repository settings
          file: ./coverage.txt  # file from the previous step
          fail_ci_if_error: false

And that's all! This is the build and/or test workflow that I use daily in 100+ Go repositories.

Vulncheck

Go team released govulncheck tool with Go 1.19. It's a simple and very fast tool to check "Does your code contain vulnerable lines?". Check the announcement for more info https://go.dev/blog/vuln.

And see vuln.yml workflow in cristalhq/.github.

Thanks to https://brandur.org/fragments/govulncheck-ci where I found this originally.

How it can be reused?

Quite easily. GitHub Actions allow us to reuse workflows. To use the described above workflow in any of my projects I need a few lines in .github/workflows/build.yml:

jobs:
  build:
    uses: cristalhq/.github/.github/workflows/build.yml@v0.4.0

  vuln:
    uses: cristalhq/.github/.github/workflows/vuln.yml@v0.4.0

(where cristalhq/.github is a repository for common stuff across the whole cristalhq organisation, like security policies, code of conduct, etc)

One of the projects that use this workflow cristalhq/jsn. See Reusing workflows
documentation for more.

Conclusions

Here is the full workflow: build.yml.

That's the basic CI for most of my Go projects. Many things can be added like fuzzing, automatic releases, linting, etc.

But in this post, I wanted to share the core component of my Go CI workflow.

Thanks.

Links

DEV / Lobsters / Medium / Reddit / Twitter