elastic / elastic/elastic-package

Gradually stricter configuration for golangci-lint

Open
#3,443 0 comments 0 reactions 0 assignees View on GitHub
Team:Ecosystem technnical debt
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.