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 NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* Remove hidden, never-functional `--existing-dashboard-id`, `--existing-dashboard-path`, `--existing-alert-id`, and `--existing-genie-space-id` alias flags from `bundle generate`; use the documented `--existing-id` / `--existing-path` flags instead ([#5591](https://github.com/databricks/cli/pull/5591)).
* engine/direct: Fix WAL corruption after two consecutive failed deploys ([#5606](https://github.com/databricks/cli/pull/5606)).
* engine/direct: Don't open the deployment state WAL when a deploy's plan fails ([#5607](https://github.com/databricks/cli/pull/5607)).
* direct: Stop spurious recreate/rename on redeploy when the backend normalizes a resource's name-based ID (e.g. Unity Catalog lowercasing a schema or volume name) ([#5599](https://github.com/databricks/cli/pull/5599)).

### Dependency updates

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
},
"name": {
"action": "recreate",
"reason": "immutable",
"reason": "id_field",
"old": "[ENDPOINT_NAME_1]",
"new": "[ENDPOINT_NAME_2]",
"remote": "[ENDPOINT_NAME_1]"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"changes": {
"name": {
"action": "recreate",
"reason": "immutable",
"reason": "id_field",
"old": "[ORIGINAL_ENDPOINT_ID]",
"new": "[NEW_ENDPOINT_ID]",
"remote": "[ORIGINAL_ENDPOINT_ID]"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"changes": {
"table_name": {
"action": "recreate",
"reason": "immutable",
"reason": "id_field",
"old": "main.qm_test_[UNIQUE_NAME].test_table",
"new": "main.qm_test_[UNIQUE_NAME].test_table_2",
"remote": "main.qm_test_[UNIQUE_NAME].test_table"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ json.plan.resources.secret_scopes.my_scope.new_state.value.scope_backend_type =
json.plan.resources.secret_scopes.my_scope.remote_state.backend_type = "DATABRICKS";
json.plan.resources.secret_scopes.my_scope.remote_state.name = "test-scope-[UNIQUE_NAME]-1";
json.plan.resources.secret_scopes.my_scope.changes.scope.action = "recreate";
json.plan.resources.secret_scopes.my_scope.changes.scope.reason = "immutable";
json.plan.resources.secret_scopes.my_scope.changes.scope.reason = "id_field";
json.plan.resources.secret_scopes.my_scope.changes.scope.old = "test-scope-[UNIQUE_NAME]-1";
json.plan.resources.secret_scopes.my_scope.changes.scope.new = "test-scope-[UNIQUE_NAME]-2";
json.plan.resources.secret_scopes.my_scope.changes.scope.remote = "test-scope-[UNIQUE_NAME]-1";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Deployment complete!
"changes": {
"name": {
"action": "skip",
"reason": "uc_case"
"reason": "id_field"
}
}
},
Expand All @@ -41,11 +41,11 @@ Deployment complete!
"changes": {
"name": {
"action": "skip",
"reason": "uc_case"
"reason": "id_changes"
},
"schema_name": {
"action": "skip",
"reason": "uc_case"
"reason": "id_field"
},
"storage_location": {
"action": "skip",
Expand Down
70 changes: 45 additions & 25 deletions bundle/direct/bundle_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,12 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
} else if reason, ok := shouldSkip(generatedCfg, path, ch); ok {
ch.Action = deployplan.Skip
ch.Reason = reason
} else if action, reason, ok := classifyIDField(cfg, path, ch); ok {
ch.Action = action
ch.Reason = reason
} else if action, reason, ok := classifyIDField(generatedCfg, path, ch); ok {
ch.Action = action
ch.Reason = reason
} else if reason, ok := shouldSkipBackendDefault(cfg, path, ch); ok {
ch.Action = deployplan.Skip
ch.Reason = reason
Expand All @@ -381,11 +387,11 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
} else if reason, ok := shouldSkipNormalized(generatedCfg, path, ch); ok {
ch.Action = deployplan.Skip
ch.Reason = reason
} else if action, reason := shouldUpdateOrRecreate(cfg, path); action != deployplan.Undefined {
ch.Action = action
} else if reason, ok := findMatchingRule(path, cfg.RecreateOnChanges); ok {
ch.Action = deployplan.Recreate
ch.Reason = reason
} else if action, reason := shouldUpdateOrRecreate(generatedCfg, path); action != deployplan.Undefined {
ch.Action = action
} else if reason, ok := findMatchingRule(path, generatedCfg.RecreateOnChanges); ok {
ch.Action = deployplan.Recreate
ch.Reason = reason
} else {
ch.Action = deployplan.Update
Expand Down Expand Up @@ -440,11 +446,41 @@ func shouldSkip(cfg *dresources.ResourceLifecycleConfig, path *structpath.PathNo
return "", false
}

// classifyIDField decides the action for a field that composes the resource's
// name-based ID, in one place so the remote-only-vs-local rule does not depend on
// ordering elsewhere in the ladder. Returns ok=false if the path is not such a field.
//
// - Remote-only difference (Old==New): the resource was just fetched by that ID,
// so a differing remote value can only be backend normalization (e.g. UC
// lowercasing) — a real out-of-band rename would 404 and is handled as
// resource-gone. Skip.
// - Local change: provided_id_fields recreate (delete + create); updatable_id_fields
// rename via UpdateWithID.
func classifyIDField(cfg *dresources.ResourceLifecycleConfig, path *structpath.PathNode, ch *deployplan.ChangeDesc) (deployplan.ActionType, string, bool) {
if cfg == nil {
return deployplan.Undefined, "", false
}
localChange := !structdiff.IsEqual(ch.Old, ch.New)
if reason, ok := findMatchingRule(path, cfg.ProvidedIDFields); ok {
if localChange {
return deployplan.Recreate, reason, true
}
return deployplan.Skip, reason, true
}
if reason, ok := findMatchingRule(path, cfg.UpdatableIDFields); ok {
if localChange {
return deployplan.UpdateWithID, reason, true
}
return deployplan.Skip, reason, true
}
return deployplan.Undefined, "", false
}

// shouldSkipNormalized skips a change that is a false diff caused by UC API
// normalization: the API lowercases identifier names (normalize_case) and strips
// trailing slashes from storage URLs (normalize_slash). The direct engine saves
// local config to state, so without this the next plan sees the original value
// against the normalized remote value and triggers a spurious recreate/update.
// normalization: the API strips trailing slashes from storage URLs
// (normalize_slash). The direct engine saves local config to state, so without
// this the next plan sees the original value against the normalized remote value
// and triggers a spurious recreate/update.
func shouldSkipNormalized(cfg *dresources.ResourceLifecycleConfig, path *structpath.PathNode, ch *deployplan.ChangeDesc) (string, bool) {
if cfg == nil {
return "", false
Expand All @@ -454,28 +490,12 @@ func shouldSkipNormalized(cfg *dresources.ResourceLifecycleConfig, path *structp
if !newOk || !remoteOk {
return "", false
}
if reason, ok := findMatchingRule(path, cfg.NormalizeCase); ok && strings.EqualFold(newStr, remoteStr) {
return reason, true
}
if reason, ok := findMatchingRule(path, cfg.NormalizeSlash); ok && strings.TrimRight(newStr, "/") == strings.TrimRight(remoteStr, "/") {
return reason, true
}
return "", false
}

func shouldUpdateOrRecreate(cfg *dresources.ResourceLifecycleConfig, path *structpath.PathNode) (deployplan.ActionType, string) {
if cfg == nil {
return deployplan.Undefined, ""
}
if reason, ok := findMatchingRule(path, cfg.RecreateOnChanges); ok {
return deployplan.Recreate, reason
}
if reason, ok := findMatchingRule(path, cfg.UpdateIDOnChanges); ok {
return deployplan.UpdateWithID, reason
}
return deployplan.Undefined, ""
}

// shouldSkipBackendDefault checks if a change should be skipped because the remote value
// is a known backend default. Applies when old and new are nil but remote is set.
// If the rule has allowed values, the remote value must match one of them.
Expand Down Expand Up @@ -679,7 +699,7 @@ func (b *DeploymentBundle) LookupReferencePreDeploy(ctx context.Context, path *s
return value, nil
}

canReadRemoteCache := targetAction == deployplan.Skip || (targetAction.KeepsID() && adapter.IsFieldInRecreateOnChanges(fieldPath))
canReadRemoteCache := targetAction == deployplan.Skip || (targetAction.KeepsID() && adapter.FieldTriggersRecreate(fieldPath))

if configValidErr != nil && remoteValidErr == nil {
// The field is only present in remote state schema.
Expand Down
2 changes: 1 addition & 1 deletion bundle/direct/dresources/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Each field with special plan/deploy behavior must be declared in `resources.yml`
- `managed` — managed by the cloud provider or platform, not by the user config
- **`ignore_local_changes`**: Ignore changes the user makes to this field. Use for fields that cannot be updated via API — either they are immutable after creation or require a separate API that is not yet implemented. Must have a comment in resources.yml explaining why.
- **`recreate_on_changes`**: Changing this field requires delete + create. Use for truly immutable fields (name, type, location). The reason should reference API docs or TF provider.
- **`update_id_on_changes`**: Changing this field changes the resource's ID. Requires `DoUpdateWithID` to be implemented.
- **`updatable_id_fields`**: Changing this field changes the resource's ID. Requires `DoUpdateWithID` to be implemented.

## Update mask

Expand Down
16 changes: 12 additions & 4 deletions bundle/direct/dresources/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,12 +346,12 @@ func (a *Adapter) validate() error {

// Validate resourceConfig consistency with DoUpdateWithID
if a.overrideChangeDesc == nil {
hasUpdateWithIDTrigger := a.resourceConfig != nil && len(a.resourceConfig.UpdateIDOnChanges) > 0
hasUpdateWithIDTrigger := a.resourceConfig != nil && len(a.resourceConfig.UpdatableIDFields) > 0
if hasUpdateWithIDTrigger && a.doUpdateWithID == nil {
return errors.New("resourceConfig has update_id_on_changes but DoUpdateWithID is not implemented")
return errors.New("resourceConfig has updatable_id_fields but DoUpdateWithID is not implemented")
}
if a.doUpdateWithID != nil && !hasUpdateWithIDTrigger {
return errors.New("DoUpdateWithID is implemented but resourceConfig lacks update_id_on_changes")
return errors.New("DoUpdateWithID is implemented but resourceConfig lacks updatable_id_fields")
}
}

Expand All @@ -378,12 +378,20 @@ func (a *Adapter) GeneratedResourceConfig() *ResourceLifecycleConfig {
return a.generatedResourceConfig
}

func (a *Adapter) IsFieldInRecreateOnChanges(path *structpath.PathNode) bool {
// FieldTriggersRecreate reports whether a local change to the field forces a
// delete + create. Both recreate_on_changes and provided_id_fields do this, so a
// caller that knows the ID is preserved can conclude the field is unchanged.
func (a *Adapter) FieldTriggersRecreate(path *structpath.PathNode) bool {
for _, p := range a.resourceConfig.RecreateOnChanges {
if path.HasPatternPrefix(p.Field) {
return true
}
}
for _, p := range a.resourceConfig.ProvidedIDFields {
if path.HasPatternPrefix(p.Field) {
return true
}
}
return false
}

Expand Down
7 changes: 5 additions & 2 deletions bundle/direct/dresources/all_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1003,8 +1003,11 @@ func validateResourceConfig(t *testing.T, stateType reflect.Type, cfg *ResourceL
for _, p := range cfg.RecreateOnChanges {
assert.NoError(t, structaccess.ValidatePattern(stateType, p.Field), "RecreateOnChanges: %s", p.Field)
}
for _, p := range cfg.UpdateIDOnChanges {
assert.NoError(t, structaccess.ValidatePattern(stateType, p.Field), "UpdateIDOnChanges: %s", p.Field)
for _, p := range cfg.ProvidedIDFields {
assert.NoError(t, structaccess.ValidatePattern(stateType, p.Field), "ProvidedIDFields: %s", p.Field)
}
for _, p := range cfg.UpdatableIDFields {
assert.NoError(t, structaccess.ValidatePattern(stateType, p.Field), "UpdatableIDFields: %s", p.Field)
}
for _, p := range cfg.IgnoreRemoteChanges {
assert.NoError(t, structaccess.ValidatePattern(stateType, p.Field), "IgnoreRemoteChanges: %s", p.Field)
Expand Down
5 changes: 5 additions & 0 deletions bundle/direct/dresources/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,11 @@ func TestAppDoUpdate_UpdateMaskHasAllFields(t *testing.T) {
nonUpdatableFields = append(nonUpdatableFields, field.Field.String())
}

// provided_id_fields recreate on local changes, so they are not updatable either.
for _, field := range config.ProvidedIDFields {
nonUpdatableFields = append(nonUpdatableFields, field.Field.String())
}

fields := reflect.TypeFor[apps.App]()
var allFields []string
for field := range fields.Fields() {
Expand Down
24 changes: 16 additions & 8 deletions bundle/direct/dresources/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,20 @@ type ResourceLifecycleConfig struct {
// RecreateOnChanges: field patterns that trigger delete + create when changed.
RecreateOnChanges []FieldRule `yaml:"recreate_on_changes,omitempty"`

// UpdateIDOnChanges: field patterns that trigger UpdateWithID when changed.
UpdateIDOnChanges []FieldRule `yaml:"update_id_on_changes,omitempty"`

// NormalizeCase: string field patterns the UC API lowercases on write.
// A change is skipped when local and remote differ only by case.
NormalizeCase []FieldRule `yaml:"normalize_case,omitempty"`
// ProvidedIDFields: field patterns that compose the resource's ID — a name the
// user provides (not a server-generated id), which DoRead fetches by. Local
// changes trigger delete + create. Remote-only differences are skipped: since the
// user supplies the value and we just fetched by it, a differing remote value can
// only be backend normalization (e.g. UC lowercasing) — a real out-of-band rename
// would 404 and is handled as resource-gone.
ProvidedIDFields []FieldRule `yaml:"provided_id_fields,omitempty"`

// UpdatableIDFields: like ProvidedIDFields, these compose the resource's
// user-provided ID, but the backend supports renaming them in place. A local
// change triggers UpdateWithID (a rename; the ID changes) instead of delete +
// create. A remote-only difference is still skipped (see classifyIDField) — it
// can only be backend normalization, since we just fetched by this ID.
UpdatableIDFields []FieldRule `yaml:"updatable_id_fields,omitempty"`

// NormalizeSlash: string field patterns the UC API strips trailing slashes from.
// A change is skipped when local and remote differ only by trailing slashes.
Expand All @@ -87,8 +95,8 @@ var empty = ResourceLifecycleConfig{
IgnoreRemoteChanges: nil,
IgnoreLocalChanges: nil,
RecreateOnChanges: nil,
UpdateIDOnChanges: nil,
NormalizeCase: nil,
ProvidedIDFields: nil,
UpdatableIDFields: nil,
NormalizeSlash: nil,
BackendDefaults: nil,
}
Expand Down
37 changes: 35 additions & 2 deletions bundle/direct/dresources/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func TestGetResourceConfig(t *testing.T) {
assert.Empty(t, GetResourceConfig("nonexistent").RecreateOnChanges)
}

// categoryRules projects ResourceLifecycleConfig's five categories onto a
// categoryRules projects ResourceLifecycleConfig's categories onto a
// uniform [name, []FieldRule] shape so the redundancy check can iterate them.
func categoryRules(c ResourceLifecycleConfig) []struct {
name string
Expand All @@ -33,7 +33,8 @@ func categoryRules(c ResourceLifecycleConfig) []struct {
{"ignore_remote_changes", c.IgnoreRemoteChanges},
{"ignore_local_changes", c.IgnoreLocalChanges},
{"recreate_on_changes", c.RecreateOnChanges},
{"update_id_on_changes", c.UpdateIDOnChanges},
{"provided_id_fields", c.ProvidedIDFields},
{"updatable_id_fields", c.UpdatableIDFields},
{"backend_defaults", backendAsFieldRules},
}
}
Expand Down Expand Up @@ -72,3 +73,35 @@ func TestResourcesYMLNoRedundantRules(t *testing.T) {
}
}
}

// TestResourcesYMLActionCategoriesExclusive guards that a field is in at most one
// of the action categories that decide a change's action. They are not
// independent: classifyIDField (provided_id_fields, updatable_id_fields) runs
// before recreate_on_changes in the ladder and short-circuits, so a field listed
// in more than one would have all but the first entry silently dead — and the
// categories disagree (e.g. provided_id_fields skips a remote-only diff that
// recreate_on_changes would recreate).
func TestResourcesYMLActionCategoriesExclusive(t *testing.T) {
cfg := MustLoadConfig()
for resourceType, rc := range cfg.Resources {
actionCats := []struct {
name string
rules []FieldRule
}{
{"recreate_on_changes", rc.RecreateOnChanges},
{"provided_id_fields", rc.ProvidedIDFields},
{"updatable_id_fields", rc.UpdatableIDFields},
}
firstCat := map[string]string{}
for _, c := range actionCats {
for _, r := range c.rules {
field := r.Field.String()
if prev, ok := firstCat[field]; ok {
t.Errorf("bundle/direct/dresources/resources.yml: %s lists %q in both %s and %s; a field's action belongs to exactly one category", resourceType, field, prev, c.name)
} else {
firstCat[field] = c.name
}
}
}
}
}
Loading
Loading