Skip to content

fix(quality): test panic, PR-time CI, lint cleanup - #146

Merged
mayankpande88 merged 5 commits into
mainfrom
fix/test-and-lint-cleanup
May 12, 2026
Merged

fix(quality): test panic, PR-time CI, lint cleanup#146
mayankpande88 merged 5 commits into
mainfrom
fix/test-and-lint-cleanup

Conversation

@mayankpande88

Copy link
Copy Markdown
Contributor

This PR fixes #

Checklist

  • I have updated/added any relevant documentation

Description

What's the goal of this PR?

Address three pre-existing quality gaps surfaced while validating the recent dependabot dependency bumps:

  1. pkg/helm.Test_prepareOutput was panicking on every go test run on main.
  2. There was no PR-time CI — only push-to-branch image-build workflows, so dep bumps had to be hand-validated.
  3. 24 long-standing golangci-lint warnings on main.

What changes did you make?

Structured as three independent, bisectable commits:

1. test(helm): fix Test_prepareOutput nil-pointer panic

The helmRelease test fixture in pkg/helm/findscore_test.go was missing the Info field. prepareOutput accesses release.Info.LastDeployed.Time, panicking on nil Info. Production callers always receive a non-nil Info from helm. One-line fix to the fixture.

2. ci: add PR validation workflow (build, lint, test)

New .github/workflows/ci.yaml runs go build, golangci-lint, and go test -race on every PR to main and every push to main. Existing image-build workflows (only on push) are untouched.

3. chore: clear golangci-lint warnings

Clears all 24 lint issues. Semantics-preserving — every change either silences a discarded return with _/_ =, deletes provably dead code, or applies a trivial rewrite (copy() over a copy loop, De Morgan's law, lowercased error strings).

Review Notes → Risks & Counterarguments

  • Latent bugs preserved as-is in cmd/root.go: at lines 385–391 the original code does ahClient, err := NewArtifactHubCachedPackageClient(...) followed by helmClient, err := NewHelmRepoHubCachedPackageClient(...) then checks err. The err from the first call is silently overwritten. The error message at the check site ("error setting up artifact hub client") suggests the original intent was to check the first call. Same pattern with ahClient.List() and helmClient.ListRepo() below. I converted these to explicit _ discards to silence ineffassign without changing runtime behavior. These remain real bugs — a follow-up should add proper error handling for both calls.
  • defer file.Close() ordering bug also preserved: pkg/output/output.go:146 defers file.Close() before checking the os.Create error, so a nil file would panic on the deferred call. Lint just flags the unchecked Close; the ordering is a separate bug not in scope here.
  • errcheck via _ = rather than handling: I deliberately silenced discarded returns from fmt.Fprintln, w.Flush, w.WriteAll, os.Remove, resp.Body.Close. Proper handling (log on failure, surface stream errors, etc.) is a separate decision that shouldn't be bundled with mechanical cleanup. The pre-existing code clearly intended to ignore these; this PR just makes that intent explicit.
  • Bundling: Three concerns in one PR is unusual; chosen explicitly to ship a coherent "post-dep-bump quality baseline" in one review pass. Each commit stands alone for bisect.

How Has This Been Tested?

  • go build ./... — clean
  • golangci-lint run ./... — 0 issues (from 24 on main)
  • go test ./... — all packages pass (was failing on pkg/helm.Test_prepareOutput on main)

Validated locally on golangci-lint v2.11.3 and Go 1.26.1, on top of current main (HEAD f85179b).

The helmRelease fixture was missing the Info field, causing prepareOutput
to panic on release.Info.LastDeployed.Time. Production callers always
receive a non-nil Info from helm; the fixture should reflect that.
The repo only had image-build workflows gated to push events on main/test/prod.
PRs (including dependabot updates) had no automated gate, so dep bumps had
to be validated by hand. Adds a CI workflow that runs on pull_request and
pushes to main.
Clears all 24 lint warnings flagged on main. Semantics-preserving:

  errcheck (15) - wrap discarded defer Close/Remove and discarded
    fmt.Fprintln / w.Flush / w.WriteAll returns with explicit '_ ='
    instead of adding real error handling.
  staticcheck (5):
    - S1001 in pkg/helm/artifacthub.go: replace copy loop with copy()
    - QF1001 in cmd/root.go: De Morgan's law on format check
    - ST1005 (x2) in pkg/output/output.go: lowercase error strings
  ineffassign (2) in cmd/root.go: discard first err with '_' (NOTE:
    the original NewArtifactHubCachedPackageClient and ahClient.List
    errors were already silently dropped before this commit; this
    preserves that behavior but does not fix it).
  unused (3):
    - pkg/containers/images_test.go: remove dead setupKubeObjects /
      teardownKubeObjects helpers and the now-unused context import.
    - pkg/helm/helm_repo_exporter.go: remove dead helmRepocacheFile.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request focuses on code cleanup and improving error handling consistency. Key changes include removing unused variables and functions, replacing manual loops with the "copy" function, and explicitly ignoring errors in deferred calls and output formatting. Review feedback identifies critical issues where "defer" is called before error checks, potentially causing nil pointer panics, and highlights the risks of ignoring errors during client initialization and data listing in the helm handling logic.

Comment thread pkg/output/output.go Outdated
Comment thread cmd/root.go Outdated
pkg/output/output.go: move 'defer file.Close()' below the os.Create
error check so a failed Create can't trigger a nil-receiver panic on
the deferred Close.

cmd/root.go: replace the '_ =' suppressions of
NewArtifactHubCachedPackageClient and ahClient.List with proper error
checks. Also fix the misleading error message that attributed the
helm-repo-hub client error to the artifact hub client.
golangci-lint v2.x requires golangci-lint-action v7; v6 errors with
'golangci-lint v2 is not supported by golangci-lint-action v6'.
@mayankpande88
mayankpande88 merged commit c30bfdf into main May 12, 2026
1 check passed
@mayankpande88
mayankpande88 deleted the fix/test-and-lint-cleanup branch May 12, 2026 10:16
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.

3 participants