Golangci-lint: Support --disable-default-skip-dirs

[ntindall]

We are using golangci-lint to lint our ~320k LOC go monorepo. We have some logic set up to lint files in the monorepo that change on a given diff. You can imagine it does something to the effect of git diff in order to compute a list of changed packages, and then passes these to golang-ci-lint.

But there are some packages that we do not want to lint, e.g. for historical / legacy / transition reasons. Using the new option is not a solution to our use cases in CI. Hence why we have some custom tooling around this.

https://github.com/golangci/golangci-lint/blob/658f4addbdc61ec1518bf0cffc8a819ceef714f0/pkg/result/processors/skip_dirs.go#L85-L90

The current behavior of skip-dirs is to _not skip_ any explicitly specified package as a command line argument that conflicts with the skip-dirs configuration.

The following is a minimal reproducible case:

bin/golangci-lint run lib/zoom

$ cat .golangci.yml 
# options for analysis running
run:
  skip-dirs:
  - (^|/)lib/zoom($|/)

# rest of the config file is not relevant

Silently linting the directory even though it is explicitly skipped in the configuration is very surprising behavior. At minimum, I would expect a warning to be printed when this case is encountered, since it seems to indicate a configuration error.

Our hack around this for now is to put the excluded packages into the skip-files option, which does not suffer from this unexpected behavior. An alternative would be to prevent the directories from being passed into the linter... but that seems to bring into question the point of this configuration option at all...

@jirfag I am wondering why this behavior is the desired default. Is this something we can change? If not, would strongly advocate for a warning to be printed on this code path.

[jirfag]

hi, thank you for reporting!
This behavior was looking good for me but I couldn't imagine your use case. Also, it helped in our tests. Not having a warning is a bug. But I think we should completely remove these checks and make an option for not using default skip dirs list (testdata, etc).

[ntindall]

Thanks @jirfag - just to be clear, you're proposing some sort of --disable-skip-dirs command line argument that would cause skip-dirs config option to not be respected? And when skip-dirs is present in the configuration, the directory _would_ be skipped unless this option is supplied on the command line?

Not sure I totally understand what you mean by "our tests" - do you mean the unit tests for this package or something else?

Once we agree on the right pathway forward I am happy to help drive the solution, if needed.

[jirfag]

Yes, something like --disable-default-skip-dirs option, which will disable default dirs to skip (testdata, vendor, etc).
We store almost all test files in directory test/testdata and run tests like golangci-lint run ./test/testdata/specific_test.go. We can't store files outside of testdata dir because some files there are specially with compilation errors. If we will always skip all issues inside testdata - these tests won't work. Therefore we need to use option --disable-default-skip-dirs in all test runs.

[nathanhruby]

Another use case: We have a number of things in a builtin directory tree which are features that are built-in to the app versus features from plugins. This tree is being skipped when using run ./... because of the defaults but we would like it to be checked.

[AlekSi]

Just hit this issue with /builtin/. It was driving me crazy for some time.

[AlekSi]

Also, the documentation says that builtin$ is skipped. In practice, all packages with /builtin/ are skipped. What is the reason for skipping this pattern?

[jirfag]

hi,
it's already done: see skip-dirs-use-default. I've added it to README in #820