fix: use %w for error wrapping and lowercase error messages#5180
fix: use %w for error wrapping and lowercase error messages#5180sebrandon1 wants to merge 1 commit intocrc-org:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
WalkthroughStandardized error handling across multiple packages: replaced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (3)
pkg/crc/cluster/kubeadmin_password.go (1)
63-63: Prefer context-first error text with wrapped error at the end.Line 63 wraps correctly, but
"%w: %s"makes the rendered message harder to read. Prefer placing stderr before the wrapped error (and trim stderr) for cleaner diagnostics.♻️ Suggested adjustment
- return fmt.Errorf("failed to update user passwords %w: %s", err, stderr) + return fmt.Errorf("failed to update user passwords: %s: %w", strings.TrimSpace(stderr), err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/cluster/kubeadmin_password.go` at line 63, Change the error construction so the human-readable context comes first, include a trimmed stderr before the wrapped error, and ensure stderr is trimmed (import strings if needed); replace the current return fmt.Errorf("failed to update user passwords %w: %s", err, stderr) with something like fmt.Errorf("failed to update user passwords: %s: %w", strings.TrimSpace(stderr), err) so the message is cleaner and the wrapped error remains at the end.pkg/crc/preflight/preflight_checks_linux.go (2)
187-194: Inconsistent error handling within the same function.Line 189 was updated to use lowercase and
%w, but line 193 still uses uppercase "Unable" and%v. For consistency, consider updating line 193 as well.♻️ Suggested fix for consistency
supportedLibvirtVersion, err := semver.NewVersion(minSupportedLibvirtVersion) if err != nil { - return fmt.Errorf("Unable to parse %s libvirt version %v", minSupportedLibvirtVersion, err) + return fmt.Errorf("unable to parse %s libvirt version: %w", minSupportedLibvirtVersion, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/preflight/preflight_checks_linux.go` around lines 187 - 194, The error handling is inconsistent: change the second semver.NewVersion error return for supportedLibvirtVersion to match the style used for installedLibvirtVersion by using a lowercase message and %w for error wrapping; locate the call to semver.NewVersion(minSupportedLibvirtVersion) and update its return to fmt.Errorf("unable to parse %s libvirt version: %w", minSupportedLibvirtVersion, err) so messages and wrapping are consistent across installedLibvirtVersion and supportedLibvirtVersion.
567-570: Unconventional error format with%wfollowed by%s.The format
%w - %splaces the wrapped error before the stderr content. While technically correct, a more conventional pattern would put contextual information before the wrapped error:- return fmt.Errorf("failed to create libvirt 'crc' network: %w - %s", err, buf.String()) + return fmt.Errorf("failed to create libvirt 'crc' network: %s: %w", buf.String(), err)This produces cleaner error chains like
"failed to create...: <stderr>: <underlying error>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/preflight/preflight_checks_linux.go` around lines 567 - 570, The error formatting places the wrapped error before the stderr output; update the fmt.Errorf call in the libvirt 'crc' network creation error path to put the stderr context first and then wrap the underlying error (e.g., change "failed to create libvirt 'crc' network: %w - %s" to "failed to create libvirt 'crc' network: %s: %w" using buf.String() then err) while keeping the existing logging.Debugf call unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/crc/network/nameservers.go`:
- Line 61: The error message returned when writing the nameserver file uses the
wrong destination path string; update the fmt.Errorf call in the function that
writes the resolv file (look for the writer that returns fmt.Errorf("error
creating /etc/resolv on instance: %w", err)) to reference "/etc/resolv.conf"
instead of "/etc/resolv" so the error accurately reflects the target file path.
---
Nitpick comments:
In `@pkg/crc/cluster/kubeadmin_password.go`:
- Line 63: Change the error construction so the human-readable context comes
first, include a trimmed stderr before the wrapped error, and ensure stderr is
trimmed (import strings if needed); replace the current return
fmt.Errorf("failed to update user passwords %w: %s", err, stderr) with something
like fmt.Errorf("failed to update user passwords: %s: %w",
strings.TrimSpace(stderr), err) so the message is cleaner and the wrapped error
remains at the end.
In `@pkg/crc/preflight/preflight_checks_linux.go`:
- Around line 187-194: The error handling is inconsistent: change the second
semver.NewVersion error return for supportedLibvirtVersion to match the style
used for installedLibvirtVersion by using a lowercase message and %w for error
wrapping; locate the call to semver.NewVersion(minSupportedLibvirtVersion) and
update its return to fmt.Errorf("unable to parse %s libvirt version: %w",
minSupportedLibvirtVersion, err) so messages and wrapping are consistent across
installedLibvirtVersion and supportedLibvirtVersion.
- Around line 567-570: The error formatting places the wrapped error before the
stderr output; update the fmt.Errorf call in the libvirt 'crc' network creation
error path to put the stderr context first and then wrap the underlying error
(e.g., change "failed to create libvirt 'crc' network: %w - %s" to "failed to
create libvirt 'crc' network: %s: %w" using buf.String() then err) while keeping
the existing logging.Debugf call unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e996c14-603c-4eff-9244-f1e3e3375a50
📒 Files selected for processing (18)
pkg/crc/cluster/cluster.gopkg/crc/cluster/csr.gopkg/crc/cluster/kubeadmin_password.gopkg/crc/gpg/gpg.gopkg/crc/machine/bundle/metadata.gopkg/crc/machine/exists.gopkg/crc/machine/machine.gopkg/crc/machine/start.gopkg/crc/manpages/manpages_unix.gopkg/crc/network/nameservers.gopkg/crc/preflight/preflight_checks_linux.gopkg/crc/preflight/preflight_checks_network_linux.gopkg/crc/preflight/preflight_windows.gopkg/crc/ssh/client.gopkg/crc/ssh/keys.gopkg/crc/validation/validation.gopkg/drivers/vfkit/driver_darwin.gopkg/libmachine/host/migrate.go
| err = sshRunner.CopyDataPrivileged([]byte(resolvFile), "/etc/resolv.conf", 0644) | ||
| if err != nil { | ||
| return fmt.Errorf("Error creating /etc/resolv on instance: %s", err.Error()) | ||
| return fmt.Errorf("error creating /etc/resolv on instance: %w", err) |
There was a problem hiding this comment.
Fix the destination path in the error message.
Line [61] says "/etc/resolv" but this function writes to "/etc/resolv.conf". The current text can mislead operators during incident triage.
Suggested patch
- return fmt.Errorf("error creating /etc/resolv on instance: %w", err)
+ return fmt.Errorf("error creating /etc/resolv.conf on instance: %w", err)📝 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.
| return fmt.Errorf("error creating /etc/resolv on instance: %w", err) | |
| return fmt.Errorf("error creating /etc/resolv.conf on instance: %w", err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/crc/network/nameservers.go` at line 61, The error message returned when
writing the nameserver file uses the wrong destination path string; update the
fmt.Errorf call in the function that writes the resolv file (look for the writer
that returns fmt.Errorf("error creating /etc/resolv on instance: %w", err)) to
reference "/etc/resolv.conf" instead of "/etc/resolv" so the error accurately
reflects the target file path.
There was a problem hiding this comment.
This is worth fixing while you are changing this line.
There was a problem hiding this comment.
@cfergeau, agreed — since the line is already being touched in this PR, it's a natural opportunity to correct the path. @sebrandon1, please apply the following one-character fix:
- return fmt.Errorf("error creating /etc/resolv on instance: %w", err)
+ return fmt.Errorf("error creating /etc/resolv.conf on instance: %w", err)This ensures the error message accurately reflects /etc/resolv.conf — the actual destination file — which is important for quick diagnosis during incident triage.
|
/ok-to-test |
These nitpick comments are worth looking at. |
Replace %v/%s with %w in fmt.Errorf calls across the codebase to preserve error chains for errors.Is()/errors.As(). Also lowercase error message prefixes per Go conventions.
sebrandon1
force-pushed
the
fix/consistent-error-wrapping
branch
from
641c33a to
48e5679
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/crc/network/nameservers.go (1)
61-61:⚠️ Potential issue | 🟡 MinorFix incorrect destination path in error text.
Line [61] still says
/etc/resolv, but this write targets/etc/resolv.conf.Suggested patch
- return fmt.Errorf("error creating /etc/resolv on instance: %w", err) + return fmt.Errorf("error creating /etc/resolv.conf on instance: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/network/nameservers.go` at line 61, The error message contains the wrong path string; update the returned error in the code that writes the resolver file so it references "/etc/resolv.conf" instead of "/etc/resolv" (replace the fmt.Errorf call that currently says "error creating /etc/resolv on instance: %w" so it reads "error creating /etc/resolv.conf on instance: %w"); this change should be made where the resolver file write/creation is handled (the function returning that fmt.Errorf in pkg/crc/network/nameservers.go).
🧹 Nitpick comments (1)
pkg/crc/cluster/cluster.go (1)
171-172: Refactor error formatting for consistency and clarity.All 12 instances in this file use
%w: %s(error then stderr) while other parts of the codebase prefer...: %s: %w(stderr context first, wrapped error last). Additionally, consider wrapping stderr withstrings.TrimSpace()to avoid extra whitespace in error messages.Affected lines: 171, 182, 194, 216, 228, 245, 262, 289, 321, 334, 347, 398
Example refactoring
- return fmt.Errorf("failed to get machine configs %w: %s", err, stderr) + return fmt.Errorf("failed to get machine configs: %s: %w", strings.TrimSpace(stderr), err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/cluster/cluster.go` around lines 171 - 172, Update the 12 error return sites in pkg/crc/cluster/cluster.go (currently using fmt.Errorf("... %w: %s", err, stderr)) to follow the codebase convention of placing stderr/context first and the wrapped error last, and trim stderr whitespace; e.g., replace the format with "...: %s: %w" and wrap the stderr value with strings.TrimSpace(...) and wrap the original error with %w so callers can unwrap the underlying error (affects the return statements referenced in the comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/crc/network/nameservers.go`:
- Line 61: The error message contains the wrong path string; update the returned
error in the code that writes the resolver file so it references
"/etc/resolv.conf" instead of "/etc/resolv" (replace the fmt.Errorf call that
currently says "error creating /etc/resolv on instance: %w" so it reads "error
creating /etc/resolv.conf on instance: %w"); this change should be made where
the resolver file write/creation is handled (the function returning that
fmt.Errorf in pkg/crc/network/nameservers.go).
---
Nitpick comments:
In `@pkg/crc/cluster/cluster.go`:
- Around line 171-172: Update the 12 error return sites in
pkg/crc/cluster/cluster.go (currently using fmt.Errorf("... %w: %s", err,
stderr)) to follow the codebase convention of placing stderr/context first and
the wrapped error last, and trim stderr whitespace; e.g., replace the format
with "...: %s: %w" and wrap the stderr value with strings.TrimSpace(...) and
wrap the original error with %w so callers can unwrap the underlying error
(affects the return statements referenced in the comment).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d23c1bd-cc71-4af0-98aa-dad943a0e6b9
📒 Files selected for processing (18)
pkg/crc/cluster/cluster.gopkg/crc/cluster/csr.gopkg/crc/cluster/kubeadmin_password.gopkg/crc/gpg/gpg.gopkg/crc/machine/bundle/metadata.gopkg/crc/machine/exists.gopkg/crc/machine/machine.gopkg/crc/machine/start.gopkg/crc/manpages/manpages_unix.gopkg/crc/network/nameservers.gopkg/crc/preflight/preflight_checks_linux.gopkg/crc/preflight/preflight_checks_network_linux.gopkg/crc/preflight/preflight_windows.gopkg/crc/ssh/client.gopkg/crc/ssh/keys.gopkg/crc/validation/validation.gopkg/drivers/vfkit/driver_darwin.gopkg/libmachine/host/migrate.go
✅ Files skipped from review due to trivial changes (4)
- pkg/crc/machine/exists.go
- pkg/crc/machine/machine.go
- pkg/crc/gpg/gpg.go
- pkg/crc/ssh/client.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/crc/manpages/manpages_unix.go
- pkg/crc/ssh/keys.go
- pkg/crc/preflight/preflight_checks_network_linux.go
- pkg/drivers/vfkit/driver_darwin.go
- pkg/libmachine/host/migrate.go
- pkg/crc/cluster/kubeadmin_password.go
- pkg/crc/cluster/csr.go
- pkg/crc/preflight/preflight_checks_linux.go
- pkg/crc/machine/start.go
3 participants
Summary
%v/%swith%winfmt.Errorfcalls across 18 files to preserve error chains forerrors.Is()/errors.As()"Error reading..."→"error reading...")cluster.goerror messageWhy
%w?Per the Go error handling best practices,
fmt.Errorfwith%wwraps the original error so callers can inspect the error chain usingerrors.Is()anderrors.As(). Using%vor%sinstead converts the error to a string, breaking the chain and making it impossible for callers to programmatically match specific error types.See also: Go Code Review Comments — error strings for the lowercase convention.
Affected packages
pkg/crc/machine—machine.go,exists.go,start.go,bundle/metadata.gopkg/crc/cluster—cluster.go,csr.go,kubeadmin_password.gopkg/crc/network—nameservers.gopkg/crc/ssh—client.go,keys.gopkg/crc/gpg—gpg.gopkg/crc/validation—validation.gopkg/crc/preflight—preflight_checks_linux.go,preflight_checks_network_linux.go,preflight_windows.gopkg/crc/manpages—manpages_unix.gopkg/drivers/vfkit—driver_darwin.gopkg/libmachine/host—migrate.goTest plan
make lintpasses with 0 issuesSummary by CodeRabbit