Repository navigation
Conversation
arloc
force-pushed
the
main
branch
2 times, most recently
from
September 25, 2026 11:55
ea4ca8e to
2d7fd63
Compare
- Replace the `allEmpty(ch.Old, ch.New, ch.Remote)` call in
`addPerFieldActions` with `allEmptyChange(ch)`: `Old`/`Remote` keep the
permissive `isEmpty` (a zero-ish backend echo for a field nobody asked
to change still counts as empty), but `New` is checked strictly — a
concrete scalar zero/false there already survived structdiff's
ForceSendFields-aware nil substitution, so it is a real, explicitly
force-sent value and must not be classified as empty.
- Add three regression tests in bundle_plan_test.go:
- TestScalarZeroIsNotEmpty: the fix itself, gcp_attributes.local_ssd_count
set to 0 for the first time is detected as an update.
- TestUnsetScalarWithBackendEchoIsStillEmpty: no-regression guard, an
unrelated field the config never sets (e.g. timeout_seconds) whose
remote echoes a scalar zero stays a no-op.
- TestRemovingManagedOverrideMatchingRemoteIsEmpty: documents intentional
behavior - removing a managed override when remote already matches
the last explicit value stays a no-op (gcp_attributes is
ignore_remote_changes: managed, so the tool never actively resets it).
- Add a Cloud=false acceptance test under
acceptance/bundle/resources/clusters/deploy/local_ssd_count_update
covering the update path; the existing local_ssd_count test only
covers create.
Direct-engine `bundle plan` silently dropped an update that explicitly
forces `gcp_attributes.local_ssd_count: 0` on a cluster/job previously
deployed without the field. `isEmpty()`'s blanket `IsZero() -> true`
shortcut conflated a real, force-sent `0` with an unset field, so the
change was classified `ReasonEmpty` before ever reaching the
`ignore_remote_changes: managed` rule for gcp_attributes. GCP doesn't
reliably echo local_ssd_count back on GET, so the field never converged
even across repeated deploys. Reported against a real GCP workspace;
see databricks/terraform-provider-databricks#4089 for the same
underlying backend quirk in Terraform.
- `go test ./bundle/direct/...`
- `go test ./bundle/... ./libs/structs/...`
- New acceptance test (generated output.txt/out.test.toml)
- Manually verified against a real GCP workspace: setting the field,
unrelated fields (disabled, timeout_seconds), and removing the
override all now behave correctly.
denik
previously approved these changes
Sep 28, 2026
| return false | ||
| } | ||
| if ch.New == nil { | ||
| return true |
Contributor
There was a problem hiding this comment.
Does it cover only direction from empty to 0?
Should we also recognize the other direction - from 0 to absent?
Contributor
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Contributor
|
Thanks for raising the PR! For contributing to Databricks CLI we require users to sign CLA, if that's something you are willing to do, please drop an email with a request to dabs-feedback@databricks.com |
denik
enabled auto-merge
September 28, 2026 15:30
Contributor
auto-merge was automatically disabled
September 29, 2026 10:11
Pull request was closed
philip
pushed a commit
to philip/databricks-cli
that referenced
this pull request
Sep 30, 2026
…6867) ## Changes The direct engine treated an integer `0` as empty, so an explicitly configured `gcp_attributes.local_ssd_count: 0` (or any integer zero) added to a resource first deployed without the field was classified as an empty no-op and never applied — subsequent deploys detected no change. A change is now skipped as empty only when it is not a genuine local change: an integer the config force-sent that differs from the prior state is applied, while a backend-echoed zero or an unchanged value stays a no-op. Related: - original report & fix databricks#6825 - databricks/terraform-provider-databricks#4089 ## Tests - New acceptance test `acceptance/bundle/resources/clusters/deploy/local_ssd_count_update`: create without the field, add `local_ssd_count: 0` (detected and applied), then a no-op third deploy that converges. 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
2 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.
direct: don't treat an explicit scalar zero as an empty no-op change
Changes
allEmpty(ch.Old, ch.New, ch.Remote)call inaddPerFieldActionswithallEmptyChange(ch):Old/Remotekeep the permissiveisEmpty(a zero-ish backend echo for a field nobody asked to change still counts as empty), butNewis checked strictly — a concrete scalar zero/false there already survived structdiff's ForceSendFields-aware nil substitution, so it is a real, explicitly force-sent value and must not be classified as empty.remote echoes a scalar zero stays a no-op.
Why
Direct-engine
bundle plansilently dropped an update that explicitly forcesgcp_attributes.local_ssd_count: 0on a cluster/job previously deployed without the field.isEmpty()'s blanketIsZero() -> trueshortcut conflated a real, force-sent0with an unset field, so the change was classifiedReasonEmptybefore ever reaching theignore_remote_changes: managedrule for gcp_attributes. GCP doesn't reliably echo local_ssd_count back on GET, so the field never converged even across repeated deploys. Reported against a real GCP workspace; see databricks/terraform-provider-databricks#4089 for the same underlying backend quirk in Terraform.Tests
go test ./bundle/direct/...go test ./bundle/... ./libs/structs/...