Repository navigation
Enable errorlint for error comparisons and type assertions - #7084
Merged
Merged
Conversation
Compare errors with errors.Is and unwrap them with errors.AsType, so the checks keep working if the error is wrapped later. errorf is disabled: formatting errors with %v is often deliberate. Co-authored-by: Isaac <no-reply@databricks.com>
github-actions
Bot
added
DABs
Co-authored-by: Isaac <no-reply@databricks.com>
denik
marked this pull request as ready for review
October 16, 2026 08:46
denik
enabled auto-merge
October 16, 2026 08:46
shreyas-goenka
approved these changes
Oct 16, 2026
Collaborator
Integration test reportCommit: 753f11d
Top 6 slowest tests (at least 2 minutes):
|
arushiagarwal-db
pushed a commit
to arushiagarwal-db/cli
that referenced
this pull request
Oct 19, 2026
…s#7090) ## Changes Turn on errorlint's `errorf` check and switch 29 `fmt.Errorf` calls from `%v`/`%s` to `%w`. Seven calls keep `%v` with a `//nolint:errorlint` reason, because wrapping would change behavior: - `bundle/config/mutator/set_variables.go` (4): `setVariable` runs inside `dyn.Map`, which returns the original value with no error when the callback fails with a no-such-key or cannot-traverse-nil error. A wrapped `dyn.Get(v, "default")` failure would leave the variable unset without any error. - `libs/sync/snapshot.go` snapshot write: `bundle/deploy/files/upload.go` reports `fs.ErrPermission` from `RunOnce` as 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 databricks#7084, which left `errorf` off pending per-site review. For each converted site I checked the error's path up to the command for `errors.Is`/`errors.As` that could now match the cause. The global handlers only inspect `root.ErrAlreadyPrinted` and `*apierr.APIError` (in `diag.FromErr`), and none of these causes can contain an API error. ## Tests The error text is identical under `%v` and `%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 under `bundle/variables`, `bundle/artifacts`, `bundle/python` and `bundle/destroy` pass. This pull request and its description were written by Isaac. Co-authored-by: Isaac <no-reply@databricks.com>
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
Enable
errorlintwith only its comparison and type-assertion checks, and fix its 29 findings:err == ErrXbecomeserrors.Is(err, ErrX)anderr.(*T)becomeserrors.AsType[*T](err). Theerrorfcheck (%v→%winfmt.Errorf) stays off: wrapping makes the cause visible to everyerrors.Is/errors.Asup the stack (e.g.fs.ErrNotExist,context.Canceled), so converting its 36 sites is a behavior change that needs per-site review in a separate PR.Why
These checks break silently as soon as the error is wrapped with
%w. No current site is broken, since every sentinel is still returned unwrapped, but the codebase was inconsistent: for examplecmd/bundle/validate.gocomparedroot.ErrAlreadyPrintedwith!=whilecmd/bundle/utils/process.gouseserrors.Is.Tests
golangci-lint reports 0 issues on the root and tools modules. Unit tests pass for every changed package, and the
cmd/andbundle/validate/acceptance tests pass. One test keeps its!=with a//nolint:errorlint, because it checks thatmapUploadErrorreturns the same error rather than a wrapper.This pull request and its description were written by Isaac.