From 518cf6e6f810f96aac355c27824208d453804ce6 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Tue, 8 Sep 2026 09:02:37 -0700 Subject: [PATCH] ci: replace the vet and gofmt steps with a golangci-lint job CI ran `gofmt -l apps` and `go vet ./...`. golangci-lint's default set includes govet, and its gofmt formatter reports formatting, so both steps are redundant and are removed; lint is one job, checked once. The formatting check also gets wider. `gofmt -l apps` looked at two of the repo's nineteen packages; cli/, test/ and tools/ were never checked. The linter covers all of them, and names the `acceptance` build tag so those files are loaded too. One real bug fixed. tarGzDir deferred Close on the tar writer, the gzip writer and the file, discarding all three errors. The tar trailer and gzip footer are written during those Closes, after the return value is set, so a failure there produced a truncated archive reported as a successful export. Closes now run innermost-first through a named return that keeps the first error. The other 30 unchecked errors were already benign and are now explicit: read handles, error-path cleanup where a failure is already being returned, and SSH teardown. errcheck excludes fmt.Fprint and deferred os.RemoveAll of a temp dir alongside the existing entries. ST1005 is off, with the reason in the config: it wants error strings without trailing punctuation, which fits errors meant to be wrapped, not a CLI's top-level messages written as full sentences on purpose. The staticcheck `checks` list repeats golangci-lint's own default exclusions, because naming only "all" silently re-enables ST1000/ST1003/ST1016/ST1020/ST1021/ST1022 and buries real findings in doc-comment noise. Three golangci-lint defaults that drop findings are disabled: max-issues-per-linter, max-same-issues and uniq-by-line. Signed-off-by: Tim Smith --- .github/workflows/test.yml | 34 ++++++++----- .golangci.yml | 79 +++++++++++++++++++++++++++++ Makefile | 8 ++- apps/cinc/cmd/keys.go | 4 +- cli/cookbook/cookbook.go | 4 +- cli/cookbook/extract.go | 4 +- cli/explore/actions_test.go | 6 +-- cli/explore/search_test.go | 4 +- cli/nodeedit/convert_test.go | 3 +- cli/policyfile/archive.go | 6 +-- cli/policyfile/export.go | 22 +++++--- cli/policyfile/extract.go | 4 +- cli/policyfile/rubyeval/loader.go | 6 +-- cli/policyfile/rubyeval/rubyeval.go | 2 +- cli/remote/remote.go | 6 +-- 15 files changed, 147 insertions(+), 45 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index eaa686c..aab67cc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,7 +10,7 @@ permissions: jobs: test: - name: Unit tests, vet & build + name: Unit tests & build runs-on: ubuntu-latest steps: - name: Check out code @@ -22,18 +22,6 @@ jobs: go-version-file: go.mod cache: true - - name: Check formatting - run: | - unformatted=$(gofmt -l apps) - if [ -n "$unformatted" ]; then - echo "These files are not gofmt-formatted:" - echo "$unformatted" - exit 1 - fi - - - name: Vet - run: go vet ./... - - name: Install gotestsum run: go install gotest.tools/gotestsum@v1.13.0 @@ -45,3 +33,23 @@ jobs: - name: Build run: go build ./apps/cinc + + lint: + name: Lint + runs-on: ubuntu-latest + steps: + - name: Check out code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + cache: true + + - name: golangci-lint + uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 + with: + # Pinned so a new linter release cannot turn a green branch red + # without a commit. + version: v2.13.2 diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..8808fba --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,79 @@ +version: "2" + +run: + # test/acceptance sits behind a build tag; without naming it here the linter + # never loads those files. (The race/!race pair in cli/policyfile/rubyeval + # is inherently one-or-the-other and follows the default build, as `go vet` + # did.) + build-tags: + - acceptance + +# The default set: errcheck, govet, ineffassign, staticcheck, unused. +# govet makes a separate `go vet` step redundant, and the gofmt formatter +# below makes a separate `gofmt -l` step redundant, so CI runs this alone. +linters: + default: standard + + settings: + staticcheck: + # ST1005 wants error strings without trailing punctuation. That fits + # errors meant to be wrapped; these are a CLI's top-level messages, + # written as full sentences on purpose ("can't find your client key at + # %s — it's set as client_key in %s. Create the key file, or ..."). The + # style is deliberate and documented at each site, so the check is off + # rather than the messages flattened. + # This repeats golangci-lint's own default exclusions; listing only + # "all" and -ST1005 would silently re-enable ST1000/ST1003/ST1016/ + # ST1020/ST1021/ST1022 and bury the real findings in doc-comment noise. + checks: + - all + - -ST1000 + - -ST1003 + - -ST1016 + - -ST1020 + - -ST1021 + - -ST1022 + - -ST1005 + + errcheck: + # Errors that are genuinely nothing to act on. Everything else must be + # handled, including the ones that look boring. + exclude-functions: + # Deferred close on a read-only handle: the error reports a flush + # failure there is no write to lose. + - (io.Closer).Close + - (*database/sql.Rows).Close + # Rollback after a committed or already-failed transaction returns + # sql.ErrTxDone by design; the deferred call is the safety net. + - (*database/sql.Tx).Rollback + # Draining a response body to make the connection reusable, and + # writing usage text to stderr. A failure changes nothing we can do. + - io.Copy + - fmt.Fprintf + - fmt.Fprintln + - fmt.Fprint + # Deferred cleanup of a temp dir the process is done with. + - os.RemoveAll + + exclusions: + rules: + # Test helpers close temp handles and ignore returns on purpose; the + # test failing IS the error handling. + - path: _test\.go + linters: [errcheck] + +formatters: + # Reported as issues by `golangci-lint run`, so this replaces a standalone + # gofmt check. Applied (rather than reported) by `golangci-lint fmt`. + enable: + - gofmt + +issues: + # Defaults are max-issues-per-linter: 50 and max-same-issues: 3, which + # truncate repeated findings — fix three and three more appear. A blocking + # gate has to report everything at once. + max-issues-per-linter: 0 + max-same-issues: 0 + # Also on by default: only the first issue on any given line is reported. + # Two distinct problems on one line means one of them is silently dropped. + uniq-by-line: false diff --git a/Makefile b/Makefile index 03efed6..19ed680 100644 --- a/Makefile +++ b/Makefile @@ -15,7 +15,7 @@ LDFLAGS := -X $(LDFLAGS_PKG).version=$(VERSION) \ -X $(LDFLAGS_PKG).commit=$(COMMIT) \ -X $(LDFLAGS_PKG).buildDate=$(BUILD_DATE) -.PHONY: all build dist install test test-acceptance vet fmt tidy clean run docs help +.PHONY: all build dist install test test-acceptance vet lint fmt tidy clean run docs help all: build @@ -59,6 +59,12 @@ test-acceptance: vet: go vet ./... +## lint: run golangci-lint (config in .golangci.yml). Subsumes vet and a gofmt +## check, and unlike `gofmt -l apps` it covers every package. +## Install: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.13.2 +lint: + golangci-lint run ./... + ## fmt: format all Go source fmt: gofmt -w apps diff --git a/apps/cinc/cmd/keys.go b/apps/cinc/cmd/keys.go index 92b10f1..b7384b3 100644 --- a/apps/cinc/cmd/keys.go +++ b/apps/cinc/cmd/keys.go @@ -274,11 +274,11 @@ func writePrivateKey(out io.Writer, priv, keyFile, fileMsg string) error { return fmt.Errorf("cinc: write key file: %w", err) } if err := f.Chmod(0o600); err != nil { - f.Close() + _ = f.Close() // already returning an error return fmt.Errorf("cinc: write key file: %w", err) } if _, err := f.WriteString(priv); err != nil { - f.Close() + _ = f.Close() // already returning an error return fmt.Errorf("cinc: write key file: %w", err) } if err := f.Close(); err != nil { diff --git a/cli/cookbook/cookbook.go b/cli/cookbook/cookbook.go index 84099d6..b84009a 100644 --- a/cli/cookbook/cookbook.go +++ b/cli/cookbook/cookbook.go @@ -570,7 +570,7 @@ func writeFileToTar(tw *tar.Writer, dir, entryPath string) error { if err != nil { return fmt.Errorf("open %s: %w", entryPath, err) } - defer f.Close() + defer func() { _ = f.Close() }() // read handle if _, err := io.Copy(tw, f); err != nil { return fmt.Errorf("write tar entry %s: %w", entryPath, err) } @@ -745,7 +745,7 @@ func ExtractArchiveFiles(data []byte) ([]string, error) { if err != nil { return nil, err } - defer gz.Close() + defer func() { _ = gz.Close() }() // read handle tr := tar.NewReader(gz) var files []string for { diff --git a/cli/cookbook/extract.go b/cli/cookbook/extract.go index 5c848e5..03db84b 100644 --- a/cli/cookbook/extract.go +++ b/cli/cookbook/extract.go @@ -37,7 +37,7 @@ func ExtractArchive(r io.Reader, destDir string) (string, error) { if err != nil { return "", fmt.Errorf("open gzip: %w", err) } - defer gz.Close() + defer func() { _ = gz.Close() }() // read handle tr := tar.NewReader(gz) roots := map[string]struct{}{} @@ -119,7 +119,7 @@ func writeFile(r io.Reader, target, name string, total *int64) error { return fmt.Errorf("create %s: %w", target, err) } if err := boundedCopy(f, r, name, total); err != nil { - f.Close() + _ = f.Close() // already returning an error return fmt.Errorf("write %s: %w", target, err) } if err := f.Close(); err != nil { diff --git a/cli/explore/actions_test.go b/cli/explore/actions_test.go index fbaccef..5cbc69b 100644 --- a/cli/explore/actions_test.go +++ b/cli/explore/actions_test.go @@ -39,7 +39,7 @@ func TestEditFlowSavesEditedObject(t *testing.T) { // e starts an edit of the selected node; the seed is fetched async. m, cmd = pressRune(t, m, 'e') - m, cmd = step(t, m, drain(cmd)) // editSeedMsg → editor opens + m, _ = step(t, m, drain(cmd)) // editSeedMsg → editor opens if m.screen != screenEditor { t.Fatalf("screen = %v, want editor", m.screen) } @@ -74,11 +74,11 @@ func TestCreateWithSecretShowsResultModal(t *testing.T) { m.cur = newUserKind() m.screen = screenList - m, cmd := m2(m.startCreate()) // n: Creatable → editor with template + m, _ = m2(m.startCreate()) // n: Creatable → editor with template if m.screen != screenEditor { t.Fatalf("screen = %v, want editor", m.screen) } - m, cmd = commitEditor(t, m) + m, cmd := commitEditor(t, m) m, _ = step(t, m, drain(cmd)) // mutationDoneMsg with secret if m.screen != screenResult { diff --git a/cli/explore/search_test.go b/cli/explore/search_test.go index 5c80d52..1eed894 100644 --- a/cli/explore/search_test.go +++ b/cli/explore/search_test.go @@ -55,7 +55,7 @@ func TestSearchNarrowsListToServerMatches(t *testing.T) { m, _ := openNodes(t, searchMux(t)) // Open the search modal; it defaults to the current list's index. - m, cmd := pressRune(t, m, 's') + m, _ = pressRune(t, m, 's') if m.screen != screenSearch { t.Fatalf("screen = %v, want search", m.screen) } @@ -65,7 +65,7 @@ func TestSearchNarrowsListToServerMatches(t *testing.T) { // Type a query and run it. m, _ = step(t, m, keyRunes("role:web")) - m, cmd = pressKey(t, m, 13) // enter + m, cmd := pressKey(t, m, 13) // enter msg := drain(cmd) sl, ok := msg.(searchLoadedMsg) if !ok { diff --git a/cli/nodeedit/convert_test.go b/cli/nodeedit/convert_test.go index 70f8677..e3757ea 100644 --- a/cli/nodeedit/convert_test.go +++ b/cli/nodeedit/convert_test.go @@ -39,7 +39,8 @@ func TestAttributesSeedShowsEditableBagsInOrderAndExcludesAutomatic(t *testing.T iNormal := strings.Index(s, "normal") iDefault := strings.Index(s, "default") iOverride := strings.Index(s, "override") - if !(iNormal >= 0 && iNormal < iDefault && iDefault < iOverride) { + ordered := iNormal >= 0 && iNormal < iDefault && iDefault < iOverride + if !ordered { t.Errorf("editable bags out of order in seed:\n%s", s) } if strings.Contains(s, "automatic") { diff --git a/cli/policyfile/archive.go b/cli/policyfile/archive.go index c7349e1..7c111e9 100644 --- a/cli/policyfile/archive.go +++ b/cli/policyfile/archive.go @@ -86,12 +86,12 @@ func extractBundleTarball(archivePath, dest string) error { if err != nil { return err } - defer f.Close() + defer func() { _ = f.Close() }() // read handle gz, err := gzip.NewReader(f) if err != nil { return fmt.Errorf("policyfile: open archive %s: %w", archivePath, err) } - defer gz.Close() + defer func() { _ = gz.Close() }() // read handle tr := tar.NewReader(gz) var total int64 @@ -122,7 +122,7 @@ func extractBundleTarball(archivePath, dest string) error { return err } if err := boundedCopy(out, tr, hdr.Name, &total); err != nil { - out.Close() + _ = out.Close() // already returning an error return fmt.Errorf("policyfile: %w", err) } if err := out.Close(); err != nil { diff --git a/cli/policyfile/export.go b/cli/policyfile/export.go index f12475c..84464ac 100644 --- a/cli/policyfile/export.go +++ b/cli/policyfile/export.go @@ -129,14 +129,14 @@ func copyTree(src, dst string) error { if err != nil { return err } - defer in.Close() + defer func() { _ = in.Close() }() // read handle // Clamp to a safe mode rather than trusting the source file's bits. out, err := os.OpenFile(target, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode) if err != nil { return err } if _, err := io.Copy(out, in); err != nil { - out.Close() + _ = out.Close() // already returning an error return err } return out.Close() @@ -145,16 +145,24 @@ func copyTree(src, dst string) error { // tarGzDir writes dir as a gzip-compressed tarball at archivePath, with entries // rooted at the directory's base name. -func tarGzDir(dir, archivePath string) error { +func tarGzDir(dir, archivePath string) (err error) { f, err := os.Create(archivePath) if err != nil { return err } - defer f.Close() gz := gzip.NewWriter(f) - defer gz.Close() tw := tar.NewWriter(gz) - defer tw.Close() + // The tar trailer and the gzip footer are written by Close, so a failure + // there means a truncated archive. Closing innermost first and keeping the + // first error is what makes that surface instead of being reported as a + // successful export. + defer func() { + for _, c := range []io.Closer{tw, gz, f} { + if cerr := c.Close(); err == nil { + err = cerr + } + } + }() root := filepath.Base(dir) return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { @@ -183,7 +191,7 @@ func tarGzDir(dir, archivePath string) error { if err != nil { return err } - defer in.Close() + defer func() { _ = in.Close() }() // read handle _, err = io.Copy(tw, in) return err }) diff --git a/cli/policyfile/extract.go b/cli/policyfile/extract.go index 859fa2a..62622a2 100644 --- a/cli/policyfile/extract.go +++ b/cli/policyfile/extract.go @@ -64,7 +64,7 @@ func extractCookbookTarball(r io.Reader, dest string) error { if err != nil { return fmt.Errorf("supermarket: open gzip: %w", err) } - defer gz.Close() + defer func() { _ = gz.Close() }() // read handle tr := tar.NewReader(gz) var total int64 @@ -98,7 +98,7 @@ func extractCookbookTarball(r io.Reader, dest string) error { return err } if err := boundedCopy(f, tr, hdr.Name, &total); err != nil { - f.Close() + _ = f.Close() // already returning an error return fmt.Errorf("supermarket: %w", err) } if err := f.Close(); err != nil { diff --git a/cli/policyfile/rubyeval/loader.go b/cli/policyfile/rubyeval/loader.go index 4c4da4d..3b35a82 100644 --- a/cli/policyfile/rubyeval/loader.go +++ b/cli/policyfile/rubyeval/loader.go @@ -191,7 +191,7 @@ func verifyFileSHA256(path, wantHex string) error { if err != nil { return err } - defer f.Close() + defer func() { _ = f.Close() }() // read handle h := sha256.New() if _, err := io.Copy(h, f); err != nil { return err @@ -210,7 +210,7 @@ func extractTarGz(archive []byte, dest string) error { if err != nil { return err } - defer gz.Close() + defer func() { _ = gz.Close() }() // read handle tr := tar.NewReader(gz) for { hdr, err := tr.Next() @@ -238,7 +238,7 @@ func extractTarGz(archive []byte, dest string) error { return err } if _, err := io.Copy(f, tr); err != nil { //nolint:gosec // pinned, checksum-verified archive - f.Close() + _ = f.Close() // already returning an error return err } if err := f.Close(); err != nil { diff --git a/cli/policyfile/rubyeval/rubyeval.go b/cli/policyfile/rubyeval/rubyeval.go index 7571bc1..de208ca 100644 --- a/cli/policyfile/rubyeval/rubyeval.go +++ b/cli/policyfile/rubyeval/rubyeval.go @@ -324,7 +324,7 @@ func readCapped(path string, max int64) ([]byte, error) { if err != nil { return nil, err } - defer f.Close() + defer func() { _ = f.Close() }() // read handle if info, err := f.Stat(); err == nil && info.Size() > max { return nil, fmt.Errorf("policyfile: engine result is %d bytes, over the %d-byte cap", info.Size(), max) } diff --git a/cli/remote/remote.go b/cli/remote/remote.go index 5457a62..2661d9d 100644 --- a/cli/remote/remote.go +++ b/cli/remote/remote.go @@ -81,7 +81,7 @@ func (NativeRunner) Run(ctx context.Context, target Target, command string, opts result.Error = fmt.Sprintf("dial ssh: %v", err) return result } - defer conn.Close() + defer func() { _ = conn.Close() }() sshConn, chans, reqs, err := ssh.NewClientConn(conn, host, config) if err != nil { result.ExitCode = 255 @@ -89,14 +89,14 @@ func (NativeRunner) Run(ctx context.Context, target Target, command string, opts return result } client := ssh.NewClient(sshConn, chans, reqs) - defer client.Close() + defer func() { _ = client.Close() }() session, err := client.NewSession() if err != nil { result.ExitCode = 255 result.Error = fmt.Sprintf("new ssh session: %v", err) return result } - defer session.Close() + defer func() { _ = session.Close() }() var stdout, stderr bytes.Buffer session.Stdout = &stdout session.Stderr = &stderr