CNTRLPLANE-3510: Enable additional golangci-lint linters#8567
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR standardizes error handling and context usage across the repository: it enables the errorlint and nilerr linters; replaces many non-wrapping fmt.Errorf uses with %w; converts direct type assertions/equality to errors.As / errors.Is where appropriate; makes HTTP requests, dialing, and some command executions context-aware (http.NewRequestWithContext, DialContext, exec.CommandContext); adds //nolint:nilerr annotations on intentional retry branches returning nil errors; and updates tests and minor textual fixes. No public API signatures were changed. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
bryan-cox
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8567 +/- ##
==========================================
+ Coverage 40.61% 41.18% +0.57%
==========================================
Files 755 755
Lines 93227 93253 +26
==========================================
+ Hits 37864 38408 +544
+ Misses 52640 52129 -511
+ Partials 2723 2716 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/nodepool/core/create.go`:
- Around line 198-201: The Get call that checks the HostedCluster payload
currently swallows all errors (client.Get(..., hc) then return nil), which hides
transient API/RBAC issues; update the error handling to only ignore NotFound by
using apierrors.IsNotFound(err) (from k8s.io/apimachinery/pkg/api/errors) — if
IsNotFound(err) return nil, otherwise return the error (or wrap and return it);
ensure you add the import for apierrors and adjust the branch after the
client.Get so logger.Info remains for NotFound but other errors are propagated
instead of being dropped.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 4469-4471: The code currently embeds the raw AWS error text into
the returned error (fmt.Errorf("%w: aws returned an error: %w", wrapped, err)),
which then flows into the ValidOIDCConfiguration status message; change this to
avoid surfacing provider-specific error text by returning a generic provider
error (e.g., fmt.Errorf("%w: aws returned an error", wrapped)) and separately
log the original err to the controller logger (or record it as an event) so the
detailed AWS error is available in logs but not in the status message; update
the site that constructs/returns wrapped (the place where variables wrapped and
err are used with fmt.Errorf) to remove embedding err and ensure you call the
controller logger (or event recorder) to log err.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 567af7d2-b72e-4f37-b4ed-c788a51be81d
📒 Files selected for processing (86)
.golangci.ymlcmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/gcpapi/gcs_client.gosupport/konnectivityproxy/dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/util.gotest/e2e/util/version.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/infra/aws/route53.go`:
- Around line 189-191: The cleanup currently swallows every error from
LookupZone; change it to only ignore the specific "zone not found" case and
return any other error. Update the block around the LookupZone call (id, err :=
LookupZone(ctx, client, name, false)) to inspect err: if it matches the
sentinel/not-found condition returned by LookupZone (or use errors.Is/ a helper
like IsZoneNotFound(err) / AWS's NotFound error check), then continue silently,
otherwise return the error so real Route53/API failures are propagated.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Line 1435: The error return wraps the wrong variable because the outer err
(from platform.GetPlatform) is nil; update the return in the
PlatformCredentialsFound status update failure so only the statusErr is wrapped
with %w and the reconcile error is included with a non-wrapping verb (or capture
the reconcile error into a separate variable). Locate the code around
p.ReconcileCredentials(...) and the fmt.Errorf(...) return and change the
formatting to include the reconcile error without %w (e.g., %v) and use %w only
for statusErr so you don't accidentally wrap a nil outer err.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c16340aa-bb8a-4eb6-91c1-7304d463990f
📒 Files selected for processing (86)
.golangci.ymlcmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/gcpapi/gcs_client.gosupport/konnectivityproxy/dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/util.gotest/e2e/util/version.go
✅ Files skipped from review due to trivial changes (16)
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.go
- etcd-recovery/etcdrecovery.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.go
- test/e2e/util/reqserving/vpa.go
- support/thirdparty/docker/pkg/archive/archive.go
- .golangci.yml
- test/e2e/util/reqserving/verifycp.go
- hypershift-operator/controllers/nodepool/conditions.go
- test/e2e/util/node.go
- hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.go
- test/e2e/util/reqserving/waitfor.go
- control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.go
- karpenter-operator/controllers/karpenter/machine_approver.go
- test/e2e/util/aws.go
- test/e2e/util/oauth.go
openshift-ci
Bot
added
the
area/platform/azure
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
1004-1011:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose HTTP response body in KAS health check.
httpClient.Do(req)response is never closed. In a recurrent reconcile loop this can leak connections and eventually degrade controller reliability.Suggested fix
resp, err := httpClient.Do(req) if err != nil { return err } +defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return fmt.Errorf("APIServer endpoint %s is not healthy", ingressPoint) }🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 1004 - 1011, The HTTP response from httpClient.Do(req) is not closed, which can leak connections; after the successful call to httpClient.Do(req) (the resp variable returned) add proper cleanup by ensuring the response body is consumed/ discarded as needed and closed (e.g., call io.Copy(io.Discard, resp.Body) then defer resp.Body.Close() immediately after checking err) before checking resp.StatusCode for ingressPoint health so connections are returned to the pool.kubernetes-default-proxy/kubernetes_default_proxy.go (1)
70-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the listener on ctx cancellation so
Accept()unblocks and shutdown completes.
net.ListenConfig.Listen(ctx, ...)only usesctxduring the listen setup; cancelingctxdoes not close the returnednet.Listener, so a blockedlistener.Accept()can remain stuck. Trigger shutdown by callinglistener.Close()whenctx.Done()fires, and whenAccept()returns an error, exit cleanly when the error is due to closure (e.g.,net.ErrClosed) / whenctx.Err()!=nil—don’t just log andcontinue.🤖 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 `@kubernetes-default-proxy/kubernetes_default_proxy.go` around lines 70 - 84, The listener created by (&net.ListenConfig{}).Listen(ctx, ...) must be closed when ctx is cancelled so Accept() unblocks: start a goroutine that waits for <-ctx.Done() and calls listener.Close(), and change the Accept() error handling in the accept loop (function/method containing listener, Accept, s.log and ctx) to stop continuing on errors caused by closure—use errors.Is(err, net.ErrClosed) or check ctx.Err()!=nil and then return (or return ctx.Err()) instead of logging and continue; keep logging for unexpected Accept errors only.
🤖 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 `@availability-prober/availability_prober.go`:
- Around line 120-127: The infinite retry loop around creating and sending the
request (the for ; ; time.Sleep(sleepTime) loop that calls
http.NewRequestWithContext(ctx, ...) and client.Do(req)) doesn't stop when ctx
is canceled; update the loop to observe ctx.Done() and exit cleanly: before
sleeping or retrying check if ctx.Err() != nil (or select on ctx.Done()) and
break/return when canceled, and also after a failed client.Do(req) check
ctx.Err() and stop rather than continuing; ensure this change touches the loop
that constructs req and calls client.Do so the goroutine can exit when the
provided context is canceled.
---
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 1004-1011: The HTTP response from httpClient.Do(req) is not
closed, which can leak connections; after the successful call to
httpClient.Do(req) (the resp variable returned) add proper cleanup by ensuring
the response body is consumed/ discarded as needed and closed (e.g., call
io.Copy(io.Discard, resp.Body) then defer resp.Body.Close() immediately after
checking err) before checking resp.StatusCode for ingressPoint health so
connections are returned to the pool.
In `@kubernetes-default-proxy/kubernetes_default_proxy.go`:
- Around line 70-84: The listener created by (&net.ListenConfig{}).Listen(ctx,
...) must be closed when ctx is cancelled so Accept() unblocks: start a
goroutine that waits for <-ctx.Done() and calls listener.Close(), and change the
Accept() error handling in the accept loop (function/method containing listener,
Accept, s.log and ctx) to stop continuing on errors caused by closure—use
errors.Is(err, net.ErrClosed) or check ctx.Err()!=nil and then return (or return
ctx.Err()) instead of logging and continue; keep logging for unexpected Accept
errors only.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 47b1656b-eed1-4f13-beb4-589d2a91e790
📒 Files selected for processing (98)
.golangci.ymlavailability-prober/availability_prober.gocmd/bastion/aws/create.gocmd/cluster/core/create.gocmd/infra/aws/iam.gocmd/infra/aws/iam_policies.gocmd/infra/aws/route53.gocmd/infra/aws/util/errors.gocmd/infra/azure/rbac.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/kubeconfig/create.gocmd/nodepool/core/create.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/dns.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/kas_pki_setup.gocontrol-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/assets.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/kubevirt/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/powervs/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/cno/deployment_init_container_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/ignitionserver/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/auth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/olm/catalogs/deployment.gocontrol-plane-operator/endpoint-resolver/server_test.gocontrol-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/kas/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/registry/admissionpolicies.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/operator/config.gocontrol-plane-operator/metrics-proxy/proxy_test.gocontrol-plane-pki-operator/certificatesigningcontroller/certificatesigningcontroller.gocontrol-plane-pki-operator/targetconfigcontroller/targetconfigcontroller.gocontrol-plane-pki-operator/topology/detector.goetcd-backup/etcdbackup.goetcd-recovery/etcdrecovery.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/proxy/validation.gohypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nto.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.goignition-server/cmd/start.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/tokensecret_controller.gokarpenter-operator/controllers/karpenter/machine_approver.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/main.gokubernetes-default-proxy/kubernetes_default_proxy.gokubevirtexternalinfra/externalinfra.gopkg/etcdcli/etcdcli.gosharedingress-config-generator/controller.gosharedingress-config-generator/controller_test.gosharedingress-config-generator/haproxy_client.gosupport/azureutil/azureutil.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/controlplane-component_test.gosupport/controlplane-component/kubeconfig.gosupport/gcpapi/gcs_client.gosupport/konnectivityproxy/dialer.gosupport/releaseinfo/registryclient/client.gosupport/releaseinfo/releaseinfo.gosupport/supportedversion/version.gosupport/thirdparty/docker/pkg/archive/archive.gosupport/thirdparty/library-go/pkg/image/registryclient/client.gosupport/thirdparty/oc/pkg/cli/image/manifest/manifest.gosupport/util/util.gosupport/validations/authentication.gotest/e2e/util/aws.gotest/e2e/util/dump/dump.gotest/e2e/util/dump/journals.gotest/e2e/util/external_oidc.gotest/e2e/util/node.gotest/e2e/util/oauth.gotest/e2e/util/reqserving/verifycp.gotest/e2e/util/reqserving/verifypods.gotest/e2e/util/reqserving/vpa.gotest/e2e/util/reqserving/waitfor.gotest/e2e/util/util.gotest/e2e/util/version.gotest/integration/framework/hosted-cluster.go
✅ Files skipped from review due to trivial changes (14)
- support/validations/authentication.go
- support/util/util.go
- .golangci.yml
- control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go
- hypershift-operator/controllers/nodepool/apiserver-haproxy/haproxy.go
- test/e2e/util/aws.go
- hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go
- test/e2e/util/reqserving/vpa.go
- etcd-backup/etcdbackup.go
- test/e2e/util/node.go
- karpenter-operator/controllers/karpenter/machine_approver.go
- test/e2e/util/reqserving/verifycp.go
- cmd/nodepool/core/create.go
- cmd/cluster/core/create.go
| for ; ; time.Sleep(sleepTime) { | ||
| response, err := client.Get(target.String()) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) | ||
| if err != nil { | ||
| log.Error(err, "Failed to create request, retrying...") | ||
| continue | ||
| } | ||
| response, err := client.Do(req) | ||
| if err != nil { |
There was a problem hiding this comment.
Handle context cancellation to stop the probe loop.
Line 120 currently retries forever. Once ctx is canceled, Do(req) will keep failing and this loop never exits.
Suggested fix
for ; ; time.Sleep(sleepTime) {
+ select {
+ case <-ctx.Done():
+ log.Info("probe canceled, exiting", "reason", ctx.Err())
+ return
+ default:
+ }
req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil)
if err != nil {
log.Error(err, "Failed to create request, retrying...")
continue
}
response, err := client.Do(req)
if err != nil {
+ if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+ log.Info("probe canceled, exiting", "reason", err)
+ return
+ }
log.Error(err, "Request failed, retrying...")
continue
}As per coding guidelines "Do not leak goroutines — ensure they exit cleanly".
📝 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.
| for ; ; time.Sleep(sleepTime) { | |
| response, err := client.Get(target.String()) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) | |
| if err != nil { | |
| log.Error(err, "Failed to create request, retrying...") | |
| continue | |
| } | |
| response, err := client.Do(req) | |
| if err != nil { | |
| for ; ; time.Sleep(sleepTime) { | |
| select { | |
| case <-ctx.Done(): | |
| log.Info("probe canceled, exiting", "reason", ctx.Err()) | |
| return | |
| default: | |
| } | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil) | |
| if err != nil { | |
| log.Error(err, "Failed to create request, retrying...") | |
| continue | |
| } | |
| response, err := client.Do(req) | |
| if err != nil { | |
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | |
| log.Info("probe canceled, exiting", "reason", err) | |
| return | |
| } | |
| log.Error(err, "Request failed, retrying...") | |
| continue | |
| } |
🤖 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 `@availability-prober/availability_prober.go` around lines 120 - 127, The
infinite retry loop around creating and sending the request (the for ; ;
time.Sleep(sleepTime) loop that calls http.NewRequestWithContext(ctx, ...) and
client.Do(req)) doesn't stop when ctx is canceled; update the loop to observe
ctx.Done() and exit cleanly: before sleeping or retrying check if ctx.Err() !=
nil (or select on ctx.Done()) and break/return when canceled, and also after a
failed client.Do(req) check ctx.Err() and stop rather than continuing; ensure
this change touches the loop that constructs req and calls client.Do so the
goroutine can exit when the provided context is canceled.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
Scheduling tests matching the |
|
/pipeline required |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
The changes are clearly lint-only fixes: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is an infrastructure flake unrelated to PR #8567. The PR only enables additional golangci-lint linters (dupword, durationcheck, errorlint, fatcontext, nilerr, noctx, usestdlibvars) and applies their auto-fixes ( Root CauseThe root cause is GCP/GKE infrastructure instability during this CI run, manifesting as two independent hosted cluster provisioning failures:
Why this is unrelated to PR #8567: The PR changes are exclusively lint-compliance fixes — converting Recommendations
Evidence
|
|
/verified by lint |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
Bot
merged commit eb04f61
into
openshift:main
5 participants
Summary
Enables five new golangci-lint linters and fixes all violations across the codebase:
errors.As/errors.Isinstead of type assertions/direct comparisons on errors, enabling proper wrapped-error detectionerr != nilthen silently returnnil, swallowing errors. Intentional cases (e.g., not-found during cleanup) are suppressed with//nolint:nilerrcommentshttp.NewRequestWithContextinstead ofhttp.NewRequest/http.Get, ensuring HTTP requests respect context cancellationhttp.StatusOK,http.MethodGet) instead of magic literalsAlso adds unit tests covering the behavioral changes introduced by the linter fixes.
Commits
.golangci.ymlconfigurationerrors.As/errors.Ismigration across ~30 files//nolint:nilerrfor legitimate error-swallowing patternshttp.NewRequestWithContextmigrationTest plan
make lintpasses with all new linters enabledmake testpasses (unit tests)errorlint(errors.Aswrapped error detection) andnoctxfixes🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores