diff --git a/.nextchanges/bundles/empty-grants-migrate.md b/.nextchanges/bundles/empty-grants-migrate.md new file mode 100644 index 00000000000..01b1434d007 --- /dev/null +++ b/.nextchanges/bundles/empty-grants-migrate.md @@ -0,0 +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. 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"] diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index fda8180cfc4..dcc8c4de21f 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -975,6 +975,17 @@ func (b *DeploymentBundle) makePlan(ctx context.Context, configRoot *config.Root return nil, fmt.Errorf("%s: %w", prefix, err) } + // 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 { + 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..6ff3c7518e9 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 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 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 fdaa15bfcea..0c6232e00d8 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -56,6 +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] 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) @@ -103,6 +108,7 @@ type Adapter struct { doCreate *calladapt.BoundCaller // Optional: + skipCreate *calladapt.BoundCaller doUpdate *calladapt.BoundCaller doUpdateWithID *calladapt.BoundCaller waitAfterCreate *calladapt.BoundCaller @@ -136,6 +142,7 @@ func NewAdapter(typedNil any, resourceType string, client *databricks.WorkspaceC doRefresh: nil, doDelete: nil, doCreate: nil, + skipCreate: nil, doUpdate: nil, doUpdateWithID: nil, doResize: nil, @@ -205,6 +212,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 @@ -313,6 +325,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) @@ -470,6 +486,19 @@ 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 { + 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 f0a9423838c..780e2d0765e 100644 --- a/bundle/direct/dresources/grants.go +++ b/bundle/direct/dresources/grants.go @@ -76,6 +76,12 @@ 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 { + 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 a1c5895a33d..3e6f25cef70 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) { @@ -75,6 +76,57 @@ func TestBuildGrantChanges(t *testing.T) { } } +// Calls through the adapter so the optional-method wiring is covered too. +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) +} + func TestNormalizeAssignments(t *testing.T) { tests := []struct { name string