diff --git a/.nextchanges/bundles/remote-already-set-input-only.md b/.nextchanges/bundles/remote-already-set-input-only.md new file mode 100644 index 00000000000..6782e5a3502 --- /dev/null +++ b/.nextchanges/bundles/remote-already-set-input-only.md @@ -0,0 +1 @@ +direct: A local change to an input-only field (one the API accepts on write but never returns on read, e.g. pipelines' `run_as` or external locations' `skip_validation`) is no longer silently skipped when the new value coincidentally matches the field's fabricated remote value. Previously such a change could hit the `remote_already_set` shortcut and be dropped from the plan. diff --git a/acceptance/bundle/migrate/runas/out.pipelines_get.json b/acceptance/bundle/migrate/runas/out.pipelines_get.json index 715c10e9e13..fb06fd6bdb6 100644 --- a/acceptance/bundle/migrate/runas/out.pipelines_get.json +++ b/acceptance/bundle/migrate/runas/out.pipelines_get.json @@ -4,6 +4,9 @@ "last_modified": [UNIX_TIME_MILLIS], "name": "DABs Revenue Pipeline", "pipeline_id": "[UUID]", + "run_as": { + "service_principal_name": "[UUID]" + }, "run_as_user_name": "[USERNAME]", "spec": { "catalog": "main", diff --git a/acceptance/bundle/migrate/runas/out.plan.json b/acceptance/bundle/migrate/runas/out.plan.json index 231903729b8..0888cdc22ef 100644 --- a/acceptance/bundle/migrate/runas/out.plan.json +++ b/acceptance/bundle/migrate/runas/out.plan.json @@ -27,22 +27,13 @@ ], "name": "DABs Revenue Pipeline", "pipeline_id": "[UUID]", + "run_as": { + "service_principal_name": "[UUID]" + }, "run_as_user_name": "[USERNAME]", "serverless": true, "state": "IDLE", "target": "team_eng_deco" - }, - "changes": { - "run_as": { - "action": "skip", - "reason": "input_only", - "old": { - "service_principal_name": "[UUID]" - }, - "new": { - "service_principal_name": "[UUID]" - } - } } }, "resources.pipelines.foo.permissions": { diff --git a/acceptance/bundle/refschema/out.fields.txt b/acceptance/bundle/refschema/out.fields.txt index 2d7f6d65536..ffec0976857 100644 --- a/acceptance/bundle/refschema/out.fields.txt +++ b/acceptance/bundle/refschema/out.fields.txt @@ -279,7 +279,7 @@ resources.catalogs.*.grants[*] catalog.PrivilegeAssignment ALL resources.catalogs.*.grants[*].principal string ALL resources.catalogs.*.grants[*].privileges []catalog.Privilege ALL resources.catalogs.*.grants[*].privileges[*] catalog.Privilege ALL -resources.clusters.*.apply_policy_default_values bool INPUT STATE +resources.clusters.*.apply_policy_default_values bool ALL resources.clusters.*.autoscale *compute.AutoScale ALL resources.clusters.*.autoscale.max_workers int ALL resources.clusters.*.autoscale.min_workers int ALL diff --git a/acceptance/bundle/resources/pipelines/remote_matches_config/databricks.yml b/acceptance/bundle/resources/pipelines/remote_matches_config/databricks.yml new file mode 100644 index 00000000000..c8b9ac895e7 --- /dev/null +++ b/acceptance/bundle/resources/pipelines/remote_matches_config/databricks.yml @@ -0,0 +1,13 @@ +bundle: + name: test-bundle + +resources: + pipelines: + my_pipeline: + name: "Test Pipeline run_as guard" + serverless: true + libraries: + - notebook: + path: /Users/{{workspace_user_name}}/test_notebook + run_as: + user_name: original@example.test diff --git a/acceptance/bundle/resources/pipelines/remote_matches_config/out.plan.direct.json b/acceptance/bundle/resources/pipelines/remote_matches_config/out.plan.direct.json new file mode 100644 index 00000000000..c07de448831 --- /dev/null +++ b/acceptance/bundle/resources/pipelines/remote_matches_config/out.plan.direct.json @@ -0,0 +1,74 @@ +{ + "plan_version": 2, + "cli_version": "[CLI_VERSION]", + "lineage": "[UUID]", + "serial": 1, + "plan": { + "resources.pipelines.my_pipeline": { + "action": "update", + "new_state": { + "value": { + "channel": "CURRENT", + "deployment": { + "kind": "BUNDLE", + "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json" + }, + "edition": "ADVANCED", + "libraries": [ + { + "notebook": { + "path": "/Users/{{workspace_user_name}}/test_notebook" + } + } + ], + "name": "Test Pipeline run_as guard", + "run_as": { + "user_name": "changed@example.test" + }, + "serverless": true + } + }, + "remote_state": { + "channel": "CURRENT", + "creator_user_name": "[USERNAME]", + "deployment": { + "kind": "BUNDLE", + "metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json" + }, + "edition": "ADVANCED", + "effective_publishing_mode": "DEFAULT_PUBLISHING_MODE", + "id": "[MY_PIPELINE_ID]", + "last_modified": [UNIX_TIME_MILLIS], + "libraries": [ + { + "notebook": { + "path": "/Users/{{workspace_user_name}}/test_notebook" + } + } + ], + "name": "Test Pipeline run_as guard", + "pipeline_id": "[MY_PIPELINE_ID]", + "run_as": { + "user_name": "changed@example.test" + }, + "run_as_user_name": "[USERNAME]", + "serverless": true, + "state": "IDLE", + "storage": "dbfs:/pipelines/[MY_PIPELINE_ID]" + }, + "changes": { + "run_as.user_name": { + "action": "update", + "old": "original@example.test", + "new": "changed@example.test", + "remote": "changed@example.test" + }, + "storage": { + "action": "skip", + "reason": "backend_default", + "remote": "dbfs:/pipelines/[MY_PIPELINE_ID]" + } + } + } + } +} diff --git a/acceptance/bundle/resources/pipelines/remote_matches_config/out.plan.terraform.json b/acceptance/bundle/resources/pipelines/remote_matches_config/out.plan.terraform.json new file mode 100644 index 00000000000..ae88ec10c4d --- /dev/null +++ b/acceptance/bundle/resources/pipelines/remote_matches_config/out.plan.terraform.json @@ -0,0 +1,8 @@ +{ + "cli_version": "[CLI_VERSION]", + "plan": { + "resources.pipelines.my_pipeline": { + "action": "update" + } + } +} diff --git a/acceptance/bundle/resources/pipelines/remote_matches_config/out.test.toml b/acceptance/bundle/resources/pipelines/remote_matches_config/out.test.toml new file mode 100644 index 00000000000..f784a183258 --- /dev/null +++ b/acceptance/bundle/resources/pipelines/remote_matches_config/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/pipelines/remote_matches_config/output.txt b/acceptance/bundle/resources/pipelines/remote_matches_config/output.txt new file mode 100644 index 00000000000..19ee7fce845 --- /dev/null +++ b/acceptance/bundle/resources/pipelines/remote_matches_config/output.txt @@ -0,0 +1,13 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Make local change: run_as user -> changed@example.test +=== Force remote run_as to match the new config value +>>> [CLI] bundle plan +update pipelines.my_pipeline + +Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged diff --git a/acceptance/bundle/resources/pipelines/remote_matches_config/script b/acceptance/bundle/resources/pipelines/remote_matches_config/script new file mode 100644 index 00000000000..5b9c37402c7 --- /dev/null +++ b/acceptance/bundle/resources/pipelines/remote_matches_config/script @@ -0,0 +1,20 @@ +# run_as is declared ignore_remote_changes: input_only. This test forces the situation where +# a genuine local change (state has run_as A, config changes it to B) coincides with remote +# already reading back B. Before the RemoteAlreadySet input-only fix, remote == new wrongly +# skipped the change (reason: remote_already_set); it must instead classify as an update. + +trace $CLI bundle deploy +pipeline_id="$(read_id.py my_pipeline)" + +title "Make local change: run_as user -> changed@example.test" +update_file.py databricks.yml "original@example.test" "changed@example.test" + +title "Force remote run_as to match the new config value" +edit_resource.py pipelines "$pipeline_id" < out.plan.$DATABRICKS_BUNDLE_ENGINE.json + +rm out.requests.txt diff --git a/acceptance/bundle/resources/pipelines/remote_matches_config/test.toml b/acceptance/bundle/resources/pipelines/remote_matches_config/test.toml new file mode 100644 index 00000000000..eb672886872 --- /dev/null +++ b/acceptance/bundle/resources/pipelines/remote_matches_config/test.toml @@ -0,0 +1,2 @@ +RecordRequests = true +Ignore = [".databricks"] diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index fda8180cfc4..669c187aac5 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -385,7 +385,13 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change return err } - if structdiff.IsEqual(ch.Remote, ch.New) { + // RemoteAlreadySet only holds when ch.Remote is a real remote value we can compare + // against ch.New. Skip it for fields whose remote value is fabricated and thus + // meaningless: declared ignore_remote_changes (present in RemoteType but read back + // backend-managed/input-only), or absent from RemoteType (a guaranteed-nil + // placeholder, since RemapState is a dumb copy). Otherwise a coincidental + // new == remote (both nil, say) wrongly skips a real local change. + if structdiff.IsEqual(ch.Remote, ch.New) && !ignoreRemoteChanges(cfg, generatedCfg, path) && !isFieldMissingInRemote(adapter, path) { ch.Action = deployplan.Skip ch.Reason = deployplan.ReasonRemoteAlreadySet } else if allEmpty(ch.Old, ch.New, ch.Remote) { @@ -455,8 +461,28 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change return nil } +func ignoreRemoteChanges(cfg1, cfg2 *dresources.ResourceLifecycleConfig, path *structpath.PathNode) bool { + if _, ok := findMatchingRule(path, cfg1.IgnoreRemoteChanges); ok { + return true + } + + if _, ok := findMatchingRule(path, cfg2.IgnoreRemoteChanges); ok { + return true + } + + return false +} + // isFieldMissingInRemote reports whether path exists in StateType but is absent from RemoteType. // Such fields are accepted by the API on write but not returned by GET. +// +// Because RemapState is a dumb subset copy (see the "RemapState is a dumb copy" section in +// dresources/README.md), a field absent from RemoteType is always nil/zero in the remapped +// remote state the planner compares against (ch.Remote): there is nothing for RemapState to +// copy it from, so it can never appear. That guarantee is what makes this a sound signal that +// the field's remote value is meaningless. Fields the API returns under a different path are +// added to RemoteType and populated in DoRead, so they are NOT missing here and keep their +// real remote value. func isFieldMissingInRemote(adapter *dresources.Adapter, path *structpath.PathNode) bool { if structaccess.ValidatePath(adapter.StateType(), path) != nil { return false diff --git a/bundle/direct/bundle_plan_test.go b/bundle/direct/bundle_plan_test.go index 22db70ef62f..37605302874 100644 --- a/bundle/direct/bundle_plan_test.go +++ b/bundle/direct/bundle_plan_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/yamlloader" "github.com/databricks/cli/libs/structs/structpath" + "github.com/databricks/databricks-sdk-go/service/pipelines" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -168,6 +169,102 @@ func TestShouldSkipBackendDefault_ManagedPropertiesOnly(t *testing.T) { } } +// A field present in StateType but absent from RemoteType (input-only, e.g. external +// locations' skip_validation) reads back nil, so a coincidental new == remote must NOT +// trip RemoteAlreadySet — that would skip a real local change. A field present in RemoteType +// (jobs' max_concurrent_runs) is not affected. +func TestIsFieldMissingInRemote(t *testing.T) { + adapters, err := dresources.InitAll(nil) + require.NoError(t, err) + + tests := []struct { + resource string + path string + expected bool + }{ + {"external_locations", "skip_validation", true}, + {"jobs", "max_concurrent_runs", false}, + {"jobs", "name", false}, + } + + for _, tt := range tests { + t.Run(tt.resource+"."+tt.path, func(t *testing.T) { + adapter, ok := adapters[tt.resource] + require.True(t, ok) + path, err := structpath.ParsePath(tt.path) + require.NoError(t, err) + assert.Equal(t, tt.expected, isFieldMissingInRemote(adapter, path)) + }) + } +} + +// TestRemoteAlreadySetGuards drives addPerFieldActions directly to pin the two guards that +// keep the RemoteAlreadySet shortcut from swallowing a real local change when the remote +// value is fabricated (read back as a fixed nil/zero rather than the true remote value): +// - a field declared ignore_remote_changes (present in RemoteType, e.g. pipelines' run_as) +// - a field absent from RemoteType (missing_in_remote, e.g. external_locations' skip_validation) +// +// In both cases a genuine local change (old != new) that coincidentally makes new == remote +// must still classify as Update, not Skip/remote_already_set. +func TestRemoteAlreadySetGuards(t *testing.T) { + adapters, err := dresources.InitAll(nil) + require.NoError(t, err) + + runAs := &pipelines.RunAs{UserName: "someone@example.test"} + + tests := []struct { + name string + resource string + field string + ch *deployplan.ChangeDesc + expectedAction deployplan.ActionType + expectedReason string + }{ + { + // ignore_remote_changes guard: run_as read back nil, config changed A -> B. + name: "ignore_remote_changes real local change", + resource: "pipelines", + field: "run_as", + ch: &deployplan.ChangeDesc{Old: runAs, New: nil, Remote: nil}, + expectedAction: deployplan.Update, + }, + { + // missing_in_remote guard: skip_validation absent from RemoteType (remote nil), + // config unset the field it previously had. + name: "missing_in_remote real local change", + resource: "external_locations", + field: "skip_validation", + ch: &deployplan.ChangeDesc{Old: true, New: nil, Remote: nil}, + expectedAction: deployplan.Update, + }, + { + // Control: a real remote value that matches config is correctly skipped. + name: "genuine remote_already_set is skipped", + resource: "jobs", + field: "max_concurrent_runs", + ch: &deployplan.ChangeDesc{Old: 1, New: 2, Remote: 2}, + expectedAction: deployplan.Skip, + expectedReason: deployplan.ReasonRemoteAlreadySet, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + adapter, ok := adapters[tt.resource] + require.True(t, ok) + changes := deployplan.Changes{tt.field: tt.ch} + err := addPerFieldActions(t.Context(), adapter, changes, nil) + require.NoError(t, err) + assert.Equal(t, tt.expectedAction, tt.ch.Action) + if tt.expectedReason != "" { + assert.Equal(t, tt.expectedReason, tt.ch.Reason) + } else { + assert.NotEqual(t, deployplan.ReasonRemoteAlreadySet, tt.ch.Reason, "a real local change must not be skipped as remote_already_set") + } + }) + } +} + // Map drift handling synthesizes child paths to match against rules. structdiff // always emits map keys in bracket notation, so synthetic child paths must too; // otherwise rules wouldn't match for identifier-like keys. diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index da9e9edab28..472631f0e38 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -47,17 +47,35 @@ If the API may return a slice's elements in a different order between calls (e.g The state struct is serialized to JSON and persisted between deploys. Backward incompatible changes will result in a drift, which depending on field behaviour might result in recreate. See dstate/migrate.go on how to handle state migration. -## RemapState and missing remote fields +## RemapState is a dumb copy; DoRead owns all remapping -Do not populate a field in `RemapState` by mapping it from a differently-named field in `RemoteType`. When a field is absent from `RemoteType` the engine automatically suppresses remote drift for it (reason: `missing_in_remote`), which means any value `RemapState` sets there is invisible to drift detection — real remote changes go undetected. +`RemapState` converts `RemoteType` to `StateType` only because `StateType` is typically a +subset of `RemoteType`. It must be a field-by-field copy (or no-op), never a place for +logic. In particular, do not remap a differently-named field there (e.g. `state.x = remote.status.x`). +Any remapping the API requires belongs in `DoRead`: add `x` directly to `RemoteType` and +populate it from `status.x` inside `DoRead`. -The correct pattern is: +This is not a style preference — the plan classifier depends on it. Because `RemapState` is a +dumb copy, a field absent from `RemoteType` is always nil/zero in the remapped remote state the +planner compares against: there is nothing to copy it from. So "path present in `StateType` but +absent from `RemoteType`" reliably means "field the API accepts on write but never echoes on +read," and its remote value is guaranteed meaningless (see `isFieldMissingInRemote` / +`missing_in_remote` in `bundle_plan.go`). + +A `RemapState` that instead synthesizes `state.x = remote.status.x` breaks this: `x` is absent +from `RemoteType`, so the classifier treats it as input-only and skips it (`missing_in_remote`), +yet the field actually has a real remote value under `status.x`. Genuine remote drift is then +silently suppressed. The fix is to make the field genuinely present in `RemoteType`, not to +smuggle it in via `RemapState`. + +So when a field comes back under a different path than `StateType` uses: 1. Add the field to `RemoteType` (the struct returned by `DoRead`). 2. Populate it in `DoRead` by mapping from whatever the API returns under the other name. -3. Keep `RemapState` trivial (a direct struct copy or no-op). +3. Keep `RemapState` a trivial subset copy. -This makes the field present in `InputType`, `StateType`, and `RemoteType`, so it participates in normal drift detection and is no longer subject to the `missing_in_remote` suppression. +This makes the field present in `InputType`, `StateType`, and `RemoteType`, so it participates +in normal drift detection and is no longer subject to the `missing_in_remote` suppression. ## OverrideChangeDesc diff --git a/bundle/direct/dresources/cluster.go b/bundle/direct/dresources/cluster.go index d253c1129ad..0605d50b86c 100644 --- a/bundle/direct/dresources/cluster.go +++ b/bundle/direct/dresources/cluster.go @@ -48,9 +48,15 @@ func (s ClusterState) MarshalJSON() ([]byte, error) { // ClusterRemote extends compute.ClusterDetails with a synthetic Lifecycle field so that // RemoteType satisfies TestRemoteSuperset (every field in ClusterState exists in ClusterRemote). // Lifecycle.Started is populated by DoRead from the cluster's running state. +// +// ApplyPolicyDefaultValues is promoted to the top level because the cluster GET API returns it +// only under .spec (a snapshot of the create/edit settings), never at the top level. DoRead +// copies it up so RemapState stays a dumb copy and the field participates in normal drift +// detection instead of being suppressed as missing_in_remote. type ClusterRemote struct { compute.ClusterDetails - Lifecycle *StateLifecycle `json:"lifecycle,omitempty"` + ApplyPolicyDefaultValues bool `json:"apply_policy_default_values,omitempty"` + Lifecycle *StateLifecycle `json:"lifecycle,omitempty"` } func (r *ClusterRemote) UnmarshalJSON(b []byte) error { @@ -88,7 +94,7 @@ func (r *ResourceCluster) RemapState(input *ClusterRemote) *ClusterState { started := input.State == compute.StateRunning spec := &ClusterState{ ClusterSpec: compute.ClusterSpec{ - ApplyPolicyDefaultValues: false, + ApplyPolicyDefaultValues: input.ApplyPolicyDefaultValues, Autoscale: input.Autoscale, AutoterminationMinutes: input.AutoterminationMinutes, AwsAttributes: input.AwsAttributes, @@ -127,9 +133,6 @@ func (r *ResourceCluster) RemapState(input *ClusterRemote) *ClusterState { }, Lifecycle: &StateLifecycle{Started: &started}, } - if input.Spec != nil { - spec.ApplyPolicyDefaultValues = input.Spec.ApplyPolicyDefaultValues - } return spec } @@ -139,8 +142,14 @@ func (r *ResourceCluster) DoRead(ctx context.Context, id string) (*ClusterRemote return nil, err } remote := &ClusterRemote{ - ClusterDetails: *details, - Lifecycle: nil, + ClusterDetails: *details, + ApplyPolicyDefaultValues: false, + Lifecycle: nil, + } + // The GET response carries apply_policy_default_values only under .spec (a snapshot of the + // create/edit settings), not at the top level. Promote it so RemapState is a dumb copy. + if details.Spec != nil { + remote.ApplyPolicyDefaultValues = details.Spec.ApplyPolicyDefaultValues } switch details.State { diff --git a/bundle/direct/dresources/config_test.go b/bundle/direct/dresources/config_test.go index 314c3548015..33345306b0b 100644 --- a/bundle/direct/dresources/config_test.go +++ b/bundle/direct/dresources/config_test.go @@ -93,6 +93,12 @@ func TestResourcesYMLNoRedundantRules(t *testing.T) { // in resources.yml do not duplicate the automatic missing-in-remote suppression. A field // absent from RemoteType is already skipped automatically (reason: missing_in_remote) when // there is no local change, so a manual ignore_remote_changes entry for it is dead weight. +// +// This rests on RemapState being a dumb subset copy: absence from RemoteType reliably means +// the field is input-only (accepted on write, never echoed on read). See the "RemapState is +// a dumb copy" section in README.md. Note the redundancy claim only holds for the no-local-change +// case — missing_in_remote carries an old==new guard, so a write-only field that legitimately +// transitions old!=new->nil is NOT auto-handled and a declared entry there is load-bearing. func TestResourcesYMLNoRedundantMissingInRemote(t *testing.T) { cfg := MustLoadConfig() for resourceType, rc := range cfg.Resources { diff --git a/bundle/direct/dresources/resources.yml b/bundle/direct/dresources/resources.yml index c9df091551f..13584209788 100644 --- a/bundle/direct/dresources/resources.yml +++ b/bundle/direct/dresources/resources.yml @@ -178,6 +178,12 @@ resources: # Thus it shows up as a remote change since we don't set on the object. - field: id reason: "!drop" + # QQQ should this be here? When run_as is explicitly set, the GET response echoes it back + # as a structured run_as.user_name (verified on e2-dogfood with a real user), so it may not + # be truly input-only. The explicit-set case could not be confirmed on aws-cli, azure-cli, + # or gcp-cli: those envs authenticate as a service principal that lacks servicePrincipal.user + # on itself, so it can't self-bind run_as. In the default (unset) case on all three clouds, + # GET returns only the flat run_as_user_name and no structured run_as. - field: run_as reason: input_only diff --git a/bundle/direct/dresources/type_test.go b/bundle/direct/dresources/type_test.go index ac04be010b9..6f0327a3342 100644 --- a/bundle/direct/dresources/type_test.go +++ b/bundle/direct/dresources/type_test.go @@ -16,9 +16,6 @@ import ( // These are known issues that should be fixed. If a field listed here is found in RemoteType, // the test fails to ensure the entry is removed from this map. var knownMissingInRemoteType = map[string][]string{ - "clusters": { - "apply_policy_default_values", - }, "external_locations": { "skip_validation", }, diff --git a/libs/testserver/clusters.go b/libs/testserver/clusters.go index 0d55c82dcaa..80991b0091a 100644 --- a/libs/testserver/clusters.go +++ b/libs/testserver/clusters.go @@ -34,6 +34,11 @@ func (s *FakeWorkspace) ClustersCreate(req Request) any { clusterFixUps(&request) + // The cluster GET API returns apply_policy_default_values only under .spec, not at the top + // level (compute.ClusterDetails has no such field, so it is dropped by the unmarshal above). + // Snapshot it from the raw body so re-reads match cloud. + request.Spec = specSnapshot(req.Body) + s.Clusters[clusterId] = request return Response{ @@ -43,6 +48,23 @@ func (s *FakeWorkspace) ClustersCreate(req Request) any { } } +// specSnapshot mirrors how the cluster GET API returns apply_policy_default_values under .spec +// (it is absent from compute.ClusterDetails and thus invisible at the top level of a re-read). +// Returns nil when the field is unset so re-reads of clusters that don't use it are unchanged; +// a nil .spec reads back as false, which is what the field's absence means anyway. +func specSnapshot(body []byte) *compute.ClusterSpec { + var spec compute.ClusterSpec + if err := json.Unmarshal(body, &spec); err != nil { + return nil + } + if !spec.ApplyPolicyDefaultValues { + return nil + } + return &compute.ClusterSpec{ + ApplyPolicyDefaultValues: true, + } +} + func (s *FakeWorkspace) ClustersResize(req Request) any { var request compute.ResizeCluster if err := json.Unmarshal(req.Body, &request); err != nil { @@ -92,6 +114,8 @@ func (s *FakeWorkspace) ClustersEdit(req Request) any { request.State = existing.State request.ClusterId = existing.ClusterId clusterFixUps(&request) + // Refresh the .spec snapshot from the new settings, matching cloud behavior on edit. + request.Spec = specSnapshot(req.Body) s.Clusters[request.ClusterId] = request // Clear venv cache when cluster is edited to match cloud behavior where diff --git a/libs/testserver/pipelines.go b/libs/testserver/pipelines.go index 3512c8e5f5f..7e7aa262483 100644 --- a/libs/testserver/pipelines.go +++ b/libs/testserver/pipelines.go @@ -69,6 +69,11 @@ func (s *FakeWorkspace) PipelineCreate(req Request) Response { r.CreatorUserName = "tester@databricks.com" r.LastModified = nowMilli() r.Name = r.Spec.Name + // run_as is on CreatePipeline, not PipelineSpec, so the spec decode drops it. The backend + // echoes it top-level on GetPipelineResponse.RunAs; mirror that so a re-read is faithful. + if create.RunAs != nil { + r.RunAs = create.RunAs + } r.RunAsUserName = "tester@databricks.com" r.State = "IDLE" r.EffectivePublishingMode = pipelines.PublishingModeDefaultPublishingMode @@ -124,6 +129,11 @@ func (s *FakeWorkspace) PipelineUpdate(req Request, pipelineId string) Response item.Spec = &spec item.Parameters = edit.Parameters + // run_as is on EditPipeline, not PipelineSpec; keep it in sync like Parameters so an edit + // that changes run_as is reflected on the next read (matches cloud top-level echo). + if edit.RunAs != nil { + item.RunAs = edit.RunAs + } setSpecDefaults(&spec, pipelineId) s.Pipelines[pipelineId] = item