Skip to content

Lint and dead-code cleanup - #279

Merged
andrew merged 3 commits into
mainfrom
andrew/cleanup
Aug 21, 2026
Merged

Lint and dead-code cleanup#279
andrew merged 3 commits into
mainfrom
andrew/cleanup

Conversation

@andrew

@andrew andrew commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Three commits, reviewable independently.

Bump go tool golangci-lint to v2.13.1. .golangci.yml sets goconst.ignore-tests: true, but that setting was only added in v2.12.0 (golangci/golangci-lint#6480). On the previously pinned v2.10.1, go tool golangci-lint config verify fails with additional properties 'ignore-tests' not allowed and run silently ignores the key, so goconst counts test-file string literals toward min-occurrences. The bump pulls transitive tool-dep updates (x/tools, honnef.co, mvdan.cc) into go.sum.

Mechanical lint fixes. gofmt -w internal/server/health_test.go, and HasSuffix+TrimSuffixCutSuffix in config.ParseSize.

Remove dead code. deadcode ./... on main flags 26 symbols; after this it flags only metrics.UpdateCircuitBreakerState / RecordCircuitBreakerTrip, which #275 wires. Removed:

  • internal/storage/filesystem.go — superseded by OpenBucket("file://…") via gocloud.dev/blob/fileblob. The three test call sites (server_test.go, eviction_test.go) are migrated to storage.OpenBucket and their StorageConfig switched from the deprecated Path field to URL, which also clears the three SA1019 findings.
  • storage.HashingReader — only self-tested; Blob.Store hashes inline.
  • enrichment.Service.BulkCheckVulnerabilities, NormalizeLicense — never called.
  • server.ActiveRequestsMiddleware — no-op body; the active-request gauge is maintained by the inline r.Use at server.go:226.
  • mirror.RegistrySource — an "not yet implemented" stub with no production caller.

CONTRIBUTING.md storage section updated to match.

andrew added 3 commits August 21, 2026 09:16
The .golangci.yml goconst.ignore-tests setting was added in v2.12.0
(golangci/golangci-lint#6480). On the previously pinned v2.10.1,
config verify fails with "additional properties 'ignore-tests' not
allowed" and the setting is silently ignored at run time, so goconst
counts test-file literals toward min-occurrences.
- gofmt -w internal/server/health_test.go
- Replace HasSuffix+TrimSuffix with CutSuffix in ParseSize
Migrate the three test call sites of storage.NewFilesystem to
storage.OpenBucket("file://...") and drop the deprecated
StorageConfig.Path field from test configs, then delete code that
deadcode reports as unreachable from cmd/proxy:

- internal/storage/filesystem.go and its tests
- storage.HashingReader
- enrichment.Service.BulkCheckVulnerabilities and NormalizeLicense
- server.ActiveRequestsMiddleware (no-op body; the real tracking
  is the inline r.Use at server.go:226)
- mirror.RegistrySource (unimplemented stub)

metrics.UpdateCircuitBreakerState and RecordCircuitBreakerTrip are
kept because #275 wires them.

Update the CONTRIBUTING.md storage section to reflect blob.go.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the project’s lint/tooling baseline and performs mechanical cleanup by removing unused code and migrating remaining test call sites to the gocloud.dev/blob-backed storage implementation.

Changes:

  • Bump golangci-lint toolchain/deps (and Go/toolchain versions in go.mod), updating go.sum accordingly.
  • Apply mechanical lint fixes (formatting and strings.CutSuffix usage).
  • Remove dead code (filesystem storage implementation/tests, unused enrichment/mirror/middleware helpers, and HashingReader + its test) and migrate affected tests to storage.OpenBucket + StorageConfig.URL.

Reviewed changes

Copilot reviewed 15 out of 17 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/storage/storage.go Removes HashingReader and related imports.
internal/storage/storage_test.go Removes HashingReader unit test and now-unused import.
internal/storage/filesystem.go Deletes legacy filesystem storage implementation.
internal/storage/filesystem_test.go Deletes legacy filesystem storage tests.
internal/server/server_test.go Migrates server tests to storage.OpenBucket and StorageConfig.URL.
internal/server/middleware.go Removes dead ActiveRequestsMiddleware wrapper.
internal/server/middleware_test.go Removes tests for deleted ActiveRequestsMiddleware.
internal/server/health_test.go gofmt-style whitespace/alignment cleanup.
internal/server/eviction_test.go Migrates eviction tests to storage.OpenBucket, updates types and config usage.
internal/mirror/registry.go Deletes unused “not yet implemented” RegistrySource stub.
internal/mirror/registry_test.go Deletes tests for removed RegistrySource.
internal/enrichment/enrichment.go Removes unused enrichment helpers (BulkCheckVulnerabilities, NormalizeLicense).
internal/enrichment/enrichment_test.go Deletes tests for removed enrichment helpers.
internal/config/config.go Replaces HasSuffix+TrimSuffix with CutSuffix in ParseSize.
go.mod Updates Go version/toolchain and indirect tool dependencies.
go.sum Updates checksums for the toolchain/dependency bump.
CONTRIBUTING.md Updates storage documentation to reflect blob-based backends.
Suppressed comments (3)

internal/server/eviction_test.go:250

  • This test opens a storage bucket via storage.OpenBucket but never closes it. Add a defer/t.Cleanup that calls store.Close() so the underlying bucket is released even if the test fails early.
	store, err := storage.OpenBucket(context.Background(), "file://"+storagePath)
	if err != nil {
		t.Fatalf("failed to create storage: %v", err)
	}

internal/server/server_test.go:60

  • storage.OpenBucket returns a Storage that should be closed (Blob.Close calls bucket.Close). newTestServer stores it on testServer but testServer.close() currently only closes the DB and removes the temp dir; ensure the storage backend is also closed (e.g., call ts.storage.Close() in close()).
	store, err := storage.OpenBucket(context.Background(), "file://"+storagePath)
	if err != nil {
		_ = db.Close()
		_ = os.RemoveAll(tempDir)
		t.Fatalf("failed to create storage: %v", err)
	}

internal/server/eviction_test.go:40

  • storage.OpenBucket returns a Storage that holds resources (Blob wraps a *blob.Bucket). setupEvictionTest only closes the DB in t.Cleanup, leaving the bucket unclosed; add store.Close() to the cleanup to avoid leaking file handles/resources across tests.

This issue also appears on line 246 of the same file.

	store, err := storage.OpenBucket(context.Background(), "file://"+storagePath)
	if err != nil {
		_ = db.Close()
		t.Fatalf("failed to create storage: %v", err)
	}

	t.Cleanup(func() {
		_ = db.Close()
	})

	return db, store

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@andrew
andrew merged commit 272e6d9 into main Aug 21, 2026
9 checks passed
@andrew
andrew deleted the andrew/cleanup branch August 21, 2026 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants