Bugfix/workflow lint error #83
Conversation
WalkthroughThis PR updates container image references from ubi8-based tags to ubi9-based versions across the codebase, migrates golangci-lint configuration from v1 to v2, updates GitHub Actions workflows with newer runner and action versions, and adds lint suppression annotations throughout the Go source code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.golangci.next.yaml (1)
1-42: Golangci-lint v2 configuration enables stricter linters.The configuration explicitly enables
staticcheck(line 18) andgosec(line 13), which is good for code quality. However, the PR responds to these stricter lints by adding suppressions rather than fixes.The configuration itself is correct, but the implementation strategy in this PR is problematic:
- ✅ Enabling staticcheck and gosec improves code quality
- ❌ Suppressing all warnings with
nolintcomments defeats the purpose- ❌ The PR title "Bugfix/workflow lint error" is misleading—no bugs are being fixed
Suggested approach:
- Keep this config file
- Remove the nolint suppressions from production code
- Fix the underlying issues (use initialization helpers, proper accessors)
- Consider keeping nolint only in test code temporarily if needed
#!/bin/bash # Count how many nolint suppressions were added in this PR git diff origin/master --unified=0 | grep -c "nolint:staticcheck"internal/controller/ivorycluster/volumes.go (1)
225-231: Useinitialize.Labels()before assigning labels to ObjectMeta.Direct assignments to
ObjectMeta.Labelsat lines 225 and 278 are suppressed withnolint:staticcheck. The codebase has aninitialize.Labels()helper function that safely initializes nil labels to an empty map before assignment. This pattern is already used inpatroni/reconcile.goandpgmonitor.go. Remove the lint suppressions and callinitialize.Labels(volume)before each label assignment to align with established patterns in the codebase.
♻️ Duplicate comments (1)
internal/controller/ivorycluster/patroni_test.go (1)
114-114: ObjectMeta staticcheck suppressions consistent with pgbackrest_test.go—same linter migration concern applies.These suppressions follow the same pattern observed in pgbackrest_test.go and appear to be part of the golangci-lint v2 migration. The same recommendation applies: add PR context explaining the linter change that necessitated these suppressions.
Also applies to: 119-119, 142-142, 148-148, 489-489, 490-490
🧹 Nitpick comments (11)
internal/config/config_test.go (1)
28-38: Consider logging errors instead of suppressing them.While ignoring errors in test cleanup is common, failing to restore environment variables could cause test pollution. Instead of suppressing the error check, consider logging any errors that occur during cleanup.
Apply this diff to log cleanup errors:
func saveEnv(t testing.TB, key string) { t.Helper() previous, ok := os.LookupEnv(key) t.Cleanup(func() { if ok { - os.Setenv(key, previous) // nolint:errcheck + if err := os.Setenv(key, previous); err != nil { + t.Logf("failed to restore env var %s: %v", key, err) + } } else { - os.Unsetenv(key) // nolint:errcheck + if err := os.Unsetenv(key); err != nil { + t.Logf("failed to unset env var %s: %v", key, err) + } } }) }internal/upgradecheck/helpers_test.go (1)
58-66: Consider simplifying the switch statement.The
switchstatement withnolint:staticcheckcould be simplified to anif-elsefor better readability and to avoid the linter warning altogether.Apply this diff:
func (f *fakeClientWithError) Patch(ctx context.Context, obj crclient.Object, patch crclient.Patch, opts ...crclient.PatchOption) error { - switch { // nolint:staticcheck - case f.errorType == "patch error": + if f.errorType == "patch error" { return fmt.Errorf("patch error") - default: - return f.Client.Patch(ctx, obj, patch, opts...) } + return f.Client.Patch(ctx, obj, patch, opts...) }internal/upgradecheck/http.go (1)
110-110: Consider logging the Close error.While it's common to ignore
Close()errors in defer statements, logging the error could help diagnose issues during upgrade checks without adding significant complexity.Apply this diff:
- defer res.Body.Close() // nolint:errcheck + defer func() { + if closeErr := res.Body.Close(); closeErr != nil { + log := logging.FromContext(ctx) + log.V(1).Info("failed to close response body", "error", closeErr) + } + }()internal/controller/ivorycluster/pgbouncer_test.go (1)
120-122: Consider fixing the root cause rather than suppressing staticcheck warnings.Multiple
nolint:staticcheckdirectives have been added to silence warnings on metadata assertions. Instead of suppressing these warnings, consider addressing the underlying pattern that triggers them. For example, you could use helper functions to safely access metadata fields or initialize them properly before assertions.This pattern appears throughout the test file and may indicate a broader opportunity to improve metadata handling in tests.
Also applies to: 125-129, 151-154, 157-162, 434-436, 439-443
internal/controller/ivorycluster/pki.go (1)
77-77: Consider safer metadata handling patterns.The
nolint:staticcheckdirectives are suppressing warnings on direct OwnerReferences assignments. While this works, consider using Kubernetes' metadata helper functions for safer field handling. For example, you could check if the source OwnerReferences are nil before assignment, or use a proper deep copy pattern.This is a minor refactor suggestion for improved robustness, though the current code with suppressions may be acceptable for now.
Also applies to: 153-153
internal/bridge/client.go (1)
103-106: Targeted nolint usage is reasonable; consider a tiny Close wrapper instead of nolint (optional).
- Line [104]: Adding
// nolint:staticcheckto thec.Client.Docall keeps behavior identical and the existing//nolint:bodyclosecomment already documents that the caller owns the response body; this looks fine.- Line [190]: Ignoring the error from
response.Body.Close()is conventional for HTTP bodies, so the added// nolint:errcheckis acceptable. If you’d like to drop the nolint while keeping the same behavior, you could wrap it as:defer func() { _ = response.Body.Close() }()Also applies to: 189-191
internal/pgbackrest/reconcile_test.go (1)
139-143: PVC ClaimName check + nolint is acceptable for this invariant test.This condition is asserting that a repo volume is backed by a PVC with the expected name; if
PersistentVolumeClaimwere ever nil the test would panic, which is a valid signal thatAddRepoVolumesToPodregressed. The added// nolint:staticcheckis therefore narrowly scoped and acceptable. If you prefer to avoid nolint, you could add an explicitpvc := v.VolumeSource.PersistentVolumeClaimnil check before usingpvc.ClaimName, but that’s optional here.internal/controller/ivorycluster/patroni_test.go (1)
3067-3067: ObjectMeta field assignment suppressions appear repetitive—consider centralizing or documenting.Multiple instances of direct ObjectMeta field assignments (e.g., setting
.Namespace) are decorated withnolint:staticcheck. While these may be necessary for the linter migration, the repetitive pattern across test setup code suggests this could be:
- Documented once with a comment explaining the linter change
- Applied only to the most critical instances if possible
Consider adding a brief explanation in test setup helper functions or at the top of the file clarifying why ObjectMeta field access requires suppression in this version of golangci-lint.
Also applies to: 3071-3071, 3075-3075, 3091-3091, 3095-3095, 3099-3099, 3269-3269, 3273-3273, 3277-3277
internal/controller/ivorycluster/cluster_test.go (1)
157-158: Test code also needs proper metadata initialization.The test setup code suppresses the same staticcheck warnings as production code. Tests should follow the same patterns as production code for consistency.
Consider using the initialization helpers in test setup as well:
cluster := testCluster() +initialize.ObjectMeta(&cluster.ObjectMeta) -cluster.ObjectMeta.Name = "global-cluster" // nolint:staticcheck +cluster.ObjectMeta.Name = "global-cluster" -cluster.ObjectMeta.Namespace = ns.Name // nolint:staticcheck +cluster.ObjectMeta.Namespace = ns.NameAlso applies to: 207-208, 261-262, 308-309, 410-411, 461-462, 515-516, 562-563
internal/controller/ivorycluster/pki_test.go (2)
103-103: Review owner reference checks needing lint suppression.Multiple checks on
OwnerReferenceslength and array access are suppressed. This pattern suggests staticcheck might be warning about potential out-of-bounds access.Consider defensive coding to avoid suppression:
-assert.Check(t, len(rootSecret.ObjectMeta.OwnerReferences) == 1, "first owner reference not set") // nolint:staticcheck +assert.Check(t, len(rootSecret.ObjectMeta.OwnerReferences) == 1, "first owner reference not set") -if len(rootSecret.ObjectMeta.OwnerReferences) > 0 { // nolint:staticcheck - assert.Equal(t, rootSecret.ObjectMeta.OwnerReferences[0], expectedOR) // nolint:staticcheck +if assert.Check(t, len(rootSecret.ObjectMeta.OwnerReferences) > 0) { + assert.Equal(t, rootSecret.ObjectMeta.OwnerReferences[0], expectedOR) }Also applies to: 112-114, 128-128, 137-139
316-316: Metadata field assignment in test also needs proper initialization.Consistent with production code, test code should use initialization helpers for metadata fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/lint.yaml(1 hunks).github/workflows/test.yaml(4 hunks).golangci.next.yaml(2 hunks).golangci.yaml(1 hunks)Makefile(1 hunks)config/manager/manager.yaml(1 hunks)examples/ivorycluster/ivorycluster.yaml(2 hunks)examples/kustomize/azure/ivory.yaml(2 hunks)examples/kustomize/gcs/ivory.yaml(2 hunks)examples/kustomize/high-availability/ha-ivory.yaml(2 hunks)examples/kustomize/install/manager/manager.yaml(1 hunks)examples/kustomize/ivory/ivory.yaml(2 hunks)examples/kustomize/keycloak/ivory.yaml(2 hunks)examples/kustomize/multi-backup-repo/ivory.yaml(2 hunks)examples/kustomize/s3/ivory.yaml(2 hunks)internal/bridge/client.go(2 hunks)internal/bridge/client_test.go(5 hunks)internal/bridge/installation_test.go(2 hunks)internal/config/config_test.go(1 hunks)internal/controller/ivorycluster/cluster.go(1 hunks)internal/controller/ivorycluster/cluster_test.go(9 hunks)internal/controller/ivorycluster/controller.go(1 hunks)internal/controller/ivorycluster/helpers_test.go(1 hunks)internal/controller/ivorycluster/patroni_test.go(4 hunks)internal/controller/ivorycluster/pgadmin_test.go(3 hunks)internal/controller/ivorycluster/pgbackrest.go(5 hunks)internal/controller/ivorycluster/pgbackrest_test.go(6 hunks)internal/controller/ivorycluster/pgbouncer_test.go(3 hunks)internal/controller/ivorycluster/pki.go(2 hunks)internal/controller/ivorycluster/pki_test.go(5 hunks)internal/controller/ivorycluster/util_test.go(2 hunks)internal/controller/ivorycluster/volumes.go(6 hunks)internal/controller/ivyupgrade/apply.go(1 hunks)internal/controller/ivyupgrade/ivyupgrade_controller.go(2 hunks)internal/patroni/api_test.go(7 hunks)internal/pgbackrest/reconcile_test.go(1 hunks)internal/pgmonitor/api.go(1 hunks)internal/upgradecheck/helpers_test.go(1 hunks)internal/upgradecheck/http.go(1 hunks)internal/util/util.go(2 hunks)testing/kuttl/e2e-other/exporter-upgrade/00--cluster.yaml(1 hunks)testing/kuttl/e2e-other/exporter-upgrade/02--update-cluster.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
internal/controller/ivorycluster/patroni_test.go (2)
internal/testing/cmp/cmp.go (1)
DeepEqual(60-62)internal/initialize/metadata.go (1)
Annotations(23-27)
internal/controller/ivorycluster/pgbackrest_test.go (2)
internal/pgbackrest/config.go (1)
CMInstanceKey(41-41)internal/naming/names.go (3)
PGBackRestRestoreJob(456-461)PatroniDistributedConfiguration(388-393)PatroniTrigger(423-428)
internal/controller/ivorycluster/cluster_test.go (1)
internal/initialize/metadata.go (2)
Annotations(23-27)Labels(30-34)
internal/controller/ivorycluster/pgbouncer_test.go (2)
internal/testing/cmp/cmp.go (1)
DeepEqual(60-62)internal/initialize/metadata.go (2)
Annotations(23-27)Labels(30-34)
internal/controller/ivorycluster/pgadmin_test.go (2)
internal/testing/cmp/cmp.go (1)
DeepEqual(60-62)internal/initialize/metadata.go (2)
Annotations(23-27)Labels(30-34)
internal/controller/ivorycluster/pgbackrest.go (1)
internal/initialize/metadata.go (2)
Labels(30-34)Annotations(23-27)
internal/controller/ivorycluster/controller.go (1)
internal/bridge/client.go (1)
Client(35-41)
internal/controller/ivorycluster/volumes.go (2)
internal/initialize/metadata.go (2)
Labels(30-34)Annotations(23-27)internal/naming/labels.go (4)
DirectoryMoveJobLabels(159-165)LabelMovePGDataDir(49-49)LabelMovePGWalDir(52-52)LabelMovePGBackRestRepoDir(46-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: kuttl-k3d (v1.24)
- GitHub Check: kuttl-k3d (v1.22)
- GitHub Check: kuttl-k3d (v1.23)
- GitHub Check: kuttl-k3d (v1.25)
- GitHub Check: kubernetes-api (1.23.5)
- GitHub Check: kuttl-k3d (v1.21)
- GitHub Check: kubernetes-k3d (v1.28)
- GitHub Check: kubernetes-k3d (v1.25)
🔇 Additional comments (28)
internal/pgmonitor/api.go (1)
59-59: Typo fix in log message is correctThe updated log string fixes the spelling of “configuration” without changing behavior; everything else in the function remains consistent.
examples/kustomize/keycloak/ivory.yaml (1)
6-6: Verify UBI9 image compatibility and test results.The container images have been updated from UBI8-based to UBI9-based tags. Ensure that:
- The UBI9 images are compatible with the current IvorySQL and pgBackRest versions
- The changes have been tested in a representative environment
- Any UBI9-specific configuration or dependency changes are accounted for
Consider testing the deployment with these new image tags and documenting the results in the PR description.
Also applies to: 28-28
examples/kustomize/install/manager/manager.yaml (1)
29-32: Verify UBI9 image compatibility for operator defaults.These environment variables define the default images used by the operator. The update to UBI9-based images should be thoroughly tested to ensure:
- Backward compatibility with existing IvoryCluster resources
- No breaking changes in UBI9 that affect IvorySQL or pgBackRest functionality
- Proper documentation of the image version change for users
Document the testing performed with these new default images in the PR description.
internal/controller/ivorycluster/controller.go (1)
382-382: [rewritten review comment]
[classification tag]internal/bridge/installation_test.go (1)
111-111: The code is correct.wait.Backofffromk8s.io/apimachinery/pkg/util/waitis a public struct with public fields designed for direct assignment. There is no alternative method or constructor for settingSteps—direct field assignment is the standard and only way to configure backoff parameters in Kubernetes. Thenolint:staticchecksuppression addresses a linter style warning but does not indicate a real issue with the code.internal/controller/ivyupgrade/apply.go (1)
144-144: Confirm the staticcheck warning is consistent with similar code patterns in the codebase.The
nolint:staticcheckdirective on line 144 appears to suppress a warning related to the variadic argument spreading pattern. This identical pattern—prependingr.Ownerto options before spreading—appears ininternal/controller/ivorycluster/controller.gowith the same suppression, indicating an intentional and consistent approach. Without running staticcheck directly, the exact warning cannot be verified, but the code pattern is functionally correct and well-documented. Consider documenting the specific warning being suppressed if it relates to a known limitation or false positive in controller-runtime v0.12.3.internal/controller/ivorycluster/cluster.go (1)
131-131: Lint suppression on metadata copy.The
nolint:staticcheckdirective suppresses a warning on theDeepCopyIntocall for ObjectMeta. This is likely acceptable sinceDeepCopyIntois a Kubernetes-generated method, but consider verifying that both the source and destination metadata are properly initialized before the copy operation.examples/kustomize/azure/ivory.yaml (1)
6-6: Verify compatibility and test the major image upgrades before deployment.The container images are being updated from UBI8-based to UBI9-based versions, which includes:
- Base OS upgrade (UBI 8 → UBI 9)
- IvorySQL version jump (2.2-1.1-1 → 5.1-5.1-1)
- pgBackRest version update (2.45-1.1-1 → 2.56.0-5.1-1)
These are significant upgrades that may introduce breaking changes or require migration steps. Ensure that:
- All image versions are tested for compatibility with each other
- The upgrades have been validated in a representative environment
- Any required migration procedures are documented and communicated to users
internal/bridge/client_test.go (1)
34-39: Backoff-related nolint annotations in tests are well-scoped.All added
// nolint:staticcheckcomments here are confined to test code that intentionally pokes atwait.Backoffinternals (Steps/Duration andStep()), which is exactly where staticcheck can be noisy but not very helpful. The overall test behavior and assertions remain unchanged and the suppressions are narrowly targeted.Also applies to: 72-75, 115-119, 172-176, 192-196
examples/kustomize/multi-backup-repo/ivory.yaml (1)
6-7: Image tags updated consistently to ubi9.The IvorySQL and pgBackRest images now point to ubi9-based tags while the rest of the spec is unchanged; this aligns with the PR’s broader image upgrade and looks good.
Also applies to: 17-18
internal/controller/ivorycluster/pgadmin_test.go (2)
105-131: ConfigMap metadata DeepEqual assertions with nolint are fine.The new
// nolint:staticcheckmarkers on theassert.DeepEqualcalls forconfigmap.ObjectMeta.Annotationsandconfigmap.ObjectMeta.Labelsare tightly focused and preserve the existing merge semantics being tested (cluster-level + PGAdmin-specific metadata). No behavioral impact; this is a reasonable way to silence staticcheck here.
199-260: Service metadata/selector DeepEqual assertions with nolint are also appropriate.The added
// nolint:staticcheckcomments on theservice.ObjectMeta.Annotations,service.ObjectMeta.Labels, andservice.Spec.Selectorassert.DeepEqualcalls keep the tests’ intent clear: verify correct merging of cluster metadata, optional PGAdmin.Service metadata, and the fixed selector labels. Suppressions are narrowly scoped and do not alter behavior.examples/kustomize/s3/ivory.yaml (1)
6-7: S3 example images updated to ubi9 consistently.Both IvorySQL and pgBackRest images now reference ubi9-based tags; indentation and structure are unchanged. This matches the rest of the repo’s image updates.
Also applies to: 17-18
internal/patroni/api_test.go (1)
73-81: Ignoring write errors in test executors via nolint is acceptable.All added
// nolint:errcheckmarkers are on stdout/stderrWritecalls inside test-only executor stubs where the error value is irrelevant and already being discarded. The suppressions are precise, keep the tests simple, and don’t change behavior.Also applies to: 125-132, 177-184, 253-260, 265-272, 277-284, 289-296
internal/controller/ivyupgrade/ivyupgrade_controller.go (1)
398-412: Delete + IgnoreNotFound patterns remain correct; nolint is narrowly scoped.
- Line [410]: Deleting replica-create Jobs with
Preconditionsand an explicit background propagation policy, then wrapping inclient.IgnoreNotFound, still correctly updateserrwhile treating “not found” as non-fatal. The added// nolint:staticcheckis tightly scoped and does not alter control flow.- Line [463]: Same pattern for Patroni Endpoints; deletes honor UID/resourceVersion preconditions, ignore 404s, and propagate any other error via
errwhile requeuing. Behavior is unchanged.Also applies to: 458-463
examples/kustomize/ivory/ivory.yaml (1)
6-6: Straightforward image tag updates—no issues identified.The container image references have been updated consistently from ubi8 to ubi9 base images with corresponding version bumps for IvorySQL and pgbackrest.
Also applies to: 18-18
examples/kustomize/high-availability/ha-ivory.yaml (1)
6-6: Consistent with broader image modernization—approved.The container image references match the updates made in related manifests.
Also applies to: 29-29
examples/ivorycluster/ivorycluster.yaml (1)
6-6: Image updates are consistent across all example manifests.Also applies to: 18-18
config/manager/manager.yaml (1)
22-34: All RELATED_IMAGE environment variables updated consistently to ubi9 tags.The operator's environment variables have been comprehensively updated to reference ubi9-based container images with corresponding minor/patch version increments.
Makefile (1)
209-209: Test image reference updated to match deployment images..github/workflows/test.yaml (1)
67-69: All workflow image references updated to ubi9-based tags consistently.Test environment container images and operator environment variables have been comprehensively updated to align with deployment configuration.
Also applies to: 101-107, 121-127, 142-142
internal/controller/ivorycluster/helpers_test.go (1)
46-48: LGTM! Image updates to ubi9 base are consistent across the codebase.The test image constants are correctly updated from ubi8 to ubi9 versions (ubi9-13.6-1, ubi9-2.38-0, ubi9-1.16-2), and verification confirms no remaining ubi8 references exist in the repository. This aligns with the broader PR objective of updating container image references.
internal/controller/ivorycluster/util_test.go (1)
232-232: Thenolint:staticcheckannotations are necessary. These suppressions were explicitly added in commit 1002970 (titled "lint workflow error fix") to resolve staticcheck warnings in the linting workflow. Removing them would cause linting failures. The pattern of similar suppressions throughout the codebase (strings.Replace, Backoff fields, API field access) confirms they suppress legitimate deprecation warnings (SA1019) rather than false positives.Likely an incorrect or invalid review comment.
.golangci.yaml (5)
7-12: Enabled linters set looks well-curated.The selection of depguard, gomodguard, importas, misspell, and unconvert provides good coverage for dependency control and code quality without excessive noise. The importas linting (combined with line 58's
no-unaliased: trueenforcement) is particularly appropriate for a Kubernetes-heavy codebase.
19-35: Well-structured depguard rules with clear rationale.The rules correctly enforce:
- io/ioutil deprecation (with reference to official Go 1.16 migration guide)
- Test package isolation via the
not-testsrule scoped to non-test filesThe descriptions provide clear migration/reasoning guidance.
37-58: Remaining linter settings are appropriately configured.
exhaustivedefault-signifies-exhaustive setting is reasonable for switch statement checkinggomodguardmodule blocks enforce consistent dependency versions (YAML, gotest.tools) and prevent direct Kubernetes project dependency (with clear explanation)importasregex aliases provide appropriate shorthand for Kubernetes API imports while enforcing no-unaliased mode
60-62: Build-tags configuration is appropriate.Including
envtestallows the linter to properly analyze code with Kubernetes integration test dependencies, which is standard for operator projects.
1-62: The.golangci.yamlfile correctly follows the golangci-lint v2 configuration schema with proper nesting of linter settings underlinters.settings. The configuration syntax and structure are valid.However, the PR description is empty. Please provide a brief explanation of:
- The rationale for this v2 migration
- Why
contextcheckanderrchkjsonare disabled
| runs-on: ubuntu-24.04 | ||
| permissions: | ||
| contents: read | ||
| checks: write | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - uses: actions/setup-go@v3 | ||
| with: { go-version: 1.x } | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-go@v6 | ||
| with: { go-version: stable } | ||
|
|
||
| - uses: golangci/golangci-lint-action@v3 | ||
| - uses: golangci/golangci-lint-action@v9 | ||
| with: | ||
| version: v1.60.1 | ||
| version: latest | ||
| args: --timeout=5m | ||
|
|
||
| # Count issues reported by disabled linters. The command always | ||
| # exits zero to ensure it does not fail the pull request check. | ||
| - name: Count non-blocking issues | ||
| run: | | ||
| golangci-lint run --config .golangci.next.yaml \ | ||
| --issues-exit-code 0 \ | ||
| --max-issues-per-linter 0 \ | ||
| --max-same-issues 0 \ | ||
| --out-format json | | ||
| jq --sort-keys 'reduce .Issues[] as $i ({}; .[$i.FromLinter] += 1)' | | ||
| awk >> "${GITHUB_STEP_SUMMARY}" ' | ||
| NR == 1 { print "```json" } { print } END { if (NR > 0) print "```" } | ||
| ' || true | ||
| golangci-lint run --config .golangci.next.yaml --show-stats >> "${GITHUB_STEP_SUMMARY}" \ | ||
| --max-issues-per-linter=0 \ | ||
| --max-same-issues=0 \ | ||
| --uniq-by-line=0 \ | ||
| --output.text.path=/dev/null ||: |
There was a problem hiding this comment.
Reconsider the linting strategy: suppressions vs. fixes.
The workflow now uses .golangci.next.yaml which enables staticcheck and gosec, but the PR adds numerous nolint:staticcheck suppressions throughout the codebase instead of addressing the underlying issues. This defeats the purpose of enabling stricter linters.
Recommendation: Either:
- Fix the staticcheck warnings properly (using initialization helpers, proper accessor methods, etc.), OR
- Keep staticcheck disabled in the config until warnings can be addressed systematically
The current approach of "enable linter + suppress all warnings" provides no value and makes the codebase harder to maintain.
Based on learnings, addressing linter warnings properly improves code quality and prevents future issues.
🤖 Prompt for AI Agents
In .github/workflows/lint.yaml around lines 8–30, the workflow enables stricter
linters (staticcheck, gosec) while the PR added widespread
`//nolint:staticcheck` suppressions — this defeats the linter enablement; either
remove the suppressions and fix the underlying issues systematically or disable
staticcheck in the repo config until fixes are completed. To fix: run
golangci-lint locally against .golangci.next.yaml to collect current
staticcheck/gosec failures, group them by type, fix each class (e.g., use proper
initializers, accessors, avoid unused code, secure usages), remove the
corresponding `nolint` comments as fixes are merged, and update the
workflow/config to keep staticcheck enabled; OR if you cannot address all now,
revert staticcheck/gosec enablement in .golangci.next.yaml (and adjust the
workflow) so the repo isn’t masking warnings with `nolint` until a planned
remediation is executed.
| assert.Assert(t, marshalMatches(service.ObjectMeta.Annotations, // nolint:staticcheck | ||
| ` | ||
| some: note | ||
| `)) | ||
| `)) // nolint:staticcheck |
There was a problem hiding this comment.
Add explanatory comment for lint suppression on assertions.
Test assertions with nolint:staticcheck are unusual and warrant explanation. If the suppression is addressing a specific staticcheck rule related to comparison functions or string literals in assert.Assert() calls, add a code comment explaining which rule and why it's necessary. If the suppression is not actually needed, remove it.
🤖 Prompt for AI Agents
In internal/controller/ivorycluster/cluster_test.go around lines 777 to 780, the
test uses nolint:staticcheck on an assert.Assert call without explanation; add a
brief inline comment after the assertion explaining which specific staticcheck
rule (e.g., SA4006 or the exact staticcheck ID) is being suppressed and why that
rule is a false positive or not applicable for this comparison, or if there is
no valid justification, remove the nolint:staticcheck annotation so the linter
can run normally.
| case timelineCall: | ||
| timelineCall = false | ||
| stdout.Write([]byte(`[{"Cluster": "hippo-ha", "Member": "hippo-instance1-67mc-0", "Host": "hippo-instance1-67mc-0.hippo-pods", "Role": "Leader", "State": "running", "TL": 4}, {"Cluster": "hippo-ha", "Member": "hippo-instance1-ltcf-0", "Host": "hippo-instance1-ltcf-0.hippo-pods", "Role": "Replica", "State": "running", "TL": 4, "Lag in MB": 0}]`)) | ||
| stdout.Write([]byte(`[{"Cluster": "hippo-ha", "Member": "hippo-instance1-67mc-0", "Host": "hippo-instance1-67mc-0.hippo-pods", "Role": "Leader", "State": "running", "TL": 4}, {"Cluster": "hippo-ha", "Member": "hippo-instance1-ltcf-0", "Host": "hippo-instance1-ltcf-0.hippo-pods", "Role": "Replica", "State": "running", "TL": 4, "Lag in MB": 0}]`)) // nolint:errcheck |
There was a problem hiding this comment.
Error return values suppressed on stdout.Write() calls in mock code—reconsider.
Multiple nolint:errcheck annotations have been added to suppress error handling on stdout.Write() calls within the mock PodExec function. While test code often has relaxed error handling, suppressing write errors to stdout could hide test setup problems or data loss issues.
Recommendation:
- If errors from
stdout.Write()are truly non-critical in tests, add an explicit check with_ =instead of a blanket suppression - Alternatively, handle the error properly (e.g.,
t.Fatal()if in critical path) - This maintains readability and makes the intentional error suppression explicit
Also applies to: 557-557, 562-562, 564-564, 566-566
🤖 Prompt for AI Agents
In internal/controller/ivorycluster/patroni_test.go around lines 556 (and
similarly 557, 562, 564, 566), the test mock suppresses errors from
stdout.Write() with nolint:errcheck; replace the blanket suppression with
explicit intent: either capture and ignore the error using `_ =
stdout.Write(...)` if non‑critical, or check the returned error and call
t.Fatalf(...) (or t.Fatalf) when the write is critical to the test setup; apply
the same pattern to each listed stdout.Write call so the intent is explicit and
any real failures are surfaced.
| for k, v := range config.Data { | ||
| if v != "" { | ||
| if k == pgbackrest.CMInstanceKey { | ||
| if k == pgbackrest.CMInstanceKey { // nolint:staticcheck |
There was a problem hiding this comment.
Extensive lint suppressions added without explanation or PR context.
Multiple nolint:staticcheck annotations have been added throughout this test file on ObjectMeta field accesses and standard Kubernetes patterns (e.g., GetOwnerReferences()). The PR description provides no context explaining why these suppressions became necessary or whether the underlying code should be refactored instead.
These suppressions appear to be a band-aid for stricter linting rules in golangci-lint v2, but without clear rationale in the PR, it's difficult to assess whether this is the appropriate solution.
Recommendation: Include an explanation in the PR description clarifying:
- Which specific golangci-lint v2 rules triggered these suppressions
- Why suppressing is preferred over refactoring the code
- Whether this pattern is applied consistently across the codebase
If these suppressions are indeed necessary due to linter migration, consider adding a brief code comment explaining the reason for the suppression (beyond just nolint:staticcheck).
Also applies to: 926-926, 854-854, 858-858
🤖 Prompt for AI Agents
internal/controller/ivorycluster/pgbackrest_test.go lines 486, 854, 858, 926:
the test file contains multiple unexplained "nolint:staticcheck" suppressions on
ObjectMeta and standard Kubernetes pattern accesses; remove or document
them—either (A) remove the nolint and refactor the code to satisfy the linter
(preferable) or (B) if suppression is required due to a golangci-lint v2 rule,
replace the bare nolint with an inline comment explaining which exact linter
rule triggered (e.g., staticcheck SAxxxx), why suppression is chosen over
refactor, and confirm this is applied consistently; also update the PR
description to list the specific rules, rationale for suppression vs refactor,
and mention codebase-wide consistency.
| if isCreate { | ||
| r.Recorder.Eventf(ivoryCluster, corev1.EventTypeNormal, EventRepoHostCreated, | ||
| "created pgBackRest repository host %s/%s", repoHost.TypeMeta.Kind, repoHostName) | ||
| "created pgBackRest repository host %s/%s", repoHost.TypeMeta.Kind, repoHostName) // nolint:staticcheck |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the code around line 2069 with proper context
cd internal/controller/ivorycluster && sed -n '2055,2085p' pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 190
🏁 Script executed:
# Find the pgbackrest.go file and examine the code around line 2069
find . -name "pgbackrest.go" -type fRepository: IvorySQL/ivory-operator
Length of output: 151
🏁 Script executed:
# Examine the code around line 2069 with context
sed -n '2050,2080p' ./internal/controller/ivorycluster/pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 1146
🏁 Script executed:
# Search for other Eventf calls with nolint comments to understand the pattern
rg "Eventf.*nolint" ./internal/controller/ivorycluster/pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 49
🏁 Script executed:
# Search for all Eventf calls in the file
rg "Eventf" ./internal/controller/ivorycluster/pgbackrest.go -nRepository: IvorySQL/ivory-operator
Length of output: 825
🏁 Script executed:
# Look at several Eventf calls to compare their formatting
sed -n '1040,1044p' ./internal/controller/ivorycluster/pgbackrest.go
sed -n '2066,2070p' ./internal/controller/ivorycluster/pgbackrest.go
sed -n '2212,2216p' ./internal/controller/ivorycluster/pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 641
🏁 Script executed:
# Check more context around the other Eventf calls that might have issues
sed -n '1481,1508p' ./internal/controller/ivorycluster/pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 998
🏁 Script executed:
# Check the complete context for line 1042 and surrounding Eventf calls
sed -n '1037,1045p' ./internal/controller/ivorycluster/pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 348
🏁 Script executed:
# Search for all nolint:staticcheck suppressions in the entire codebase to understand the pattern
rg "nolint:staticcheck" . -A 2 -B 2 | head -100Repository: IvorySQL/ivory-operator
Length of output: 6843
🏁 Script executed:
# Search for EventRepoHostCreated constant definition
rg "EventRepoHostCreated" . -B 2 -A 2Repository: IvorySQL/ivory-operator
Length of output: 1156
🏁 Script executed:
# Check the type of TypeMeta.Kind and understand the Eventf signature
# First, let's look at the imports and find what TypeMeta is
head -100 ./internal/controller/ivorycluster/pgbackrest.go | grep -E "import|TypeMeta"Repository: IvorySQL/ivory-operator
Length of output: 75
🏁 Script executed:
# Get the full imports section
sed -n '1,50p' ./internal/controller/ivorycluster/pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 1717
🏁 Script executed:
# Look at the repoHost variable declaration to understand its type
sed -n '2065,2075p' ./internal/controller/ivorycluster/pgbackrest.goRepository: IvorySQL/ivory-operator
Length of output: 450
🏁 Script executed:
# Find the applyRepoHostIntent method signature
rg "func.*applyRepoHostIntent" ./internal/controller/ivorycluster/pgbackrest.go -A 5Repository: IvorySQL/ivory-operator
Length of output: 390
🏁 Script executed:
# Check if there are any other uses of TypeMeta fields with nolint suppressions
rg "TypeMeta.*nolint" ./internal/controller/ivorycluster/ -rRepository: IvorySQL/ivory-operator
Length of output: 480
🏁 Script executed:
# Check if there are any other uses of TypeMeta fields with nolint suppressions
rg "TypeMeta.*nolint" ./internal/controller/ivorycluster/Repository: IvorySQL/ivory-operator
Length of output: 222
Remove or clarify the lint suppression on the Eventf call.
The nolint:staticcheck on line 2069 appears unnecessary. The format string has two %s specifiers matching the two string arguments (repoHost.TypeMeta.Kind and repoHostName), and other similar Eventf calls in the file without format issues do not use this suppression. If a staticcheck warning is being suppressed here, it should be documented with an explanatory comment rather than a bare suppression directive.
🤖 Prompt for AI Agents
internal/controller/ivorycluster/pgbackrest.go around line 2069: the bare "//
nolint:staticcheck" on the Eventf call should be removed or justified; if
staticcheck is not actually flagging a real issue, delete the nolint directive
so the code matches other Eventf calls, otherwise replace it with a short
explanatory comment like "// nolint:staticcheck // format string uses two %s
matching repoHost.TypeMeta.Kind and repoHostName (false positive)" and re-run
linters to confirm the suppression is necessary.
| backupJob.ObjectMeta = naming.PGBackRestBackupJob(ivoryCluster) | ||
| if currentBackupJob != nil { | ||
| backupJob.ObjectMeta.Name = currentBackupJob.ObjectMeta.Name | ||
| backupJob.ObjectMeta.Name = currentBackupJob.ObjectMeta.Name // nolint:staticcheck |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use initialization helpers instead of suppressing metadata warnings.
The backup Job metadata assignments suppress staticcheck warnings that could be addressed by using initialize.Labels() and initialize.Annotations() helpers before assignment, consistent with the pattern in relevant code snippets.
As per coding guidelines, prefer fixing the root cause:
+initialize.Labels(&backupJob.ObjectMeta)
-backupJob.ObjectMeta.Labels = labels // nolint:staticcheck
+backupJob.ObjectMeta.Labels = labels
+initialize.Annotations(&backupJob.ObjectMeta)
-backupJob.ObjectMeta.Annotations = annotations // nolint:staticcheck
+backupJob.ObjectMeta.Annotations = annotationsAlso applies to: 2268-2269, 2430-2430, 2444-2445
| moveDirJob.ObjectMeta.Annotations = naming.Merge(cluster.Spec.Metadata. // nolint:staticcheck | ||
| GetAnnotationsOrNil()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using initialization helpers instead of suppressing lint warnings.
Multiple metadata assignments in the Job reconciliation functions use nolint:staticcheck. The codebase provides initialize.Annotations() and initialize.Labels() helpers (shown in relevant code snippets) that ensure fields are initialized before assignment.
Apply this pattern to address the underlying issue:
+initialize.Annotations(&moveDirJob.ObjectMeta)
moveDirJob.ObjectMeta.Annotations = naming.Merge(cluster.Spec.Metadata.
- GetAnnotationsOrNil()) // nolint:staticcheck
+ GetAnnotationsOrNil())
+initialize.Labels(&moveDirJob.ObjectMeta)
-moveDirJob.ObjectMeta.Labels = labels // nolint:staticcheck
+moveDirJob.ObjectMeta.Labels = labelsThis approach fixes the root cause rather than suppressing warnings.
Also applies to: 439-439, 554-555, 561-561, 672-673, 679-679
🤖 Prompt for AI Agents
internal/controller/ivorycluster/volumes.go around lines 432-433 (and similarly
at 439, 554-555, 561, 672-673, 679): replace the direct metadata
annotation/label assignments that currently use // nolint:staticcheck with the
initialize helpers so the maps are guaranteed initialized before merging;
specifically, call initialize.Annotations(moveDirJob.ObjectMeta) (or
initialize.Labels for labels) to ensure the map exists, then assign/merge using
naming.Merge(...) without the nolint, and remove the nolint comment.
| cluster.Spec.DataSource.Volumes.PGWALVolume.Directory, | ||
| cluster.Spec.DataSource.Volumes.PGWALVolume.Directory, | ||
| cluster.ObjectMeta.Name) | ||
| cluster.ObjectMeta.Name) // nolint:staticcheck |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see the full context around line 576
sed -n '570,585p' internal/controller/ivorycluster/volumes.go
# Also check for GetName() usage patterns in the file
echo "---"
rg "\.GetName\(\)" internal/controller/ivorycluster/volumes.go | head -20Repository: IvorySQL/ivory-operator
Length of output: 819
🏁 Script executed:
#!/bin/bash
# Check the function signature and what 'cluster' is
sed -n '550,580p' internal/controller/ivorycluster/volumes.go
# Also search for other nolint:staticcheck comments in this file
echo "---"
rg "nolint:staticcheck" internal/controller/ivorycluster/volumes.go -B 2 -A 2Repository: IvorySQL/ivory-operator
Length of output: 2999
🏁 Script executed:
#!/bin/bash
# Find the function signature to see what type 'cluster' is
sed -n '500,530p' internal/controller/ivorycluster/volumes.go
# Also search for imports and type definition
head -50 internal/controller/ivorycluster/volumes.go | grep -E "^import|^package|type"
# Check if there's a custom Name field vs ObjectMeta.Name
rg "cluster\.Name|cluster\.ObjectMeta\.Name" internal/controller/ivorycluster/volumes.go | head -20Repository: IvorySQL/ivory-operator
Length of output: 1826
🏁 Script executed:
#!/bin/bash
# Find the function signature containing the cluster parameter
rg "func.*cluster" internal/controller/ivorycluster/volumes.go -A 3 | head -40
# Also check what type cluster is by looking at function parameters
rg "cluster \*" internal/controller/ivorycluster/volumes.go | head -10Repository: IvorySQL/ivory-operator
Length of output: 584
🏁 Script executed:
#!/bin/bash
# Find the IvoryCluster type definition
fd -t f "ivorycluster.go" | head -5
# Look for type IvoryCluster definition
rg "^type IvoryCluster struct" api/ -A 10Repository: IvorySQL/ivory-operator
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Find where IvoryCluster is defined
fd -t f -name "*.go" | xargs rg "^type IvoryCluster struct" | head -5
# Also check the directory structure
git ls-files | grep -E "(api|types|v1beta1)" | head -20Repository: IvorySQL/ivory-operator
Length of output: 1398
🏁 Script executed:
#!/bin/bash
# Look at the IvoryCluster struct definition
sed -n '/^type IvoryCluster struct/,/^}/p' pkg/apis/ivory-operator.ivorysql.org/v1beta1/ivorycluster_types.go | head -30Repository: IvorySQL/ivory-operator
Length of output: 666
Use cluster.Name instead of cluster.ObjectMeta.Name and remove the nolint comment.
Line 576 should follow the idiomatic Kubernetes pattern used throughout this file and codebase. Since IvoryCluster embeds metav1.ObjectMeta, cluster.Name and cluster.ObjectMeta.Name are equivalent, but cluster.Name is the standard accessor. Change cluster.ObjectMeta.Name to cluster.Name and remove the nolint:staticcheck comment.
🤖 Prompt for AI Agents
In internal/controller/ivorycluster/volumes.go around line 576, replace the
non-idiomatic use of cluster.ObjectMeta.Name with cluster.Name and remove the
trailing "// nolint:staticcheck" comment; update the expression to use the
standard accessor (cluster.Name) so it matches the codebase conventions and
eliminate the unnecessary nolint.
| } | ||
|
|
||
| return `"` + strings.Replace(identifier, `"`, `""`, -1) + `"` | ||
| return `"` + strings.Replace(identifier, `"`, `""`, -1) + `"` // nolint:staticcheck |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace deprecated strings.Replace with strings.ReplaceAll.
The nolint:staticcheck directives are suppressing warnings about using the deprecated strings.Replace(s, old, new, -1) pattern. Instead of suppressing these warnings, update the code to use strings.ReplaceAll(s, old, new), which has been available since Go 1.12 and provides the same functionality with clearer intent.
Apply these changes:
- return `"` + strings.Replace(identifier, `"`, `""`, -1) + `"` // nolint:staticcheck
+ return `"` + strings.ReplaceAll(identifier, `"`, `""`) + `"`- literal = strings.Replace(literal, `'`, `''`, -1) // nolint:staticcheck
+ literal = strings.ReplaceAll(literal, `'`, `''`)- literal = strings.Replace(literal, `\`, `\\`, -1) // nolint:staticcheck
+ literal = strings.ReplaceAll(literal, `\`, `\\`)Also applies to: 59-59, 66-66
🤖 Prompt for AI Agents
internal/util/util.go around lines 38, 59 and 66: the code uses the deprecated
pattern strings.Replace(s, old, new, -1) (currently suppressed with
nolint:staticcheck); replace each call with strings.ReplaceAll(s, old, new) and
remove the unnecessary nolint directive so the code uses the clearer,
non-deprecated API and staticcheck warnings are not suppressed.
2 participants
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
What is the new behavior (if this is a feature change)?
Other Information:
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.