Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .nextchanges/bundles/empty-grants-migrate.md
Original file line number Diff line number Diff line change
@@ -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.
1 change: 1 addition & 0 deletions acceptance/bundle/invariant/migrate/out.test.toml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 0 additions & 11 deletions acceptance/bundle/invariant/migrate/test.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]

Expand Down
11 changes: 11 additions & 0 deletions bundle/direct/bundle_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down
4 changes: 4 additions & 0 deletions bundle/direct/dresources/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 29 additions & 0 deletions bundle/direct/dresources/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -103,6 +108,7 @@ type Adapter struct {
doCreate *calladapt.BoundCaller

// Optional:
skipCreate *calladapt.BoundCaller
doUpdate *calladapt.BoundCaller
doUpdateWithID *calladapt.BoundCaller
waitAfterCreate *calladapt.BoundCaller
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions bundle/direct/dresources/grants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
52 changes: 52 additions & 0 deletions bundle/direct/dresources/grants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
Loading