Subscribe to boilerplate openshift/golang-lint convention#556
Open
clcollins wants to merge 1 commit into
Open
Conversation
Bootstrap boilerplate framework with the golang-lint convention to standardize linting across SREP repositories. This replaces the ad-hoc golangci-lint setup with boilerplate's centrally managed config and auto-installation. Changes: - Add boilerplate/ directory with update script, config, and golang-lint convention files - Remove .golangci.yaml (boilerplate provides its own golangci.yml) - Remove lint and coverage Makefile targets (lint provided by boilerplate; coverage was a broken TODO) - Fix 42 lint errors surfaced by boilerplate's stricter config: errcheck, gosec, staticcheck, and unused findings - Update tests to match lowercased error strings per Go convention Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clcollins The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughThis PR adds boilerplate CI infrastructure (.gitattributes, Makefile include/target), removes golangci-lint configuration and related Makefile targets, and adds explicit error handling (panic on failure, warning logs) across feature registration, OCM client, and command code, plus lowercases error message text and adds nolint:gosec suppressions. ChangesBoilerplate and Lint Tooling
Estimated code review effort: 2 (Simple) | ~12 minutes Error Handling and Message Fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Estimated code review effortEstimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: N/A (not determinable from provided information) 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
cmd/root.go (1)
119-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiscarded
CloseClient()error hides real cleanup failures.
ocm.CloseClient()returns an error from client.Close() (or nil if the client is nil) — a real connection-close failure is now silently dropped via_ =. Elsewhere in this same PR (pkg/ocm/ocm.go'sconfig.Savehandling), failures on non-critical cleanup paths are logged vialog.Warningfrather than discarded outright. Consider the same pattern here for consistency and easier debugging of shutdown issues.As per path instructions,
**/*.goguidance states: "Never ignore error returns."♻️ Proposed fix
PersistentPostRun: func(cmd *cobra.Command, args []string) { - _ = ocm.CloseClient() + if err := ocm.CloseClient(); err != nil { + log.Debugf("error closing OCM client: %v", err) + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/root.go` at line 119, The cleanup in ocm.CloseClient() is currently being discarded, which hides shutdown failures. Update the caller in root command teardown to handle the returned error instead of assigning it to _, and follow the same non-critical cleanup pattern used in pkg/ocm/ocm.go by logging a warning with context when CloseClient() fails. Use the ocm.CloseClient symbol to locate the call site and preserve consistency with other cleanup error handling in this PR.Source: Path instructions
pkg/ocmcontainer/ocmcontainer.go (1)
116-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiscarded cleanup errors mask real failures (client close / container stop).
Both
ocm.CloseClient()ando.Stop(0)return an error from client.Close() (or nil if the client is nil) that is now silently discarded via_ =. A failedStop(0)in particular can mean the container is left running (resource leak) with no diagnostic trail. Consider at least a debug/warning log on failure, matching the pattern used forconfig.Saveinpkg/ocm/ocm.go.As per path instructions,
**/*.goguidance states: "Never ignore error returns."♻️ Proposed fix
- o.RegisterPreExecCleanupFunc(func() { _ = ocm.CloseClient() }) + o.RegisterPreExecCleanupFunc(func() { + if err := ocm.CloseClient(); err != nil { + log.Debugf("error closing OCM client: %v", err) + } + })o.RegisterPostExecCleanupFunc(func() { - _ = o.Stop(0) + if err := o.Stop(0); err != nil { + log.Warningf("error stopping container: %v", err) + } })Also applies to: 418-418
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ocmcontainer/ocmcontainer.go` at line 116, The cleanup callback registered via o.RegisterPreExecCleanupFunc is discarding failures from ocm.CloseClient, and the same issue applies to o.Stop(0) elsewhere in ocmcontainer.go. Update these cleanup paths to handle returned errors instead of assigning them to _, and add a debug/warning log on failure using the same style as the config.Save error handling in pkg/ocm/ocm.go. Keep the cleanup behavior intact, but make sure any close/stop failure is visible through a log message tied to ocm.CloseClient and o.Stop.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/subprocess/subprocess_test.go`:
- Line 54: The subprocess test still triggers the noctx lint because
exec.Command is used without a context, and the gosec nolint does not cover that
warning. Update the test helper that builds cmd to use exec.CommandContext with
an appropriate context (for example from the test) and add the context import if
needed, while keeping the existing subprocess invocation behavior intact.
---
Nitpick comments:
In `@cmd/root.go`:
- Line 119: The cleanup in ocm.CloseClient() is currently being discarded, which
hides shutdown failures. Update the caller in root command teardown to handle
the returned error instead of assigning it to _, and follow the same
non-critical cleanup pattern used in pkg/ocm/ocm.go by logging a warning with
context when CloseClient() fails. Use the ocm.CloseClient symbol to locate the
call site and preserve consistency with other cleanup error handling in this PR.
In `@pkg/ocmcontainer/ocmcontainer.go`:
- Line 116: The cleanup callback registered via o.RegisterPreExecCleanupFunc is
discarding failures from ocm.CloseClient, and the same issue applies to
o.Stop(0) elsewhere in ocmcontainer.go. Update these cleanup paths to handle
returned errors instead of assigning them to _, and add a debug/warning log on
failure using the same style as the config.Save error handling in
pkg/ocm/ocm.go. Keep the cleanup behavior intact, but make sure any close/stop
failure is visible through a log message tied to ocm.CloseClient and o.Stop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b36ad991-f110-4794-ad87-acef40a7868f
⛔ Files ignored due to path filters (23)
boilerplate/_data/backing-image-tagis excluded by!boilerplate/**boilerplate/_data/last-boilerplate-commitis excluded by!boilerplate/**boilerplate/_lib/boilerplate-commitis excluded by!boilerplate/**boilerplate/_lib/boilerplate.mkis excluded by!boilerplate/**boilerplate/_lib/common.shis excluded by!boilerplate/**boilerplate/_lib/container-makeis excluded by!boilerplate/**boilerplate/_lib/freeze-checkis excluded by!boilerplate/**boilerplate/_lib/release.shis excluded by!boilerplate/**boilerplate/_lib/subscriberis excluded by!boilerplate/**boilerplate/_lib/subscriber-proposeis excluded by!boilerplate/**boilerplate/_lib/subscriber-propose-updateis excluded by!boilerplate/**boilerplate/_lib/subscriber-reportis excluded by!boilerplate/**boilerplate/_lib/subscriber-report-onboardingis excluded by!boilerplate/**boilerplate/_lib/subscriber-report-pris excluded by!boilerplate/**boilerplate/_lib/subscriber-report-releaseis excluded by!boilerplate/**boilerplate/_lib/subscriber.shis excluded by!boilerplate/**boilerplate/generated-includes.mkis excluded by!boilerplate/**boilerplate/openshift/golang-lint/README.mdis excluded by!boilerplate/**boilerplate/openshift/golang-lint/ensure.shis excluded by!boilerplate/**boilerplate/openshift/golang-lint/golangci.ymlis excluded by!boilerplate/**boilerplate/openshift/golang-lint/standard.mkis excluded by!boilerplate/**boilerplate/updateis excluded by!boilerplate/**boilerplate/update.cfgis excluded by!boilerplate/**
📒 Files selected for processing (28)
.gitattributes.golangci.yamlMakefilecmd/flags.gocmd/root.gopkg/deprecation/deprecation.gopkg/engine/engine.gopkg/engine/engine_test.gopkg/features/additional-cluster-envs/additional-cluster-envs.gopkg/features/backplane/backplane.gopkg/features/certificate-authorities/certificate-authorities.gopkg/features/gcloud/gcloud.gopkg/features/image-cache/image-cache.gopkg/features/jira/jira.gopkg/features/legacy-aws-credentials/legacy-aws-credentials.gopkg/features/ops-utils/ops-utils.gopkg/features/osdctl/osdctl.gopkg/features/pagerduty/pagerduty.gopkg/features/persistent-histories/persistent-histories.gopkg/features/personalization/personalization.gopkg/features/ports/ports.gopkg/log/log-formatter.gopkg/log/logger.gopkg/log/logger_test.gopkg/ocm/ocm.gopkg/ocm/ocm_test.gopkg/ocmcontainer/ocmcontainer.gopkg/subprocess/subprocess_test.go
💤 Files with no reviewable changes (2)
- .golangci.yaml
- cmd/flags.go
| allArgs := []string{"-test.run=^$", "--", command} | ||
| allArgs = append(allArgs, args...) | ||
| cmd := exec.Command(os.Args[0], allArgs...) | ||
| cmd := exec.Command(os.Args[0], allArgs...) //nolint:gosec // test helper re-invoking own binary |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
noctx lint finding not addressed.
Static analysis still flags Line 54: exec.Command should be exec.CommandContext. The added //nolint:gosec only suppresses gosec (G204), not the separate noctx linter warning, so this line will likely still fail make lint.
🔧 Proposed fix
- cmd := exec.Command(os.Args[0], allArgs...) //nolint:gosec // test helper re-invoking own binary
+ cmd := exec.CommandContext(context.Background(), os.Args[0], allArgs...) //nolint:gosec // test helper re-invoking own binaryAdd "context" to imports if not already present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cmd := exec.Command(os.Args[0], allArgs...) //nolint:gosec // test helper re-invoking own binary | |
| cmd := exec.CommandContext(context.Background(), os.Args[0], allArgs...) //nolint:gosec // test helper re-invoking own binary |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 54-54: os/exec.Command must not be called. use os/exec.CommandContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/subprocess/subprocess_test.go` at line 54, The subprocess test still
triggers the noctx lint because exec.Command is used without a context, and the
gosec nolint does not cover that warning. Update the test helper that builds cmd
to use exec.CommandContext with an appropriate context (for example from the
test) and add the context import if needed, while keeping the existing
subprocess invocation behavior intact.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openshift/golang-lintconvention for centralized lint config and golangci-lint version management.golangci.yamland Makefile lint/coverage targets (replaced by boilerplate-provided equivalents)# TODO: Add golang build/tests here (onboard project to boilerplate?)What boilerplate provides
golangci.ymlwith errcheck, gosec, govet, ineffassign, misspell, staticcheck, unused lintersensure.shmake boilerplate-freeze-checkto prevent accidental modification of managed filesmake boilerplate-updateto pull latest convention changesWhat was NOT subscribed
openshift/golang-codecov— itscodecov.shis Prow-oriented; no value with Konflux CIopenshift/osd-container-image— generates Prow artifacts, expectsbuild/Dockerfile; incompatible with our multi-variant ContainerfileLint fixes by category
panicininit())fmt.FprintfoverWriteString(Sprintf(...)), simplify conditions, use raw strings for regexnolintdirectives for false positives (env var key names, test fixtures, intentional credential marshaling)verboseanddeferredClosevariablesTest plan
make lint— 0 issuesgo test ./...— all 22 packages passmake boilerplate-update— no diff (fully in sync with boilerplate HEAD)make boilerplate-freeze-check— clean🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores