OCPBUGS-85428: Lowercase the start of all error messages#6106
OCPBUGS-85428: Lowercase the start of all error messages#6106anandram2 wants to merge 5 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
WalkthroughThis PR standardizes error message formatting and, in some returns, switches to Go %w wrapping across controllers, operator, server, daemon, and test helpers; no control flow, behavior, or public API signatures were changed. ChangesError Message Standardization
🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anandram2 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @anandram2. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/render.go (1)
411-432:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix accidental leading
Lin error messages.Line 411 and Line 432 prepend an unintended
L("LInvalid","LAzure"), which looks like a typo and breaks message standardization.Proposed fix
- return nil, fmt.Errorf("LInvalid AWS Load Balancer Type provided : %s", lbType) + return nil, fmt.Errorf("invalid AWS Load Balancer Type provided : %s", lbType) ... - return nil, fmt.Errorf("LAzure %s Load Balancer IPs unavailable when the DNSType is ClusterHosted", lbType) + return nil, fmt.Errorf("azure %s Load Balancer IPs unavailable when the DNSType is ClusterHosted", lbType)🤖 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/operator/render.go` around lines 411 - 432, In pkg/operator/render.go, fix the typo in the error message strings returned by the logic that handles AWS/Azure load balancer IP state (functions/branches using cloudPlatformLoadBalancerIPState, availableLBIPState, absentLBIPState and the lbType switch): remove the accidental leading "L" from the messages ("LInvalid" -> "Invalid" and "LAzure" -> "Azure") so fmt.Errorf calls produce standardized messages; keep the surrounding logic and return values unchanged.
🧹 Nitpick comments (2)
pkg/operator/sync.go (1)
1157-1157: ⚡ Quick winWrap the create error with
%w.Line 1157 still uses
%v, which prevents error unwrapping and diverges from adjacent%wmigrations.Proposed fix
- return fmt.Errorf("could not make kubeconfig-data CM, %v", err) + return fmt.Errorf("could not make kubeconfig-data CM: %w", 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 `@pkg/operator/sync.go` at line 1157, Replace the non-wrapping fmt.Errorf call that uses "%v" when returning the create error (the statement returning fmt.Errorf("could not make kubeconfig-data CM, %v", err)) with an error wrap using "%w" so callers can unwrap the original error (e.g., produce a message like "could not make kubeconfig-data CM: %w" wrapping err); locate the fmt.Errorf(...) that references kubeconfig-data CM and change the format verb only, preserving the same message text and the err variable.test/extended-priv/machineosconfig.go (1)
131-131: ⚡ Quick winKeep the underlying error in these returned errors.
These new messages drop the original
err, which weakens failure triage. Wrap/include the original error (%w) so callers retain full context.Suggested fix
- return NewMachineOSConfig(oc, name), fmt.Errorf("could not extract dockerConfig from the namespaced pull secret") + return NewMachineOSConfig(oc, name), fmt.Errorf("could not extract dockerConfig from the namespaced pull secret: %w", err) - return NewMachineOSConfig(oc, name), fmt.Errorf("could not extract dockerConfig from the cluster's pull secret") + return NewMachineOSConfig(oc, name), fmt.Errorf("could not extract dockerConfig from the cluster's pull secret: %w", err) - return NewMachineOSConfig(oc, name), fmt.Errorf("could not merge the namespaced pull-secret dockerconfig and the cluster's pull-secret docker config") + return NewMachineOSConfig(oc, name), fmt.Errorf("could not merge the namespaced pull-secret dockerconfig and the cluster's pull-secret docker config: %w", err) - return NewMachineOSConfig(oc, name), fmt.Errorf("could not configure the cluster's pull-secret with the merged dockerconfig") + return NewMachineOSConfig(oc, name), fmt.Errorf("could not configure the cluster's pull-secret with the merged dockerconfig: %w", err) - return nil, fmt.Errorf("error copying the resulting authorization file to local") + return nil, fmt.Errorf("error copying the resulting authorization file to local: %w", err)Also applies to: 137-137, 142-142, 147-147, 437-437
🤖 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 `@test/extended-priv/machineosconfig.go` at line 131, The error returns currently discard the original error; change the fmt.Errorf calls that return NewMachineOSConfig(oc, name), fmt.Errorf("could not extract dockerConfig from the namespaced pull secret") to wrap the underlying err (use fmt.Errorf("...: %w", err)) so the original error is preserved; update the similar return sites (the fmt.Errorf uses at the lines referenced, i.e. the occurrences around the NewMachineOSConfig(oc, name) returns) to wrap the original err with %w in each fmt.Errorf invocation.
🤖 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/controller/kubelet-config/kubelet_config_features.go`:
- Line 166: Update the tombstone error message to reference the correct object
type: change the fmt.Errorf call in the tombstone handler (the
utilruntime.HandleError(...) invocation in kubelet_config_features.go) so it
reports "*osev1.FeatureGate" (or "FeatureGate") instead of "KubeletConfig";
ensure the message text and any surrounding context reflect that this handler
expects a *osev1.FeatureGate to make debugging accurate.
In `@pkg/operator/status.go`:
- Line 585: Two error return strings have an accidental leading "L" in
pkg/operator/status.go; update the fmt.Errorf calls that build messages for
ctrlcommon.ReleaseImageVersionAnnotationKey (using renderedMC.Name and
releaseVersion) and the similar one for the boot image annotation to remove the
stray "L" prefix so the messages start with "Unable..." and "Boot..."
respectively, leaving the rest of the formatting and variables unchanged.
In `@pkg/operator/sync.go`:
- Around line 2126-2131: The error messages returned after calling setGVK
contain a stray leading "L" ("LFailed...") — fix both occurrences by updating
the fmt.Errorf calls that wrap errors from setGVK (the ones handling the infra
object and the dns object) to start with "Failed setting gvk for infra object:
%w" and "Failed setting gvk for dns object: %w" respectively so the messages
read correctly; locate the fmt.Errorf calls surrounding setGVK(dns,
configclientscheme.Scheme) and the equivalent infra object setGVK call and
remove the accidental leading "L".
In `@pkg/server/server.go`:
- Line 282: The error string returned when downgrade detection fails contains a
stray leading "L" ("LCan't serve version %s with ignition
KernelArguments.ShouldNotExist populated"); remove the leading "L" so the
message reads "Can't serve version %s with ignition
KernelArguments.ShouldNotExist populated". Update the return value in the
function that produces this error (the return fmt.Errorf(...) call in server.go)
to use the corrected string.
In `@test/extended-priv/configmap.go`:
- Around line 59-60: The error message in the ConfigMap error return contains an
accidental leading "L" ("LKey ...") — update the string in the return that
builds the error (the fmt.Errorf call referencing key, cm.GetNamespace(),
cm.GetName()) to remove the stray "L" so it reads "Key %s does not exist in the
.data in Configmap -n %s %s"; locate the fmt.Errorf usage in
test/extended-priv/configmap.go that returns the error with variables key and cm
and correct the prefix.
In `@test/extended-priv/containerruntimeconfig.go`:
- Line 63: The error message in the fmt.Errorf call that returns the finalizer
contains a stray leading "L" ("LIt was not possible..."); update the format
string used where fmt.Errorf is called (the line that references cr.GetKind(),
cr.GetName(), cr.PrettyString()) to remove the stray "L" and produce a clean,
correctly capitalized message such as "It was not possible to get the finalizer
from %s %s: %s".
In `@test/extended-priv/controller.go`:
- Line 112: There are two error messages that mistakenly include a duplicated
"LL" prefix in the string (e.g., the fmt.Errorf call that currently builds
"LLCannot get controller pod name. Failed getting MCO controller logs"); locate
those fmt.Errorf invocations in test/extended-priv/controller.go (the
occurrences around the controller pod name retrieval and the later similar
error) and change the message to remove the extra "LL" so it reads "Cannot get
controller pod name. Failed getting MCO controller logs" (and the analogous
message at the second occurrence) ensuring both error strings are consistent.
In `@test/extended-priv/controllerconfig.go`:
- Line 121: Remove the accidental "LL" prefix in the bundle-not-found error
messages: locate the fmt.Errorf calls that construct the message "LLThere is no
image registry bundle with file name %s" (seen at lines near the return in
controllerconfig.go) and change the format string to "There is no image registry
bundle with file name %s" in both occurrences (also the similar one around line
136) so the error reads correctly without the stray "LL".
In `@test/extended-priv/controlplanemachineset.go`:
- Line 371: The returned error strings contain an accidental leading "L" (e.g.
"LFailed to get disks array: %v" and "LNo ...") — remove the stray "L" from
those error messages so they read "Failed to get disks array: %v" and "No ..."
respectively; update the literal error strings wherever "LFailed" or "LNo"
appear (search for those exact substrings) to fix the returned errors in the
function that constructs those fmt.Errorf calls.
In `@test/extended-priv/inside_node_containers.go`:
- Line 76: Replace the accidental leading "L" in error message prefixes so
returned errors use "Error" instead of "LError"; locate the formatting strings
that read e.g. "LError extracting pull-secret. Error: %s" and similar
occurrences noted (also at the other reported spots) and update them to "Error
extracting pull-secret. Error: %s" (and analogous messages) in the functions
that build those errors so all returned fmt.Errorf(...) calls produce
consistently prefixed "Error ..." messages.
In `@test/extended-priv/machineconfigpool.go`:
- Around line 854-855: The error message uses the pool's name instead of the
node's name; update the fmt.Errorf call that currently passes mcp.GetName() to
instead pass node.GetName() so the returned error "error restoring desired
config in node %s" shows the actual node name (locate the fmt.Errorf invocation
near the mcp and node variables and replace the argument to GetName
accordingly).
In `@test/extended-priv/node.go`:
- Around line 1276-1280: The guard after computing availIndex is wrong: it
checks usedIndex instead of availIndex, which can allow availIndex == -1 and
cause a panic when accessing match[availIndex]; update the condition to check
availIndex (< 0) and return the fmt.Errorf with stdout if missing, ensuring the
code around re.SubexpIndex("Avail"), availIndex, match and strconv.ParseInt uses
the corrected guard before using match[availIndex].
- Around line 405-407: The error path that checks "if lenImageSplit < 2" logs
"%s" with err (which is often nil) and thus loses context; change the formatted
message to include the actual image reference string you attempted to parse (the
variable holding the container image value that was split) instead of err, so
the error becomes something like "wrong container-image-reference in deployment:
<imageRef>\n<deployment>" and update the process/error formatting in that branch
(refer to lenImageSplit, err, and deployment to locate the code).
In `@test/extended-priv/util.go`:
- Around line 964-965: Replace the misspelled error message string "no
current-contenxt in the provided kubeconfig" with the correct text "no
current-context in the provided kubeconfig" so the error returned by the
function (the line returning fmt.Errorf(...)) uses the proper `current-context`
spelling for clarity and reliable grepability.
In `@test/helpers/utils.go`:
- Around line 526-527: The error returned from CreateToken is being discarded;
update the error return to wrap the original err so debugging context is
preserved (e.g., change the fmt.Errorf call used where err != nil in the block
referencing CreateToken to include the original error with %w or errors.Wrap).
Locate the clause that checks `if err != nil` after calling CreateToken in
test/helpers/utils.go and modify the return from `fmt.Errorf("could not request
openshift-monitoring token")` to a wrapped error containing `err` (keeping the
same descriptive message) so callers can inspect the underlying cause.
- Around line 622-623: The code currently discards the original error after
calling Get (the err variable) and returns a generic message; update the error
return to wrap the original error so callers retain context (e.g., change the
return that currently uses fmt.Errorf("could not get in-cluster
kube-apiserver-client-ca configmap") to include the original err using
fmt.Errorf("could not get in-cluster kube-apiserver-client-ca configmap: %w",
err)), locating the change around the Get call and the err variable handling.
---
Outside diff comments:
In `@pkg/operator/render.go`:
- Around line 411-432: In pkg/operator/render.go, fix the typo in the error
message strings returned by the logic that handles AWS/Azure load balancer IP
state (functions/branches using cloudPlatformLoadBalancerIPState,
availableLBIPState, absentLBIPState and the lbType switch): remove the
accidental leading "L" from the messages ("LInvalid" -> "Invalid" and "LAzure"
-> "Azure") so fmt.Errorf calls produce standardized messages; keep the
surrounding logic and return values unchanged.
---
Nitpick comments:
In `@pkg/operator/sync.go`:
- Line 1157: Replace the non-wrapping fmt.Errorf call that uses "%v" when
returning the create error (the statement returning fmt.Errorf("could not make
kubeconfig-data CM, %v", err)) with an error wrap using "%w" so callers can
unwrap the original error (e.g., produce a message like "could not make
kubeconfig-data CM: %w" wrapping err); locate the fmt.Errorf(...) that
references kubeconfig-data CM and change the format verb only, preserving the
same message text and the err variable.
In `@test/extended-priv/machineosconfig.go`:
- Line 131: The error returns currently discard the original error; change the
fmt.Errorf calls that return NewMachineOSConfig(oc, name), fmt.Errorf("could not
extract dockerConfig from the namespaced pull secret") to wrap the underlying
err (use fmt.Errorf("...: %w", err)) so the original error is preserved; update
the similar return sites (the fmt.Errorf uses at the lines referenced, i.e. the
occurrences around the NewMachineOSConfig(oc, name) returns) to wrap the
original err with %w in each fmt.Errorf invocation.
🪄 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: ad4c85b8-9e32-454a-bdb8-0b5dafe5a479
📒 Files selected for processing (38)
pkg/controller/bootimage/vsphere_helpers.gopkg/controller/build/osbuildcontroller.gopkg/controller/common/helpers.gopkg/controller/kubelet-config/kubelet_config_controller.gopkg/controller/kubelet-config/kubelet_config_features.gopkg/controller/kubelet-config/kubelet_config_nodes.gopkg/controller/node/node_controller.gopkg/controller/render/render_controller.gopkg/controller/template/render.gopkg/operator/render.gopkg/operator/status.gopkg/operator/sync.gopkg/server/server.gotest/extended-priv/configmap.gotest/extended-priv/containerruntimeconfig.gotest/extended-priv/controller.gotest/extended-priv/controllerconfig.gotest/extended-priv/controlplanemachineset.gotest/extended-priv/gomega_json_matcher.gotest/extended-priv/gomega_matchers.gotest/extended-priv/inside_node_containers.gotest/extended-priv/jsondata.gotest/extended-priv/kubeletconfig.gotest/extended-priv/machine.gotest/extended-priv/machineconfigpool.gotest/extended-priv/machineosbuild.gotest/extended-priv/machineosconfig.gotest/extended-priv/machineset.gotest/extended-priv/mco_bootimages.gotest/extended-priv/mco_scale.gotest/extended-priv/mco_security.gotest/extended-priv/node.gotest/extended-priv/remote_images.gotest/extended-priv/remotefile_gomegamatchers.gotest/extended-priv/resource.gotest/extended-priv/util.gotest/extended-priv/util/vsphere.gotest/helpers/utils.go
|
/retitle OCPBUGS-85428 Lowercase the start of all error messages |
|
@anandram2: Re-titling can only be requested by trusted users, like repository collaborators. 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 kubernetes-sigs/prow repository. |
anandram2
changed the title
|
/retitle OCPBUGS-85428: Lowercase the start of all error messages |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/severity-low
|
@anandram2: This pull request references Jira Issue OCPBUGS-85428, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
anandram2
force-pushed
the
fix-error-message-casing
branch
from
5057fa5 to
deb1e51
Compare
|
/jira refresh |
|
@anandram2: This pull request references Jira Issue OCPBUGS-85428, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@anandram2: This pull request references Jira Issue OCPBUGS-85428, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
/retest-required |
|
@anandram2: This pull request references Jira Issue OCPBUGS-85428, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
Nice job on this! In the interest of completeness, we should probably also search for errors that were constructed using errors.New(). Here's a ripgrep incantation you can use:
$ rg -g '!vendor' --context=0 'errors\.New\(\"[A-Z]'
pkg/daemon/daemon.go
191:var ErrAuxiliary = errors.New("Error from auxiliary packages")
pkg/osimagestream/clusterversion.go
29: return "", errors.New("ClusterVersion desired image is not yet available")
38: return nil, errors.New("ClusterVersion cannot be nil")
47: return nil, errors.New("ClusterVersion has no completed updates in history")
pkg/operator/status_test.go
35: configNotFound := errors.New("Config Not Found")
pkg/controller/bootstrap/bootstrap.go
570: return errors.New("Manifest: UnmarshalJSON on nil pointer")
pkg/controller/common/iri_secret_merger.go
24:var errIRIDisabled = errors.New("IRI not enabled or not present")The only one that I would not change is the one in pkg/controller/common/iri_secret_merger.go since that one is an acronym.
There are also a few CodeRabbit comments where it suggests adding additional context to the error being returned by wrapping the error. Those are pretty easy to resolve by doing something like this:
if err != nil {
return fmt.Errorf("couldn't fulfill request: %w", err)
}
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/apiserver-watcher/run.go`:
- Line 113: The returned error message contains a typo: change the string
"failed to start heath checker" to "failed to start health checker" in the error
return (the fmt.Errorf call shown in cmd/apiserver-watcher/run.go), so the
returned error reads "failed to start health checker: %w" and preserves the
wrapped 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 32564bfd-6b55-42bd-8173-02c53d0af160
📒 Files selected for processing (12)
cmd/apiserver-watcher/run.gopkg/controller/bootimage/cache/cache.gopkg/controller/bootstrap/bootstrap.gopkg/controller/container-runtime-config/helpers.gopkg/controller/drain/drain_controller.gopkg/controller/kubelet-config/kubelet_config_controller.gopkg/daemon/certificate_writer.gopkg/daemon/daemon.gopkg/operator/status_test.gopkg/operator/sync.gopkg/osimagestream/clusterversion.gotest/extended-priv/machineosconfig.go
✅ Files skipped from review due to trivial changes (2)
- pkg/osimagestream/clusterversion.go
- pkg/operator/status_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/kubelet-config/kubelet_config_controller.go
- test/extended-priv/machineosconfig.go
|
|
||
| if err := h.Start(); err != nil { | ||
| return fmt.Errorf("failed to start heath checker: %v", err) | ||
| return fmt.Errorf("failed to start heath checker: %w", err) |
There was a problem hiding this comment.
Fix typo in error text ("heath" → "health").
Line 113 has a typo in the returned error message, which makes troubleshooting output less clear.
Suggested patch
- return fmt.Errorf("failed to start heath checker: %w", err)
+ return fmt.Errorf("failed to start health checker: %w", 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/apiserver-watcher/run.go` at line 113, The returned error message
contains a typo: change the string "failed to start heath checker" to "failed to
start health checker" in the error return (the fmt.Errorf call shown in
cmd/apiserver-watcher/run.go), so the returned error reads "failed to start
health checker: %w" and preserves the wrapped err.
|
@anandram2: The following tests 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. |
4 participants
- What I did
Lowercased the first letter of error messages to comply with Staticcheck rule ST1005.
- How to verify it
Run
staticcheck -checks ST1005 ./...- Description for the changelog
Lowercased first letter of all error messages. Fixes: OCPBUGS-85428
Summary by CodeRabbit
Style
Bug Fixes