From 23ac1f9b4edabf6cd707649648084496d06b6362 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Thu, 23 Jul 2026 12:53:39 +0000 Subject: [PATCH 1/7] Fix spurious grants create for empty grants list on direct engine An empty grants: [] list produced no databricks_grants resource in terraform state, so bundle deployment migrate wrote no grants entry. The direct engine's makePlan then always emitted a plan node for the grants config node, so bundle plan showed a spurious 'create' for the empty grants node -- contradicting migrate's 'no actions planned'. Skip an empty grants node in makePlan when there is no existing state entry. When a state entry exists (grants were deployed and are now being emptied), the node is still emitted so the revoke is planned. --- .nextchanges/bundles/empty-grants-migrate.md | 1 + bundle/direct/bundle_plan.go | 13 +++++++++++++ 2 files changed, 14 insertions(+) create mode 100644 .nextchanges/bundles/empty-grants-migrate.md diff --git a/.nextchanges/bundles/empty-grants-migrate.md b/.nextchanges/bundles/empty-grants-migrate.md new file mode 100644 index 00000000000..121aa2b4c1f --- /dev/null +++ b/.nextchanges/bundles/empty-grants-migrate.md @@ -0,0 +1 @@ +Fixed the direct deployment engine planning a spurious `create` for a resource's empty `grants: []` list. An empty grants list with no existing state entry is now a no-op, so `bundle plan` reports no actions after `bundle deployment migrate` (Terraform never records a grants resource for an empty list). diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 139012423ea..d074be52814 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -961,6 +961,19 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root if err != nil { return nil, err } + // An empty grants list with no state entry is a no-op: there is nothing + // to create and, unlike a non-empty-to-empty edit, no principals to revoke. + // Terraform never writes a databricks_grants resource for an empty list, so + // after "bundle deployment migrate" there is no state entry to match against; + // emitting a plan node here would show a spurious "create" and contradict + // migrate's promise of no actions planned. When a state entry does exist + // (grants were deployed and are now being emptied), keep the node so the + // revoke is still planned. + if gs, ok := inputConfigStructVar.Value.(*dresources.GrantsState); ok && len(gs.EmbeddedSlice) == 0 { + if _, hasState := db.State[node]; !hasState { + continue + } + } inputConfig = inputConfigStructVar.Value baseRefs = inputConfigStructVar.Refs } From e4d8d20adcea278ee4b7b01051847c17453a1667 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Thu, 23 Jul 2026 13:01:53 +0000 Subject: [PATCH 2/7] Shorten comments in empty-grants fix --- bundle/direct/bundle_plan.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index d074be52814..07ff3257588 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -961,14 +961,9 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root if err != nil { return nil, err } - // An empty grants list with no state entry is a no-op: there is nothing - // to create and, unlike a non-empty-to-empty edit, no principals to revoke. - // Terraform never writes a databricks_grants resource for an empty list, so - // after "bundle deployment migrate" there is no state entry to match against; - // emitting a plan node here would show a spurious "create" and contradict - // migrate's promise of no actions planned. When a state entry does exist - // (grants were deployed and are now being emptied), keep the node so the - // revoke is still planned. + // Terraform writes no grants resource for an empty list, so migrate leaves + // no state entry. Skip the node here too; otherwise plan shows a spurious + // "create". Keep it when state exists so emptying grants still revokes. if gs, ok := inputConfigStructVar.Value.(*dresources.GrantsState); ok && len(gs.EmbeddedSlice) == 0 { if _, hasState := db.State[node]; !hasState { continue From 5a47a082f2145f0943eaf7ba7121f64ad9ab731a Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Fri, 24 Jul 2026 07:31:02 +0000 Subject: [PATCH 3/7] acc: re-enable empty-grants invariant migrate variant With the direct-engine fix in place, the migrate invariant no longer shows a spurious grants -> create for an empty grants: [] list, so remove the no_empty_grants exclude added by the invariant-config PR and regenerate the migrate matrix. This is the variant the fuzz-found config was disabled for. --- acceptance/bundle/invariant/migrate/out.test.toml | 1 + acceptance/bundle/invariant/migrate/test.toml | 11 ----------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/acceptance/bundle/invariant/migrate/out.test.toml b/acceptance/bundle/invariant/migrate/out.test.toml index 0ac1b789d6f..469a1c81839 100644 --- a/acceptance/bundle/invariant/migrate/out.test.toml +++ b/acceptance/bundle/invariant/migrate/out.test.toml @@ -34,6 +34,7 @@ EnvMatrix.INPUT_CONFIG = [ "postgres_synced_table.yml.tmpl", "registered_model.yml.tmpl", "schema.yml.tmpl", + "schema_empty_grants.yml.tmpl", "schema_uppercase_name.yml.tmpl", "secret_scope.yml.tmpl", "secret_scope_default_backend_type.yml.tmpl", diff --git a/acceptance/bundle/invariant/migrate/test.toml b/acceptance/bundle/invariant/migrate/test.toml index 37dc2557922..a91f990df3b 100644 --- a/acceptance/bundle/invariant/migrate/test.toml +++ b/acceptance/bundle/invariant/migrate/test.toml @@ -26,17 +26,6 @@ EnvMatrixExclude.no_cross_resource_ref = ["INPUT_CONFIG=job_cross_resource_ref.y # Grant cross-references require the EmbeddedSlice pattern not present in terraform mode. EnvMatrixExclude.no_grant_ref = ["INPUT_CONFIG=schema_grant_ref.yml.tmpl"] -# An empty grants list plans a spurious create after migrate: Terraform records no -# databricks_grants resource for grants: [], so migrate leaves no state entry for the -# grants node, but the direct plan emits a "create" for it anyway. Found by fuzz testing. -# The plan check fails with: -# Unexpected action='create' for resources.schemas.foo.grants -# ... -# "resources.schemas.foo.grants": { "action": "create", ... } -# Exit code: 10 -# Fixed by https://github.com/databricks/cli/pull/6039; re-enable once that lands. -EnvMatrixExclude.no_empty_grants = ["INPUT_CONFIG=schema_empty_grants.yml.tmpl"] - # SQL warehouses currently failing with migration with permanent drift. TODO: fix this. EnvMatrixExclude.no_sql_warehouse = ["INPUT_CONFIG=sql_warehouse.yml.tmpl"] From 661aa4e812e2bf23e507f7e7e66af30630db59db Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Mon, 27 Jul 2026 07:05:35 +0000 Subject: [PATCH 4/7] Move empty-grants skip behind an optional SkipCreate resource method The planner asserted on *dresources.GrantsState directly to detect an empty grants list. Add an optional SkipCreate method to IResource so the predicate lives with the resource, and have makePlan consult it through the adapter. The state lookup stays in the planner: a node that already has state must keep its plan entry even when creating it would be a no-op, otherwise it falls into the delete branch and grants' no-op DoDelete would silently stop revoking. --- bundle/direct/bundle_plan.go | 20 ++++++---- bundle/direct/dresources/README.md | 4 ++ bundle/direct/dresources/adapter.go | 31 +++++++++++++++ bundle/direct/dresources/grants.go | 7 ++++ bundle/direct/dresources/grants_test.go | 53 +++++++++++++++++++++++++ 5 files changed, 107 insertions(+), 8 deletions(-) diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 07ff3257588..ac62e57049f 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -961,14 +961,6 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root if err != nil { return nil, err } - // Terraform writes no grants resource for an empty list, so migrate leaves - // no state entry. Skip the node here too; otherwise plan shows a spurious - // "create". Keep it when state exists so emptying grants still revokes. - if gs, ok := inputConfigStructVar.Value.(*dresources.GrantsState); ok && len(gs.EmbeddedSlice) == 0 { - if _, hasState := db.State[node]; !hasState { - continue - } - } inputConfig = inputConfigStructVar.Value baseRefs = inputConfigStructVar.Refs } @@ -978,6 +970,18 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root return nil, fmt.Errorf("%s: %w", prefix, err) } + // A node that already has state must stay in the plan even when creating it would be + // a no-op, otherwise emptying it would plan no action at all. + if _, hasState := db.State[node]; !hasState { + skip, err := adapter.SkipCreate(newStateConfig) + if err != nil { + return nil, fmt.Errorf("%s: %w", prefix, err) + } + if skip { + continue + } + } + // Note, we're extracting references in input config but resolving them in newState.Config which is PrepareState(inputConfig) // This means input and state must be compatible: input can have more fields, but existing fields should not be moved // This means one cannot refer to fields not present in state (e.g. ${resources.jobs.foo.permissions}) diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index da9e9edab28..fc85b49a755 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -42,6 +42,10 @@ For resources whose create or update is asynchronous (the resource is not immedi If the API may return a slice's elements in a different order between calls (e.g., `depends_on` in job tasks, `privileges` in grants), implement `KeyedSlices` to compare elements by a natural key rather than by index. Without this, every deploy after any reordering shows phantom diffs. +## No-op creates: SkipCreate + +If a resource can have a desired state that requires no API call at all to create (e.g. an empty grants list), implement `SkipCreate` so the planner omits the node instead of planning a create. It is only consulted when the node has no state entry yet: once state exists the node stays in the plan, so that emptying it still plans the corresponding update. + ## State backward compatibility The state struct is serialized to JSON and persisted between deploys. Backward incompatible changes will result in a drift, which depending diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 70a9f1f5e3d..4dc17c712b5 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -56,6 +56,12 @@ type IResource interface { // Example: func (r *ResourceVolume) DoCreate(ctx context.Context, newState *catalog.CreateVolumeRequestContent) (string, *catalog.VolumeInfo, error) DoCreate(ctx context.Context, newState any) (id string, remoteState any, e error) + // [Optional] SkipCreate reports whether creating newState would be a no-op, so that the + // planner can omit the node instead of planning a create. Only consulted when there is no + // state entry for the node yet. + // Example: func (*ResourceGrants) SkipCreate(state *GrantsState) bool + SkipCreate(newState any) bool + // [Optional] DoUpdate updates the resource. ID must not change as a result of this operation. Returns optionally remote state. // If remote state is available as part of the operation, return it; otherwise return nil. // Example: func (r *ResourceSchema) DoUpdate(ctx context.Context, id string, newState *catalog.CreateSchema, entry *PlanEntry) (*catalog.SchemaInfo, error) @@ -96,6 +102,7 @@ type Adapter struct { doCreate *calladapt.BoundCaller // Optional: + skipCreate *calladapt.BoundCaller doUpdate *calladapt.BoundCaller doUpdateWithID *calladapt.BoundCaller waitAfterCreate *calladapt.BoundCaller @@ -128,6 +135,7 @@ func NewAdapter(typedNil any, resourceType string, client *databricks.WorkspaceC doRefresh: nil, doDelete: nil, doCreate: nil, + skipCreate: nil, doUpdate: nil, doUpdateWithID: nil, doResize: nil, @@ -196,6 +204,11 @@ func (a *Adapter) initMethods(resource any) error { // Optional methods with varying signatures: + a.skipCreate, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "SkipCreate") + if err != nil { + return err + } + a.doUpdate, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "DoUpdate") if err != nil { return err @@ -299,6 +312,10 @@ func (a *Adapter) validate() error { } validations = append(validations, "DoCreate remoteState return", a.doCreate.OutTypes[1], remoteType) + if a.skipCreate != nil { + validations = append(validations, "SkipCreate newState", a.skipCreate.InTypes[0], stateType) + } + // Validate DoUpdate: must return (remoteType, error) if implemented if a.doUpdate != nil { validations = append(validations, "DoUpdate newState", a.doUpdate.InTypes[2], stateType) @@ -452,6 +469,20 @@ func (a *Adapter) DoCreate(ctx context.Context, newState any) (string, any, erro return id, remoteState, nil } +// SkipCreate reports whether creating newState would be a no-op. Resources that do not +// implement it always need a create. +func (a *Adapter) SkipCreate(newState any) (bool, error) { + if a.skipCreate == nil { + return false, nil + } + + outs, err := a.skipCreate.Call(newState) + if err != nil { + return false, err + } + return outs[0].(bool), nil +} + // HasDoUpdate returns true if the resource implements DoUpdate method. func (a *Adapter) HasDoUpdate() bool { return a.doUpdate != nil diff --git a/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index bac70bc5615..f53b1ab7730 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -77,6 +77,13 @@ func (*ResourceGrants) PrepareState(state *GrantsState) *GrantsState { return state } +// SkipCreate reports that an empty grants list needs no create: there is nothing to grant, +// and Terraform records no databricks_grants resource for it either, so a bundle migrated +// from Terraform has no state entry for the node. +func (*ResourceGrants) SkipCreate(state *GrantsState) bool { + return len(state.EmbeddedSlice) == 0 +} + func grantKey(x catalog.PrivilegeAssignment) (string, string) { return "principal", x.Principal } diff --git a/bundle/direct/dresources/grants_test.go b/bundle/direct/dresources/grants_test.go index 1724eed40dc..e6ef39a165e 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -5,6 +5,7 @@ import ( "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestBuildGrantChanges(t *testing.T) { @@ -74,3 +75,55 @@ func TestBuildGrantChanges(t *testing.T) { }) } } + +// Goes through the adapter rather than calling the method directly, so that the optional +// method is also checked to be discovered and type-validated by NewAdapter. +func TestGrantsSkipCreate(t *testing.T) { + tests := []struct { + name string + state *GrantsState + expected bool + }{ + { + name: "empty grants list", + state: &GrantsState{SecurableType: "schema", EmbeddedSlice: []catalog.PrivilegeAssignment{}}, + expected: true, + }, + { + name: "unset grants list", + state: &GrantsState{SecurableType: "schema"}, + expected: true, + }, + { + name: "one assignment", + state: &GrantsState{ + SecurableType: "schema", + EmbeddedSlice: []catalog.PrivilegeAssignment{ + {Principal: "alice", Privileges: []catalog.Privilege{catalog.PrivilegeSelect}}, + }, + }, + expected: false, + }, + } + + adapter, err := NewAdapter(SupportedResources["schemas.grants"], "schemas.grants", nil) + require.NoError(t, err) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + skip, err := adapter.SkipCreate(tt.state) + require.NoError(t, err) + assert.Equal(t, tt.expected, skip) + }) + } +} + +// Resources without SkipCreate always need a create. +func TestSkipCreateNotImplemented(t *testing.T) { + adapter, err := NewAdapter(SupportedResources["schemas"], "schemas", nil) + require.NoError(t, err) + + skip, err := adapter.SkipCreate(&catalog.CreateSchema{Name: "myschema"}) + require.NoError(t, err) + assert.False(t, skip) +} From d934ef9474a80aa0e687c3011a307e323e51d8b1 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Mon, 27 Jul 2026 07:11:43 +0000 Subject: [PATCH 5/7] Shorten comments --- .nextchanges/bundles/empty-grants-migrate.md | 2 +- bundle/direct/bundle_plan.go | 3 +-- bundle/direct/dresources/README.md | 2 +- bundle/direct/dresources/adapter.go | 8 +++----- bundle/direct/dresources/grants.go | 5 ++--- bundle/direct/dresources/grants_test.go | 3 +-- 6 files changed, 9 insertions(+), 14 deletions(-) diff --git a/.nextchanges/bundles/empty-grants-migrate.md b/.nextchanges/bundles/empty-grants-migrate.md index 121aa2b4c1f..01b1434d007 100644 --- a/.nextchanges/bundles/empty-grants-migrate.md +++ b/.nextchanges/bundles/empty-grants-migrate.md @@ -1 +1 @@ -Fixed the direct deployment engine planning a spurious `create` for a resource's empty `grants: []` list. An empty grants list with no existing state entry is now a no-op, so `bundle plan` reports no actions after `bundle deployment migrate` (Terraform never records a grants resource for an empty list). +Fixed the direct deployment engine planning a spurious `create` for an empty `grants: []` list. Terraform records no grants resource for such a list, so `bundle plan` after `bundle deployment migrate` no longer reports an action for it. diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index ac62e57049f..6756c41dd21 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -970,8 +970,7 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root return nil, fmt.Errorf("%s: %w", prefix, err) } - // A node that already has state must stay in the plan even when creating it would be - // a no-op, otherwise emptying it would plan no action at all. + // New nodes only: a node with state must stay in the plan, otherwise emptying it plans nothing. if _, hasState := db.State[node]; !hasState { skip, err := adapter.SkipCreate(newStateConfig) if err != nil { diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index fc85b49a755..6ff3c7518e9 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -44,7 +44,7 @@ If the API may return a slice's elements in a different order between calls (e.g ## No-op creates: SkipCreate -If a resource can have a desired state that requires no API call at all to create (e.g. an empty grants list), implement `SkipCreate` so the planner omits the node instead of planning a create. It is only consulted when the node has no state entry yet: once state exists the node stays in the plan, so that emptying it still plans the corresponding update. +If a desired state requires no API call to create (e.g. an empty grants list), implement `SkipCreate` and the planner omits the node instead of planning a create. It is only consulted for nodes without a state entry: once state exists the node stays in the plan, so emptying it still plans an update. ## State backward compatibility diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 4dc17c712b5..f3df51b42be 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -56,9 +56,8 @@ type IResource interface { // Example: func (r *ResourceVolume) DoCreate(ctx context.Context, newState *catalog.CreateVolumeRequestContent) (string, *catalog.VolumeInfo, error) DoCreate(ctx context.Context, newState any) (id string, remoteState any, e error) - // [Optional] SkipCreate reports whether creating newState would be a no-op, so that the - // planner can omit the node instead of planning a create. Only consulted when there is no - // state entry for the node yet. + // [Optional] SkipCreate reports that creating newState is a no-op, so the planner omits the + // node instead of planning a create. Only consulted for nodes without a state entry. // Example: func (*ResourceGrants) SkipCreate(state *GrantsState) bool SkipCreate(newState any) bool @@ -469,8 +468,7 @@ func (a *Adapter) DoCreate(ctx context.Context, newState any) (string, any, erro return id, remoteState, nil } -// SkipCreate reports whether creating newState would be a no-op. Resources that do not -// implement it always need a create. +// SkipCreate reports whether creating newState is a no-op; false if not implemented. func (a *Adapter) SkipCreate(newState any) (bool, error) { if a.skipCreate == nil { return false, nil diff --git a/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index f53b1ab7730..67c5db060c0 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -77,9 +77,8 @@ func (*ResourceGrants) PrepareState(state *GrantsState) *GrantsState { return state } -// SkipCreate reports that an empty grants list needs no create: there is nothing to grant, -// and Terraform records no databricks_grants resource for it either, so a bundle migrated -// from Terraform has no state entry for the node. +// SkipCreate reports an empty grants list as a no-op: nothing to grant, and Terraform +// records no databricks_grants resource for it, so migrated bundles have no state entry. func (*ResourceGrants) SkipCreate(state *GrantsState) bool { return len(state.EmbeddedSlice) == 0 } diff --git a/bundle/direct/dresources/grants_test.go b/bundle/direct/dresources/grants_test.go index e6ef39a165e..4b0eee206f7 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -76,8 +76,7 @@ func TestBuildGrantChanges(t *testing.T) { } } -// Goes through the adapter rather than calling the method directly, so that the optional -// method is also checked to be discovered and type-validated by NewAdapter. +// Calls through the adapter so the optional-method wiring is covered too. func TestGrantsSkipCreate(t *testing.T) { tests := []struct { name string From a80615dd017e4ff8d64079f90027aa3972b96e42 Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Thu, 30 Jul 2026 08:29:45 +0000 Subject: [PATCH 6/7] acc: cover emptying a populated grants list Emptying grants on a node that already has state must still plan the revoke. SkipCreate is only consulted for nodes without state; applied unconditionally the node would fall into the delete branch, where grants' no-op DoDelete silently keeps the grant. The engines agree on the end state but not on the plan summary: direct keeps the emptied node in state and counts it as unchanged, terraform has no grants resource left, so the plan output is per-engine. --- .../schemas/remove_all/databricks.yml.tmpl | 10 ++++++ .../schemas/remove_all/out.plan.direct.txt | 12 +++++++ .../schemas/remove_all/out.plan.terraform.txt | 12 +++++++ .../grants/schemas/remove_all/out.test.toml | 4 +++ .../grants/schemas/remove_all/output.txt | 31 +++++++++++++++++++ .../grants/schemas/remove_all/script | 27 ++++++++++++++++ 6 files changed, 96 insertions(+) create mode 100644 acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl create mode 100644 acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt create mode 100644 acceptance/bundle/resources/grants/schemas/remove_all/out.plan.terraform.txt create mode 100644 acceptance/bundle/resources/grants/schemas/remove_all/out.test.toml create mode 100644 acceptance/bundle/resources/grants/schemas/remove_all/output.txt create mode 100644 acceptance/bundle/resources/grants/schemas/remove_all/script diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl b/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl new file mode 100644 index 00000000000..fcf0be8dd85 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl @@ -0,0 +1,10 @@ +bundle: + name: schema-grants-remove-all-$UNIQUE_NAME + +resources: + schemas: + grants_schema: + name: schema_remove_all_$UNIQUE_NAME + catalog_name: main + grants: # TO_EMPTY + - { principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] } # TO_EMPTY diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt new file mode 100644 index 00000000000..94672212484 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt @@ -0,0 +1,12 @@ + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.terraform.txt b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.terraform.txt new file mode 100644 index 00000000000..52a44d17f5a --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.terraform.txt @@ -0,0 +1,12 @@ + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/out.test.toml b/acceptance/bundle/resources/grants/schemas/remove_all/out.test.toml new file mode 100644 index 00000000000..e849ec85ace --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/out.test.toml @@ -0,0 +1,4 @@ +Local = true +Cloud = true +RequiresUnityCatalog = true +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/output.txt b/acceptance/bundle/resources/grants/schemas/remove_all/output.txt new file mode 100644 index 00000000000..b2d5da5dca4 --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/output.txt @@ -0,0 +1,31 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] grants get schema main.schema_remove_all_[UNIQUE_NAME] +json.privilege_assignments[].principal = "deco-test-user@databricks.com"; +json.privilege_assignments[].privileges[] = "USE_SCHEMA"; + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] grants get schema main.schema_remove_all_[UNIQUE_NAME] +json = {}; + +>>> errcode [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.schemas.grants_schema + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete resources.schemas.grants_schema + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/script b/acceptance/bundle/resources/grants/schemas/remove_all/script new file mode 100644 index 00000000000..671c689fc2f --- /dev/null +++ b/acceptance/bundle/resources/grants/schemas/remove_all/script @@ -0,0 +1,27 @@ +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace errcode $CLI bundle destroy --auto-approve + rm -f out.requests.txt +} +trap cleanup EXIT + +trace $CLI bundle deploy +trace $CLI grants get schema main.schema_remove_all_$UNIQUE_NAME | gron.py --noindex | sort_lines.py --repl | contains.py 'deco-test-user@databricks.com' + +# Empty the whole list. The grants node has state here, so it must stay in the plan +# and revoke; dropping it would move it to the delete branch where grants' no-op +# DoDelete silently keeps the grant. +grep -v 'TO_EMPTY' databricks.yml > tmp.yml && mv tmp.yml databricks.yml +echo ' grants: []' >> databricks.yml + +trace $CLI bundle deploy +trace $CLI grants get schema main.schema_remove_all_$UNIQUE_NAME | gron.py --noindex | sort_lines.py --repl | contains.py '!deco-test-user@databricks.com' + +# Per-engine: direct keeps the emptied grants node in state and reports it as unchanged, +# terraform has no grants resource left to report. +{ + trace $CLI bundle plan + trace $CLI bundle deploy + trace $CLI bundle plan +} &> out.plan.$DATABRICKS_BUNDLE_ENGINE.txt From 8f7505aebc9e9e671b18623a95da08f9124b109e Mon Sep 17 00:00:00 2001 From: Rada Kamysheva Date: Wed, 5 Aug 2026 12:41:03 +0000 Subject: [PATCH 7/7] Drop the state entry when an update empties a resource Emptying a deployed grants list revoked the grants but kept the node in state, so direct reported it as unchanged forever while a fresh deploy of the same config planned no node at all. Rename SkipCreate to IsEmptyState and consult it in apply: an update that leaves the resource empty deletes the state entry instead of saving one, and the next plan skips the node. The plan node is still created for the emptying deploy itself, so the revoke happens as before. Both engines now produce the same plan output, so the per-engine out.plan.*.txt files collapse into one. Also swap the acceptance script's grep-and-append for update_file.py, which errors out when the string it replaces is missing. --- .nextchanges/bundles/empty-grants-migrate.md | 2 +- .../schemas/remove_all/databricks.yml.tmpl | 3 +-- .../schemas/remove_all/out.plan.direct.txt | 12 --------- .../{out.plan.terraform.txt => out.plan.txt} | 0 .../grants/schemas/remove_all/script | 9 +++---- bundle/direct/apply.go | 19 +++++++++++-- bundle/direct/bundle_plan.go | 5 ++-- bundle/direct/dresources/README.md | 6 +++-- bundle/direct/dresources/adapter.go | 27 ++++++++++--------- bundle/direct/dresources/grants.go | 7 ++--- bundle/direct/dresources/grants_test.go | 14 +++++----- 11 files changed, 55 insertions(+), 49 deletions(-) delete mode 100644 acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt rename acceptance/bundle/resources/grants/schemas/remove_all/{out.plan.terraform.txt => out.plan.txt} (100%) diff --git a/.nextchanges/bundles/empty-grants-migrate.md b/.nextchanges/bundles/empty-grants-migrate.md index 01b1434d007..fa6707d3688 100644 --- a/.nextchanges/bundles/empty-grants-migrate.md +++ b/.nextchanges/bundles/empty-grants-migrate.md @@ -1 +1 @@ -Fixed the direct deployment engine planning a spurious `create` for an empty `grants: []` list. Terraform records no grants resource for such a list, so `bundle plan` after `bundle deployment migrate` no longer reports an action for it. +Fixed the direct deployment engine planning a spurious `create` for an empty `grants: []` list. Terraform records no grants resource for such a list, so `bundle plan` after `bundle deployment migrate` no longer reports an action for it. Emptying a previously deployed list still revokes the grants, after which the node is dropped from the deployment state instead of being reported as unchanged forever. diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl b/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl index fcf0be8dd85..00752f7b34f 100644 --- a/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl +++ b/acceptance/bundle/resources/grants/schemas/remove_all/databricks.yml.tmpl @@ -6,5 +6,4 @@ resources: grants_schema: name: schema_remove_all_$UNIQUE_NAME catalog_name: main - grants: # TO_EMPTY - - { principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] } # TO_EMPTY + grants: [{ principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] }] diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt deleted file mode 100644 index 94672212484..00000000000 --- a/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.direct.txt +++ /dev/null @@ -1,12 +0,0 @@ - ->>> [CLI] bundle plan -Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged - ->>> [CLI] bundle deploy -Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/schema-grants-remove-all-[UNIQUE_NAME]/default/files... -Deploying resources... -Updating deployment state... -Deployment complete! - ->>> [CLI] bundle plan -Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.terraform.txt b/acceptance/bundle/resources/grants/schemas/remove_all/out.plan.txt similarity index 100% rename from acceptance/bundle/resources/grants/schemas/remove_all/out.plan.terraform.txt rename to acceptance/bundle/resources/grants/schemas/remove_all/out.plan.txt diff --git a/acceptance/bundle/resources/grants/schemas/remove_all/script b/acceptance/bundle/resources/grants/schemas/remove_all/script index 671c689fc2f..8d8c2f55e92 100644 --- a/acceptance/bundle/resources/grants/schemas/remove_all/script +++ b/acceptance/bundle/resources/grants/schemas/remove_all/script @@ -12,16 +12,15 @@ trace $CLI grants get schema main.schema_remove_all_$UNIQUE_NAME | gron.py --noi # Empty the whole list. The grants node has state here, so it must stay in the plan # and revoke; dropping it would move it to the delete branch where grants' no-op # DoDelete silently keeps the grant. -grep -v 'TO_EMPTY' databricks.yml > tmp.yml && mv tmp.yml databricks.yml -echo ' grants: []' >> databricks.yml +update_file.py databricks.yml 'grants: [{ principal: deco-test-user@databricks.com, privileges: [USE_SCHEMA] }]' 'grants: []' trace $CLI bundle deploy trace $CLI grants get schema main.schema_remove_all_$UNIQUE_NAME | gron.py --noindex | sort_lines.py --repl | contains.py '!deco-test-user@databricks.com' -# Per-engine: direct keeps the emptied grants node in state and reports it as unchanged, -# terraform has no grants resource left to report. +# The revoking deploy drops the emptied grants node from state, so from here on both +# engines plan the schema alone. { trace $CLI bundle plan trace $CLI bundle deploy trace $CLI bundle plan -} &> out.plan.$DATABRICKS_BUNDLE_ENGINE.txt +} &> out.plan.txt diff --git a/bundle/direct/apply.go b/bundle/direct/apply.go index b3c46036c53..cbb0a2d45ff 100644 --- a/bundle/direct/apply.go +++ b/bundle/direct/apply.go @@ -149,9 +149,24 @@ func (d *DeploymentUnit) Update(ctx context.Context, db *dstate.DeploymentState, return err } - err = db.SaveState(d.ResourceKey, id, newState, d.DependsOn) + empty, err := d.Adapter.IsEmptyState(newState) if err != nil { - return fmt.Errorf("saving state id=%s: %w", id, err) + return err + } + + if empty { + // The update emptied the resource out (e.g. all grants revoked). Keeping an entry + // would report the node as tracked-and-unchanged forever, while a fresh deploy of + // the same config plans no node at all; drop it so the two agree. + err = db.DeleteState(d.ResourceKey) + if err != nil { + return fmt.Errorf("deleting state id=%s: %w", id, err) + } + } else { + err = db.SaveState(d.ResourceKey, id, newState, d.DependsOn) + if err != nil { + return fmt.Errorf("saving state id=%s: %w", id, err) + } } waitRemoteState, err := retryOnTransient(ctx, func() (any, error) { diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index dcc8c4de21f..739d9ae3cbd 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -976,12 +976,13 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root } // New nodes only: a node with state must stay in the plan, otherwise emptying it plans nothing. + // Apply drops the state entry once the node is empty, so it is skipped from then on. if _, hasState := db.State[node]; !hasState { - skip, err := adapter.SkipCreate(newStateConfig) + empty, err := adapter.IsEmptyState(newStateConfig) if err != nil { return nil, fmt.Errorf("%s: %w", prefix, err) } - if skip { + if empty { continue } } diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index 6ff3c7518e9..b6c09bda4ed 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -42,9 +42,11 @@ For resources whose create or update is asynchronous (the resource is not immedi If the API may return a slice's elements in a different order between calls (e.g., `depends_on` in job tasks, `privileges` in grants), implement `KeyedSlices` to compare elements by a natural key rather than by index. Without this, every deploy after any reordering shows phantom diffs. -## No-op creates: SkipCreate +## Empty states: IsEmptyState -If a desired state requires no API call to create (e.g. an empty grants list), implement `SkipCreate` and the planner omits the node instead of planning a create. It is only consulted for nodes without a state entry: once state exists the node stays in the plan, so emptying it still plans an update. +If a desired state describes no resource at all (e.g. an empty grants list), implement `IsEmptyState`. The planner omits such a node instead of planning a create, and apply drops its state entry instead of persisting one. + +The planner only consults it for nodes without a state entry: once state exists the node stays in the plan, so emptying it still plans an update, and the update is what removes the entry. ## State backward compatibility diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 0c6232e00d8..ea8876c4d52 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -56,10 +56,11 @@ type IResource interface { // Example: func (r *ResourceVolume) DoCreate(ctx context.Context, newState *catalog.CreateVolumeRequestContent) (string, *catalog.VolumeInfo, error) DoCreate(ctx context.Context, newState any) (id string, remoteState any, e error) - // [Optional] SkipCreate reports that creating newState is a no-op, so the planner omits the - // node instead of planning a create. Only consulted for nodes without a state entry. - // Example: func (*ResourceGrants) SkipCreate(state *GrantsState) bool - SkipCreate(newState any) bool + // [Optional] IsEmptyState reports that newState describes no resource at all: the planner + // omits the node instead of planning a create, and apply drops the state entry instead of + // persisting one, so both engines converge on "this node does not exist". + // Example: func (*ResourceGrants) IsEmptyState(state *GrantsState) bool + IsEmptyState(newState any) bool // [Optional] DoUpdate updates the resource. ID must not change as a result of this operation. Returns optionally remote state. // If remote state is available as part of the operation, return it; otherwise return nil. @@ -108,7 +109,7 @@ type Adapter struct { doCreate *calladapt.BoundCaller // Optional: - skipCreate *calladapt.BoundCaller + isEmptyState *calladapt.BoundCaller doUpdate *calladapt.BoundCaller doUpdateWithID *calladapt.BoundCaller waitAfterCreate *calladapt.BoundCaller @@ -142,7 +143,7 @@ func NewAdapter(typedNil any, resourceType string, client *databricks.WorkspaceC doRefresh: nil, doDelete: nil, doCreate: nil, - skipCreate: nil, + isEmptyState: nil, doUpdate: nil, doUpdateWithID: nil, doResize: nil, @@ -212,7 +213,7 @@ func (a *Adapter) initMethods(resource any) error { // Optional methods with varying signatures: - a.skipCreate, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "SkipCreate") + a.isEmptyState, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "IsEmptyState") if err != nil { return err } @@ -325,8 +326,8 @@ func (a *Adapter) validate() error { } validations = append(validations, "DoCreate remoteState return", a.doCreate.OutTypes[1], remoteType) - if a.skipCreate != nil { - validations = append(validations, "SkipCreate newState", a.skipCreate.InTypes[0], stateType) + if a.isEmptyState != nil { + validations = append(validations, "IsEmptyState newState", a.isEmptyState.InTypes[0], stateType) } // Validate DoUpdate: must return (remoteType, error) if implemented @@ -486,13 +487,13 @@ func (a *Adapter) DoCreate(ctx context.Context, newState any) (string, any, erro return id, remoteState, nil } -// SkipCreate reports whether creating newState is a no-op; false if not implemented. -func (a *Adapter) SkipCreate(newState any) (bool, error) { - if a.skipCreate == nil { +// IsEmptyState reports whether newState describes no resource; false if not implemented. +func (a *Adapter) IsEmptyState(newState any) (bool, error) { + if a.isEmptyState == nil { return false, nil } - outs, err := a.skipCreate.Call(newState) + outs, err := a.isEmptyState.Call(newState) if err != nil { return false, err } diff --git a/bundle/direct/dresources/grants.go b/bundle/direct/dresources/grants.go index 780e2d0765e..10e52446100 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -76,9 +76,10 @@ func (*ResourceGrants) PrepareState(state *GrantsState) *GrantsState { return state } -// SkipCreate reports an empty grants list as a no-op: nothing to grant, and Terraform -// records no databricks_grants resource for it, so migrated bundles have no state entry. -func (*ResourceGrants) SkipCreate(state *GrantsState) bool { +// IsEmptyState reports an empty grants list as no resource at all: nothing to grant, and +// Terraform records no databricks_grants resource for it either, so migrated bundles have +// no state entry. +func (*ResourceGrants) IsEmptyState(state *GrantsState) bool { return len(state.EmbeddedSlice) == 0 } diff --git a/bundle/direct/dresources/grants_test.go b/bundle/direct/dresources/grants_test.go index 3e6f25cef70..faa72b02c25 100644 --- a/bundle/direct/dresources/grants_test.go +++ b/bundle/direct/dresources/grants_test.go @@ -77,7 +77,7 @@ func TestBuildGrantChanges(t *testing.T) { } // Calls through the adapter so the optional-method wiring is covered too. -func TestGrantsSkipCreate(t *testing.T) { +func TestGrantsIsEmptyState(t *testing.T) { tests := []struct { name string state *GrantsState @@ -110,21 +110,21 @@ func TestGrantsSkipCreate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - skip, err := adapter.SkipCreate(tt.state) + empty, err := adapter.IsEmptyState(tt.state) require.NoError(t, err) - assert.Equal(t, tt.expected, skip) + assert.Equal(t, tt.expected, empty) }) } } -// Resources without SkipCreate always need a create. -func TestSkipCreateNotImplemented(t *testing.T) { +// Resources without IsEmptyState are never treated as empty. +func TestIsEmptyStateNotImplemented(t *testing.T) { adapter, err := NewAdapter(SupportedResources["schemas"], "schemas", nil) require.NoError(t, err) - skip, err := adapter.SkipCreate(&catalog.CreateSchema{Name: "myschema"}) + empty, err := adapter.IsEmptyState(&catalog.CreateSchema{Name: "myschema"}) require.NoError(t, err) - assert.False(t, skip) + assert.False(t, empty) } func TestNormalizeAssignments(t *testing.T) {