Repository navigation
Conversation
Enable errorlint's errorf check. Convert the fmt.Errorf calls whose wrapped cause cannot be matched by any errors.Is/As on the path up to the command. Keep %v, with a nolint reason, where wrapping would change behavior: - set_variables runs inside dyn.Map, which returns no error when the callback error is a no-such-key/cannot-traverse-nil error, so wrapping would silently leave the variable unset. - The sync snapshot write: files.Upload reports fs.ErrPermission as missing workspace permissions, but this error comes from a local file. - The two installer errors that carry a secondary failure next to %w. Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 8d84b3f
Top 6 slowest tests (at least 2 minutes):
|
| return fmt.Errorf("failed to json marshal in-memory snapshot: %w", err) | ||
| } | ||
| if err := atomicfile.Write(s.snapshotPath, bytes, 0o644); err != nil { | ||
| return fmt.Errorf("failed to write sync snapshot to disk: %s", err) |
Contributor
There was a problem hiding this comment.
why now use %w here?
shreyas-goenka
approved these changes
Oct 16, 2026
This branch has not been deployed
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
3 participants
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.
Changes
Turn on errorlint's
errorfcheck and switch 29fmt.Errorfcalls from%v/%sto%w. Seven calls keep%vwith a//nolint:errorlintreason, because wrapping would change behavior:bundle/config/mutator/set_variables.go(4):setVariableruns insidedyn.Map, which returns the original value with no error when the callback fails with a no-such-key or cannot-traverse-nil error. A wrappeddyn.Get(v, "default")failure would leave the variable unset without any error.libs/sync/snapshot.gosnapshot write:bundle/deploy/files/upload.goreportsfs.ErrPermissionfromRunOnceas missing workspace permissions, but this is a local file.libs/aitools/installer/installer.go(2): a secondary failure next to the primary%w.Why
Follow-up to #7084, which left
errorfoff pending per-site review. For each converted site I checked the error's path up to the command forerrors.Is/errors.Asthat could now match the cause. The global handlers only inspectroot.ErrAlreadyPrintedand*apierr.APIError(indiag.FromErr), and none of these causes can contain an API error.Tests
The error text is identical under
%vand%w, so no golden files change. golangci-lint reports 0 issues on the root and tools modules; unit tests pass for all changed packages; acceptance tests underbundle/variables,bundle/artifacts,bundle/pythonandbundle/destroypass.This pull request and its description were written by Isaac.