Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid config file silently ignored #5313

Closed
6 of 7 tasks
alexbozhenko opened this issue Jan 10, 2025 · 11 comments
Closed
6 of 7 tasks

Invalid config file silently ignored #5313

alexbozhenko opened this issue Jan 10, 2025 · 11 comments
Labels
question Further information is requested

Comments

@alexbozhenko
Copy link

alexbozhenko commented Jan 10, 2025

Welcome

  • Yes, I'm using a binary release within 2 latest releases. Only such installations are supported.
  • Yes, I've searched similar issues on GitHub and didn't find any.
  • Yes, I've read the typecheck section of the FAQ.
  • Yes, I've tried with the standalone linter if available (e.g., gocritic, go vet, etc.).
  • I agree to follow this project's Code of Conduct

Description of the problem

If golangci-lint found a config file, but the file is invalid, it still runs...

Version of golangci-lint

# golangci-lint --version
golangci-lint has version 1.63.4 built with go1.23.4 from c1149695 on 2025-01-03T19:49:42Z

Configuration

https://github.com/golangci/golangci-lint/blob/master/.golangci.yml

Go environment

# go version && go env
go version go1.23.4 linux/amd64
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/alex/.cache/go-build'
GOENV='/home/alex/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/alex/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/alex/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.4'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/alex/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/alex/code/golangci-lint/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2325651328=/tmp/go-build -gno-record-gcc-switches'

Verbose output of running

See below

A minimal reproducible example or link to a public repository

  1. Clone this repo, and run the program. Observer that 36 configured linters are running.
git clone git@github.com:golangci/golangci-lint.git
# golangci-lint run -v
INFO golangci-lint has version 1.63.4 built with go1.23.4 from c1149695 on 2025-01-03T19:49:42Z
INFO [config_reader] Config search paths: [./ /home/alex/code/golangci-lint /home/alex/code /home/alex /home /]
INFO [config_reader] Used config file .golangci.yml
INFO [lintersdb] Active 36 linters: [bodyclose copyloopvar depguard dogsled dupl errcheck errorlint funlen gocheckcompilerdirectives gochecknoinits goconst gocritic gocyclo godox gofmt goimports goprintffuncname gosec gosimple govet ineffassign intrange lll misspell mnd nakedret noctx nolintlint revive staticcheck stylecheck testifylint unconvert unparam unused whitespace]
INFO [loader] Go packages loading at mode 8767 (compiled_files|deps|name|exports_file|files|imports|types_sizes) took 7.202027354s
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 21.003626ms
INFO [linters_context/goanalysis] analyzers took 6m40.091231604s with top 10 stages: gocritic: 4m23.411923157s, buildir: 1m15.969914503s, goimports: 6.882139897s, dupl: 3.089463934s, the_only_name: 2.867083359s, unconvert: 2.782334863s, fact_deprecated: 2.303318184s, inspect: 2.182849466s, printf: 1.965027876s, ctrlflow: 1.844130118s
INFO [runner/skip_dirs] Skipped 2 issues from dir internal/go/mmap by pattern internal/go
INFO [runner/skip_dirs] Skipped 2 issues from dir internal/x/tools/analysisinternal by pattern internal/x
INFO [runner/skip_dirs] Skipped 4 issues from dir internal/x/tools/analysisflags by pattern internal/x
INFO [runner/skip_dirs] Skipped 3 issues from dir internal/go/quoted by pattern internal/go
INFO [runner/skip_dirs] Skipped 1 issues from dir test/testdata_etc/unused_exported/lib by pattern test/testdata_etc
INFO [runner/skip_dirs] Skipped 112 issues from dir internal/go/cache by pattern internal/go
INFO [runner/skip_dirs] Skipped 11 issues from dir internal/x/tools/diff by pattern internal/x
INFO [runner/skip_dirs] Skipped 63 issues from dir internal/x/tools/diff/lcs by pattern internal/x
INFO [runner/skip_dirs] Skipped 1 issues from dir internal/x/tools/diff/myers by pattern internal/x
INFO [runner/skip_dirs] Skipped 2 issues from dir internal/go/robustio by pattern internal/go
INFO [runner/skip_dirs] Skipped 3 issues from dir internal/go/testenv by pattern internal/go
INFO [runner/skip_dirs] Skipped 3 issues from dir test/testdata_etc/abspath by pattern test/testdata_etc
INFO [runner] Issues before processing: 584, after processing: 0
INFO [runner] Processors filtering stat (in/out): skip_files: 584/581, autogenerated_exclude: 374/374, invalid_issue: 584/584, identifier_marker: 374/374, exclude: 374/374, exclude-rules: 374/52, filename_unadjuster: 584/584, skip_dirs: 581/374, path_prettifier: 584/584, nolint: 52/0, cgo: 584/584
INFO [runner] processing took 51.471369ms with stages: exclude-rules: 14.172326ms, identifier_marker: 13.130339ms, path_prettifier: 10.613007ms, autogenerated_exclude: 6.04942ms, nolint: 3.850246ms, skip_dirs: 3.381996ms, skip_files: 97.102µs, invalid_issue: 79.501µs, cgo: 68.418µs, filename_unadjuster: 26.299µs, max_same_issues: 871ns, exclude: 331ns, uniq_by_line: 261ns, sort_results: 260ns, fixer: 260ns, diff: 180ns, max_from_linter: 141ns, source_code: 100ns, max_per_file_from_linter: 91ns, path_shortener: 80ns, severity-rules: 70ns, path_prefixer: 70ns
INFO [runner] linters took 29.455744695s with stages: goanalysis_metalinter: 29.404164582s
INFO File cache stats: 456 entries of total size 983.0KiB
INFO Memory: 360 samples, avg is 1897.8MB, max is 3871.1MB
INFO Execution took 36.689502277s
  1. Brake the config file, e.g. by making a typo:
