direct: Fix RemoteAlreadySet skipping input-only fields - #6112
Merged
Conversation
…tate RemoteAlreadySet skipped a field when remote == new, but remote is fabricated (fixed nil/zero) for input-only fields, so a real local change could be dropped. Gate it on !isFieldMissingInRemote in addition to !ignoreRemoteChanges so neither fabricated-remote kind trips it. Fix the cluster RemapState anti-pattern this exposed: apply_policy_default_values was copied from remote.Spec into top-level state while being absent from RemoteType, so it looked input-only but had a real remote value. Promote it into ClusterRemote, populate in DoRead from .spec, keep RemapState a dumb copy, and drop it from knownMissingInRemoteType. Match the fake server to cloud: clusters return apply_policy_default_values under .spec (only when set), pipelines echo top-level run_as. Tests: TestRemoteAlreadySetGuards (action-level, both guards); pipelines remote_matches_config acceptance test (guard A); the cluster fix and .spec fidelity are covered by the no_drift/cluster_apply_policy invariant. Document the "RemapState is a dumb copy" invariant that isFieldMissingInRemote relies on. Co-authored-by: Isaac
The prior comment overclaimed "AWS/GCP/Azure". Verified precisely: run_as is echoed as a structured run_as.user_name only in the explicit-set case on e2-dogfood (AWS). The SP-auth prod envs (aws-prod-ucws, azure-prod-ucws) can't self-bind run_as, so only the default case was exercised there (flat run_as_user_name); GCP has serverless pipelines disabled and was not tested. Co-authored-by: Isaac
Collaborator
Integration test reportCommit: 3815ce5
8 interesting tests: 4 SKIP, 3 KNOWN, 1 RECOVERED
Top 3 slowest tests (at least 2 minutes):
|
Re-verified run_as GET behavior on aws-cli, azure-cli, and gcp-cli: in the default (unset) case all three return only the flat run_as_user_name and no structured run_as. The explicit-set case still can't be confirmed on those envs because their service principal can't self-bind run_as; it is echoed as structured run_as.user_name on e2-dogfood with a real user. Co-authored-by: Isaac
janniklasrose
approved these changes
Jul 31, 2026
Comment on lines
+52
to
+56
| `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`. |
Contributor
There was a problem hiding this comment.
did you check if the rest of the codebase complies to this?
Contributor
Author
There was a problem hiding this comment.
yes, secret_scopes do not, but they are more complex.
Comment on lines
+181
to
+186
| # 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. |
Comment on lines
+61
to
+64
| "action": "update", | ||
| "old": "original@example.test", | ||
| "new": "changed@example.test", | ||
| "remote": "changed@example.test" |
Contributor
There was a problem hiding this comment.
for my understanding, we do need an update to align old & new, right?
Contributor
Author
There was a problem hiding this comment.
yes. Normally that would be skipped, but run_as is in ignore_remote_changes, so here it triggers an update.
deco-sdk-tagging Bot
added a commit
that referenced
this pull request
Aug 6, 2026
## Release v1.11.0 ### CLI * Fixed `databricks repos get/update/delete` failing with `object at path "..." is not a repo` for Git-CLI-enabled folders (currently in preview), which the workspace API reports as directories rather than repos ([#6181](#6181)). * Support `dbfs:/Skills/...` paths in `databricks fs` commands, routed to the Files API. ([#6147](#6147)) ### Bundles * For jobs where `ai_runtime_task.code_source_path` is a relative path to a local directory, the directory is now packaged into a tarball (honoring `.gitignore` and `sync.include`/`sync.exclude`), uploaded during deployment, and `code_source_path` is rewritten to the uploaded workspace path. ([#6110](#6110)) * Added JSON output to `bundle init`. Running `databricks bundle init <template> -o json` now reports the files the template wrote, relative to the output directory. This lets callers that pass `--output-dir` learn where the template materialized instead of assuming the output is a single directory named after the project. The default text output is unchanged. ([#6161](#6161)) * The terraform deployment engine is deprecated and will stop working in a future version of the CLI. Setting `bundle.engine: terraform` now emits a deprecation warning. See https://docs.databricks.com/aws/en/dev-tools/bundles/direct for how to migrate to the direct deployment engine. ([#6099](#6099)) * 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. Emptying a previously deployed list still revokes the grants, after which the node is dropped from the deployment state instead of being reported as unchanged forever. ([#6039](#6039)) * Fixed `bundle generate` downloading notebooks found inside a folder without their file extension. They are now exported like top-level notebooks, so a Python notebook lands as `notebook.py` instead of an extensionless file ([#6144](#6144)). * direct: `webhook_notifications.on_*` destinations on jobs, tasks, and `for_each_task` are now compared as unordered sets. Previously the Jobs API returning these lists in a different order than submitted produced a phantom diff that `bundle plan` and `bundle deploy` could never converge past, reporting `1 to change` on every run ([#6060](#6060)). * Fixed a pipeline with `allow_duplicate_names: true` never converging on the direct engine: the field is only accepted on create/update and is never returned by the pipelines GET API, so every subsequent `bundle plan` reported the pipeline as a perpetual update. ([#6076](#6076)) * 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. ([#6112](#6112)) * Revert usage of RedactiveSenstiveFields (added in [#5896](#5896), released in 1.10.0) which lead to incorrect behaviour (permanent drift) for duration field in Postgres resources ([#6179](#6179)). * Document postgres resource fields in the json schema ([#6164](#6164), [#6163](#6163)). * direct: Recreating a `vector_search_indexes` resource no longer fails with "Index ... is currently pending deletion" when the backend has not yet released the index name. The create is now retried until the name becomes available. ([#6143](#6143)) ### Dependency Updates * Bump `github.com/databricks/databricks-sdk-go` from v0.165.0 to v0.166.0. ([#6175](#6175)) * Upgrade Terraform provider to 1.124.0. ([#6174](#6174))
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
RemoteAlreadySet skipped a field when remote == new. But for input-only fields the remote value is fabricated (read back as a fixed nil/zero), so a real local change could coincidentally match it and be dropped from the plan. Gate the shortcut on
!isFieldMissingInRemotein addition to!ignoreRemoteChanges, so neither fabricated-remote kind trips it.This exposed a RemapState anti-pattern in clusters:
apply_policy_default_valueswas copied fromremote.Specinto top-level state while being absent from RemoteType, so it looked input-only but actually had a real remote value. Promote it intoClusterRemote, populate it inDoReadfrom.spec, keepRemapStatea dumb copy, and drop it fromknownMissingInRemoteType. The fake server is updated to match cloud (clusters returnapply_policy_default_valuesunder.spec, pipelines echo top-levelrun_as).Why
We need to know when to apply "remote already set" shortcut in the planner. Currently it can fire unexpectedly, like here https://github.com/databricks/cli/pull/5846/changes#r3675454939
With this change, we have a clear exception: if field is not in remote type, we should not check "remote already set".
Tests
TestRemoteAlreadySetGuardscovers both guards at the classifier level; a newresources/pipelines/remote_matches_configacceptance test covers the ignore_remote_changes case; the cluster fix and.specfidelity are covered by theno_drift/cluster_apply_policy_default_valuesinvariant (both verified by reverting the fix).