diff --git a/acceptance/bundle/resources/jobs/on_failure_empty_slice/out.plan.direct.json b/acceptance/bundle/resources/jobs/on_failure_empty_slice/out.plan.direct.json index 319a62d25d..d7ff65646a 100644 --- a/acceptance/bundle/resources/jobs/on_failure_empty_slice/out.plan.direct.json +++ b/acceptance/bundle/resources/jobs/on_failure_empty_slice/out.plan.direct.json @@ -1,6 +1,7 @@ { "email_notifications.on_failure": { - "action": "update", + "action": "skip", + "reason": "empty_slice", "new": [] }, "tasks[task_key='usage_logs_grants'].notebook_task.source": { diff --git a/acceptance/bundle/resources/jobs/tags_empty_map/databricks.yml b/acceptance/bundle/resources/jobs/tags_empty_map/databricks.yml new file mode 100644 index 0000000000..e527f031bf --- /dev/null +++ b/acceptance/bundle/resources/jobs/tags_empty_map/databricks.yml @@ -0,0 +1,8 @@ +bundle: + name: tags_empty_map + +resources: + jobs: + test_job: + name: Test Job with empty tags + tags: {} diff --git a/acceptance/bundle/resources/jobs/tags_empty_map/out.plan.direct.json b/acceptance/bundle/resources/jobs/tags_empty_map/out.plan.direct.json new file mode 100644 index 0000000000..f79692c1db --- /dev/null +++ b/acceptance/bundle/resources/jobs/tags_empty_map/out.plan.direct.json @@ -0,0 +1,22 @@ +{ + "email_notifications": { + "action": "skip", + "reason": "server_side_default", + "remote": {} + }, + "tags": { + "action": "skip", + "reason": "empty_map", + "new": {} + }, + "timeout_seconds": { + "action": "skip", + "reason": "server_side_default", + "remote": 0 + }, + "webhook_notifications": { + "action": "skip", + "reason": "server_side_default", + "remote": {} + } +} diff --git a/acceptance/bundle/resources/jobs/tags_empty_map/out.plan.terraform.json b/acceptance/bundle/resources/jobs/tags_empty_map/out.plan.terraform.json new file mode 100644 index 0000000000..dfcbe561e5 --- /dev/null +++ b/acceptance/bundle/resources/jobs/tags_empty_map/out.plan.terraform.json @@ -0,0 +1,9 @@ +{ + "plan_version": 2, + "cli_version": "[DEV_VERSION]", + "plan": { + "resources.jobs.test_job": { + "action": "skip" + } + } +} diff --git a/acceptance/bundle/resources/jobs/tags_empty_map/out.test.toml b/acceptance/bundle/resources/jobs/tags_empty_map/out.test.toml new file mode 100644 index 0000000000..d560f1de04 --- /dev/null +++ b/acceptance/bundle/resources/jobs/tags_empty_map/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/jobs/tags_empty_map/output.txt b/acceptance/bundle/resources/jobs/tags_empty_map/output.txt new file mode 100644 index 0000000000..8ad5b06021 --- /dev/null +++ b/acceptance/bundle/resources/jobs/tags_empty_map/output.txt @@ -0,0 +1,13 @@ + +>>> [CLI] bundle plan +create jobs.test_job + +Plan: 1 to add, 0 to change, 0 to delete, 0 unchanged + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/tags_empty_map/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] bundle plan -o json diff --git a/acceptance/bundle/resources/jobs/tags_empty_map/script b/acceptance/bundle/resources/jobs/tags_empty_map/script new file mode 100644 index 0000000000..61ebb0a913 --- /dev/null +++ b/acceptance/bundle/resources/jobs/tags_empty_map/script @@ -0,0 +1,3 @@ +trace $CLI bundle plan +trace $CLI bundle deploy +trace $CLI bundle plan -o json | jq '.plan."resources.jobs.test_job".changes // .' > out.plan.$DATABRICKS_BUNDLE_ENGINE.json diff --git a/acceptance/bundle/resources/jobs/tags_empty_map/test.toml b/acceptance/bundle/resources/jobs/tags_empty_map/test.toml new file mode 100644 index 0000000000..a030353d57 --- /dev/null +++ b/acceptance/bundle/resources/jobs/tags_empty_map/test.toml @@ -0,0 +1 @@ +RecordRequests = false diff --git a/bundle/deployplan/plan.go b/bundle/deployplan/plan.go index b819b81686..0aa50742e9 100644 --- a/bundle/deployplan/plan.go +++ b/bundle/deployplan/plan.go @@ -93,6 +93,8 @@ const ( ReasonRemoteAlreadySet = "remote_already_set" ReasonBuiltinRule = "builtin_rule" ReasonConfigOnly = "config_only" + ReasonEmptySlice = "empty_slice" + ReasonEmptyMap = "empty_map" ) // HasChange checks if there are any changes for fields with the given prefix. diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index e5d0277ec1..d24d938c0d 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -368,17 +368,28 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change return err } - if ch.New == nil && ch.Old == nil && ch.Remote != nil && path.IsDotString() { + if structdiff.IsEqual(ch.Remote, ch.New) { + ch.Action = deployplan.Skip + ch.Reason = deployplan.ReasonRemoteAlreadySet + } else if isEmptySlice(ch.Old, ch.New, ch.Remote) { + // Empty slice in config should not cause drift when remote has values + ch.Action = deployplan.Skip + ch.Reason = deployplan.ReasonEmptySlice + } else if isEmptyMap(ch.Old, ch.New, ch.Remote) { + // Empty map in config should not cause drift when remote has values + ch.Action = deployplan.Skip + ch.Reason = deployplan.ReasonEmptyMap + } else if action := shouldIgnore(cfg, pathString); action != deployplan.Undefined { + ch.Action = action + ch.Reason = deployplan.ReasonBuiltinRule + } else if ch.New == nil && ch.Old == nil && ch.Remote != nil && path.IsDotString() { // The field was not set by us, but comes from the remote state. // This could either be server-side default or a policy. // In any case, this is not a change we should react to. // Note, we only consider struct fields here. Adding/removing elements to/from maps and slices should trigger updates. ch.Action = deployplan.Skip ch.Reason = deployplan.ReasonServerSideDefault - } else if structdiff.IsEqual(ch.Remote, ch.New) { - ch.Action = deployplan.Skip - ch.Reason = deployplan.ReasonRemoteAlreadySet - } else if action := getActionFromConfig(cfg, pathString); action != deployplan.Undefined { + } else if action := shouldUpdateOrRecreate(cfg, pathString); action != deployplan.Undefined { ch.Action = action ch.Reason = deployplan.ReasonBuiltinRule } else { @@ -394,9 +405,7 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change return nil } -// getActionFromConfig returns the action for a field path based on resource config. -// Returns Undefined if no config applies. -func getActionFromConfig(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType { +func shouldIgnore(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType { if cfg == nil { return deployplan.Undefined } @@ -405,6 +414,13 @@ func getActionFromConfig(cfg *dresources.ResourceLifecycleConfig, pathString str return deployplan.Skip } } + return deployplan.Undefined +} + +func shouldUpdateOrRecreate(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType { + if cfg == nil { + return deployplan.Undefined + } for _, p := range cfg.RecreateOnChanges { if structpath.HasPrefix(pathString, p.String()) { return deployplan.Recreate @@ -418,6 +434,36 @@ func getActionFromConfig(cfg *dresources.ResourceLifecycleConfig, pathString str return deployplan.Undefined } +// Empty slices and maps cannot be represented in proto and because of that they cannot be represented +// by SDK's JSON encoder. However, they can be provided by users in the config and can be represented in +// Bundle struct (currently libs/structs and libs/dyn use ForceSendFields for maps and slices, unlike SDK). +// Thus we get permanent drift because we see that new config is [] but in the state it is omitted. +func isEmptySlice(values ...any) bool { + for _, v := range values { + if v == nil { + continue + } + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Slice || rv.Len() != 0 { + return false + } + } + return true +} + +func isEmptyMap(values ...any) bool { + for _, v := range values { + if v == nil { + continue + } + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Map || rv.Len() != 0 { + return false + } + } + return true +} + // TODO: calling this "Local" is not right, it can resolve "id" and remote refrences for "skip" targets func (b *DeploymentBundle) LookupReferenceLocal(ctx context.Context, path *structpath.PathNode) (any, error) { // TODO: Prefix(3) assumes resources.jobs.foo but not resources.jobs.foo.permissions