sed -i 's/^linters:/lintersS:/' .golangci.yml
  1. Now config file is invalid:
# golangci-lint config verify
jsonschema: "" does not validate with "/additionalProperties": additional properties 'lintersS' not allowed
Failed executing command with error: the configuration contains invalid elements

But we ignore it and fall back to the default config(?), run 6 linters, and still exit with code 0.

# golangci-lint run -v
INFO golangci-lint has version 1.63.4 built with go1.23.4 from c1149695 on 2025-01-03T19:49:42Z
INFO [config_reader] Config search paths: [./ /home/alex/code/golangci-lint /home/alex/code /home/alex /home /]
INFO [config_reader] Used config file .golangci.yml
INFO [lintersdb] Active 6 linters: [errcheck gosimple govet ineffassign staticcheck unused]
INFO [loader] Go packages loading at mode 8767 (types_sizes|exports_file|files|name|compiled_files|deps|imports) took 877.416252ms
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 25.505434ms
INFO [linters_context/goanalysis] analyzers took 38.448907595s with top 10 stages: buildir: 6.745488935s, buildssa: 885.610902ms, fact_deprecated: 714.254078ms, isgenerated: 662.182768ms, unused: 618.309336ms, S1038: 603.313363ms, ineffassign: 587.553127ms, inspect: 583.931581ms, errcheck: 516.228293ms, ctrlflow: 506.429901ms
INFO [runner/skip_dirs] Skipped 10 issues from dir internal/x/tools/diff/lcs by pattern internal/x
INFO [runner/skip_dirs] Skipped 46 issues from dir internal/go/cache by pattern internal/go
INFO [runner/skip_dirs] Skipped 1 issues from dir internal/go/testenv by pattern internal/go
INFO [runner/skip_dirs] Skipped 1 issues from dir test/testdata_etc/abspath by pattern test/testdata_etc
INFO [runner] Issues before processing: 85, after processing: 0
INFO [runner] Processors filtering stat (in/out): filename_unadjuster: 85/85, skip_dirs: 85/27, exclude-rules: 27/0, invalid_issue: 85/85, autogenerated_exclude: 27/27, identifier_marker: 27/27, cgo: 85/85, path_prettifier: 85/85, skip_files: 85/85, exclude: 27/27
INFO [runner] processing took 1.483764ms with stages: path_prettifier: 580.099µs, identifier_marker: 350.958µs, autogenerated_exclude: 190.408µs, exclude-rules: 179.838µs, skip_dirs: 148.799µs, invalid_issue: 10.449µs, cgo: 10.179µs, skip_files: 5.621µs, filename_unadjuster: 4.277µs, nolint: 742ns, max_same_issues: 632ns, fixer: 351ns, uniq_by_line: 230ns, exclude: 200ns, path_shortener: 190ns, max_from_linter: 181ns, sort_results: 170ns, diff: 120ns, path_prefixer: 90ns, source_code: 90ns, max_per_file_from_linter: 80ns, severity-rules: 60ns
INFO [runner] linters took 1.036395747s with stages: goanalysis_metalinter: 1.034854395s
INFO File cache stats: 0 entries of total size 0B
INFO Memory: 21 samples, avg is 219.3MB, max is 621.6MB
INFO Execution took 1.949103553s

