Repository navigation
[DECO-28500] Sync a remote run_as on jobs whose config leaves it unset
#7494
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,53 @@ | ||
| package configsync | ||
|
|
||
| import ( | ||
| "cmp" | ||
|
|
||
| "github.com/databricks/cli/bundle/deployplan" | ||
| "github.com/databricks/cli/bundle/direct/dresources" | ||
| "github.com/databricks/cli/libs/structs/structdiff" | ||
| "github.com/databricks/databricks-sdk-go/service/jobs" | ||
| ) | ||
|
|
||
| // The plan skips a job's run_as keys that the config lacks as backend defaults. | ||
| // run_as holds a single principal key, so when the remote switches kind (for | ||
| // example user_name to service_principal_name) the new key is the edit itself, | ||
| // and dropping it while removing the old key leaves "run_as: {}". | ||
| const ( | ||
| jobRunAsPattern = "resources.jobs.*.run_as" | ||
| jobRunAsChildPattern = "resources.jobs.*.run_as.*" | ||
| ) | ||
|
|
||
| // keepSkippedRunAsChange reports whether a run_as change the plan skipped as a | ||
| // backend default should still be considered for syncing. | ||
| func keepSkippedRunAsChange(path string, cd *deployplan.ChangeDesc) bool { | ||
| return cd.Reason == deployplan.ReasonBackendDefault && | ||
| (matchPattern(jobRunAsPattern, path) || matchPattern(jobRunAsChildPattern, path)) | ||
| } | ||
|
|
||
| // skipJobRunAs reports whether a change to the run_as object as a whole must | ||
| // not be synced. The plan diffs a run_as set on both sides key by key, so a | ||
| // change to the whole object means one side has none. | ||
| func skipJobRunAs(path string, cd *deployplan.ChangeDesc, remoteState any) bool { | ||
|
Contributor
There was a problem hiding this comment. With |
||
| if !matchPattern(jobRunAsPattern, path) { | ||
| return false | ||
| } | ||
| // A removed remote run_as is not synced, as before: the Jobs UI always | ||
| // submits a principal, and removal is unverified on a real workspace. | ||
| if cd.Remote == nil { | ||
| return true | ||
| } | ||
| // The config has no run_as. Skip one unchanged since the last deploy. | ||
| if structdiff.IsEqual(cd.Old, cd.Remote) { | ||
|
Contributor
There was a problem hiding this comment. major: A run_as removed from config and deployed is written back to YAML by the next sync The The Jobs API keeps the previous Scenario: deploy a job with
The result is The PR description lists only ownership transfer as a known gap, and says a Please decide whether writing the identity back is intended, since the remote job really does run as that principal. Then either filter this case, or document it as a known limitation next to the ownership-transfer gap. Either way, add an acceptance case to Agreed by 3 reviewer model(s).
Contributor
There was a problem hiding this comment. Major: After |
||
| return true | ||
| } | ||
| return isCreatorRunAs(cd.Remote.(jobs.JobRunAs), remoteState.(*dresources.JobRemote)) | ||
| } | ||
|
|
||
| // isCreatorRunAs reports whether run_as names the job's creator. Jobs updated by | ||
| // the terraform engine before it was removed carry an explicit run_as set to the | ||
| // job's identity, usually its creator, which is a default rather than an edit: | ||
| // https://github.com/databricks/terraform-provider-databricks/blob/76f95e4/jobs/resource_job.go#L844-L857 | ||
| func isCreatorRunAs(runAs jobs.JobRunAs, job *dresources.JobRemote) bool { | ||
| return cmp.Or(runAs.UserName, runAs.ServicePrincipalName, runAs.GroupName) == job.CreatorUserName | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: skipJobRunAs never checks cd.New, so a config that already has run_as gets a redundant whole-object replace
The comments at
runas.go:28-30and:40assume a whole-objectrun_aschange means the config has none, butskipJobRunAsnever checkscd.New. The plan also records a whole-object change atrun_aswhen the saved state has norun_asand the config now has one. That happens whenrun_aswas added locally (or inherited from a new bundle-levelrun_as) and not yet deployed. In that case the local diff givesOld=nil, New=<config value>, Remote=<remote value>. If the remote also has arun_asthat differs, the remote diff runs key by key (run_as.user_nameand so on), so the parent entry stays as an Update.Before this PR the
alwaysSkipentry forresources.jobs.*.run_asindefaults.godropped this parent entry. Now, when the remote principal is not the creator,skipJobRunAsreturns false.convertChangeDesc(diff.go:94) then emitsrun_as: replacewith the whole remote object, next to the existing per-keyrun_as.<key>changes.The PR's own flow can reach this: a sync adds
run_asto the YAML, nothing is deployed, the run-as is changed again in the Jobs UI, and sync runs again. The final YAML is the same, becauseResolveChangesapplies deeper paths first (resolve.go:226-246). The cost is a duplicated change in the command output and telemetry, and a whole-node rewrite of therun_asblock.Suggested fix: return true from
skipJobRunAswhencd.New != nil, since the per-key entries carry the change. Add a unit case toTestSkipJobRunAswithNewandRemoteboth set (no current row has both).Agreed by 3 reviewer model(s).