Conversation
Keyed slices are matched by key value alone (the key field is only used to render the path). A diff on a matched element's own key field therefore means the two sides carry the same identity under a different field — e.g. a permission declared under user_name that the Permissions API stores and returns as service_principal_name — which is not a real change. Drop those field diffs; non-key fields still diff normally. This fixes a perpetual no-op "update" of dashboard/job/etc permissions when a service principal is declared under user_name, generically for any keyed slice, without per-resource logic. Alternative to the resource-level fix in #6710. The testserver models the backend's user_name(UUID) -> service_principal_name readback so the case reproduces locally. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: fcb6e08
Top 3 slowest tests (at least 2 minutes):
|
prepareChanges indexed changes by the full path string, so a keyed element whose key field differs between the saved state and the backend (e.g. a principal saved under user_name but read back as service_principal_name) produced two half-populated change entries for one field. Index by the key-field-agnostic path (structpath.KeyValueAgnosticString) so the local and remote views merge into one entry; the displayed path stays the local one. Also document the value-match / key-field-drop semantics on structdiff.KeyFunc and cross-reference it from IResource.KeyedSlices. Co-authored-by: Isaac <no-reply@databricks.com>
This branch has not been deployed
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
2 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.
Problem
When a service principal is declared in bundle permissions under
user_name(its application-ID UUID), the Permissions API stores and returns it asservice_principal_name. The direct engine's plan compared desired (user_name) against remote (service_principal_name) and reported a no-opupdateof the ACL on every run — a false positive that never converged. Reported by a customer after migrating to the direct engine.Fix
Keyed slices are already matched by key value (the key field is only used to render the path). So within a matched pair, a diff on the element's own key field means the two sides carry the same identity under a different field — not a real change.
structdiff.diffSliceByKeynow drops those key-field diffs; non-key fields (e.g. permission level) still diff normally.This fixes the drift generically for any keyed slice with interchangeable identity fields, with no per-resource logic — the permissions resource stays as just
KeyedSlices. It's value-based, so GCP name-based service principals are handled too.The testserver models the backend's
user_name(UUID) →service_principal_namereadback, and a new acceptance test asserts the plan converges after deploy and that a genuine level change is still reported.Alternative to #6710
This is the generic-diff alternative to the resource-level
OverrideChangeDescfix in #6710. Verified no golden moves across the keyed-slice surface (jobs, grants, secret scopes, permissions, run_as, dms) and the fullbundle/invariantno_drift sweep — the change is a no-op for slices whose key field is constant, and the intended fix for permissions.This pull request and its description were written by Isaac.