I feel like this behavior violates https://en.wikipedia.org/wiki/Principle_of_least_astonishment.
Clearly, since the config file is found, the user wanted it to be there and to be used, but messed up.
We should let the user know about it, probably by exiting non-zero, so it won't be silently ignored in ci.

Validation

  • Yes, I've included all information above (version, config, etc.).

Supporter

@alexbozhenko alexbozhenko added the bug Something isn't working label Jan 10, 2025
@alexbozhenko
Copy link
Author

You could argure that one should run config verify before running golangci-lint run, but in in IDEs (e.g. vscode), the config still will be silently ignored, and I gues there is no workaround there?

@ldez ldez added question Further information is requested and removed bug Something isn't working labels Jan 10, 2025
@ldez
Copy link
Member

ldez commented Jan 10, 2025

Ignoring invalid elements is a choice: it allows to not break with deprecated and removed elements.

Most IDEs are connected to the JSONSchema store and can validate the configuration.

@ldez ldez closed this as completed Jan 10, 2025
@alexbozhenko
Copy link
Author

@ldez would it be possible to add an option for strict config parsing?
That would allow https://github.com/golangci/golangci-lint-action to enable it.
Would you agree that it is a valid desire of some users to not run with invalid config?

@ldez
Copy link
Member

ldez commented Jan 10, 2025

I don't want to do that for different reasons, one is related to the work on v2 and the kind of option will be a problem for the migration.

We already have a command to verify the configuration and most of the IDEs check the JSONSchema automatically.

FYI, your PR will never work as you think because we hacked the configuration parsing, it will just break the parsing of some elements.

@alexbozhenko
Copy link
Author

alexbozhenko commented Jan 10, 2025

@ldez may I at least document this behavior somewhere? Either here, or in gh action repo?
I mean the fact that if you want to make sure you are having valid configuration, you run
golangci-lint config verify first?

@alexbozhenko
Copy link
Author

Interestingly, not that many people do that:
https://github.com/search?q=%22golangci-lint+config+verify%22&type=code

I wonder, out of tens of thousands of config files, how many are silently ignored because of a typo...
https://github.com/search?q=path%3A.golangci.y&type=code&p=1

@ldez
Copy link
Member

ldez commented Jan 10, 2025

I will not participate in this sterile discussion: I was clear about the topic.

You can disagree, and we can agree to disagree.

@alexbozhenko
Copy link
Author

@ldez
I am not arguing with you.
I only asked if you would be open to documenting the behavior, either here or in GitHub actions repo? That would potentially save someone time and prevent undetected bugs in the future...

If you are open to documentation update, I will submit one.

@ldez
Copy link
Member

ldez commented Jan 11, 2025

Before this message and this one, I was open to thinking about a place inside the doc.

Added to the fact that you ping me on every message, now, I'm just upset, so I will keep your suggestion for later.

@alexbozhenko
Copy link
Author

I do not have an idea that you considered tagging a rude thing.
And I just sincerely want to make the great golangci-lint better, that's why I tried to find out if people already run config verify manually. The second message I posted by mistake and edited it after that.

Ok, let me know if you think it should be documented at least somewhere...

Another point is: if there is an empty .golanci.yml config file somewhere along the search path(described at https://golangci-lint.run/usage/configuration/#config-file), that empty file will be used. Probably not users would expect too.

@golangci golangci locked as resolved and limited conversation to collaborators Jan 11, 2025
@golangci golangci unlocked this conversation Jan 11, 2025
@bombsimon
Copy link
Member

To add to the context and give an example about why invalid config can be useful, gclv optionally uses the configuration file to pin version of golangci-lint and install and use that version automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants