elastic / elastic/elastic-package
Gradually stricter configuration for golangci-lint
- Dominant language
- Go
- Stars
- 72
- Forks
- 141
- Avg merge
- 19h 42m
- Merged PRs (30d)
- 55
Description
This issue tracks the TODOs introduced when setting up `golangci-lint` in https://github.com/elastic/elastic-package/pull/3440.
Each item represents a known gap that was suppressed for now to keep `make lint` green while the codebase is incrementally improved.
## Complexity thresholds
These thresholds were raised above the desired targets to make lint pass. They should be lowered as the flagged functions are refactored.
- [ ] **`gocyclo`**: current threshold is 51, target is 25. Worst offenders: `(*tester).prepareScenario` and `Run` in `internal/testrunner/script/script.go` (complexity 50).
- https://github.com/elastic/elastic-package/pull/3510
- [ ] **`gocognit`**: current threshold is 70, target is 40.
- https://github.com/elastic/elastic-package/pull/3510
## Function argument limits (`revive`)
The `argument-limit` rule is set to 11 instead of the default 8. The following functions should be refactored to use option structs:
- [ ] `BuildIntegrationPackagePolicy` in `internal/kibana/packagepolicy.go` (11 args)
- [ ] `CreatePackagePolicy` in `internal/testrunner/runners/system/tester.go` (9 args)
- [ ] `processResults` in `cmd/testrunner.go` (9 args)
## MapStr typed API (`errcheck`)
- [ ] `common.MapStr.Put` and `common.MapStr.Delete` return errors only when dot-notation path traversal fails on a non-map intermediate value. All current callsites use simple string literal keys and could use direct map operations (`m[key] = value`, `delete(m, key)`), but we want to keep the possibility of using dot-notation.
Affected files:
- `internal/builder/dashboards.go`
- `internal/export/transform_decode.go`
- `internal/fields/dependency_manager.go`
- `internal/testrunner/runners/policy/policy.go`
## testscript interface signature (`unparam`)
- [ ] Testscript command handlers in `internal/testrunner/script/` must match the `testscript.Cmd` signature `func(ts *testscript.TestScript, neg bool, args []string)`. The `neg bool` parameter is required by the interface even when unused. Consider upstreaming a type alias or interface annotation to make this explicit without suppression.
## Errors linting (`errorlint`)
- [x] Enabling it detects some comparisons that should use `errors.Is`/`errors.As`, as well as some wrapping mistakes.
- https://github.com/elastic/elastic-package/pull/3522
## Additional nitpicking (`gocritic`)
- [x] `appendAssign` slice don't assigned after append, likely a source of bugs as the slice is lost if resized.
- https://github.com/elastic/elastic-package/pull/3522
- [x] `offBy1` issues when negative indexes are not checked.
- https://github.com/elastic/elastic-package/pull/3522
- [x] `exitAfterDefer` can detect actual issues on cleanups, there is at least one occurence.
- https://github.com/elastic/elastic-package/pull/3923
- [ ] `elseIf`/`singleCaseSwitch` linting on when to use ifs vs. switch.
- [ ] `badCall` suspicious calls to functions with variable numbers of arguments, such as `filepath.Join`.
- [ ] `captLocal` local variables with capitalized first letter.
- [ ] ...
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.