Repository navigation
direct: map secrets value/owner in DoRead; drop RemapState - #6809
Merged
Merged
Conversation
Collaborator
Integration test reportCommit: c46408b
Top 4 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/secrets-normalize-prep
branch
2 times, most recently
from
September 29, 2026 12:17
fdd6d54 to
6265ca9
Compare
denik
changed the title
denik
marked this pull request as ready for review
September 29, 2026 16:52
shreyas-goenka
approved these changes
Sep 29, 2026
denik
changed the title
denik
enabled auto-merge
September 30, 2026 08:42
denik
force-pushed
the
denik/secrets-normalize-prep
branch
from
September 30, 2026 10:11
29326d6 to
e11e3f1
Compare
ResourceSecret's RemapState renamed effective_value/effective_owner to value/owner and dropped output-only fields. Move that into a shared remapSecretRemote helper applied wherever the remote is produced (DoRead/DoCreate/DoUpdate), so the remote is already mapped and RemapState becomes an identity no-op. Remove it; the adapter returns the remote unchanged. Plan remote_state now shows the mapped value/owner and omits output-only fields; the direct-only update-value golden is regenerated. Prep for replacing RemapState with the automatic copier. Co-authored-by: Isaac <no-reply@databricks.com>
…state Co-authored-by: Isaac <no-reply@databricks.com>
…e remote StateType and RemoteType are both catalog.Secret, and every field the old RemapState reshaped is already declared ignore_remote_changes (value/owner are input_only; create_time, effective_value, metastore_id, etc. are output_only). The reshape suppressed no drift; it only discarded the output-only fields from the plan's remote_state display. Removing it: DoRead/DoCreate/DoUpdate already return the raw secret, the field classifications handle drift, and the full remote is visible again. Co-authored-by: Isaac <no-reply@databricks.com>
remapSecretRemote now sets value/owner from the effective_* fields the API returns (so persisted state matches config and re-plans show no drift) but leaves the output-only metadata (create_time, effective_value, metastore_id, ...) intact for the plan's remote_state display. Replaces the old RemapState, which zeroed those fields and thus dropped them from the display. Verified against the invariant no_drift suite. Co-authored-by: Isaac <no-reply@databricks.com>
Keeping the output-only fields means metastore_id now appears in the plan. It is a fixed testserver literal locally but a real UUID on cloud, which broke the Cloud=true update-expire-time/remove-expire-time integration runs. Add [METASTORE_ID] replacements (literal + UUID regex, Order=-1) scoped to those two tests so the golden is stable across environments. Co-authored-by: Isaac <no-reply@databricks.com>
A bare UUID regex over-matched the cloud service-principal username in the workspace path (/Workspace/Users/<uuid> -> [METASTORE_ID] instead of [USERNAME]). Instead, extract the exact metastore_id from the plan and add_repl it: a literal match at Order -100 that normalizes only that value to [METASTORE_ID] — the testserver literal locally and the real UUID on cloud — before the numeric rules run, without touching usernames. Co-authored-by: Isaac <no-reply@databricks.com>
Keeping the output-only fields drags them into the plan's changes set, and their remote values differ between the testserver and a real workspace (metastore_id, comment, timestamps), which broke the Cloud runs. Project the update-expire-time/remove-expire-time plans to expire_time -- the field these tests assert -- per the allow-list rule in testing.md. This drops the earlier per-field metastore_id add_repl and fixes the remaining comment mismatch in one move; update-value still shows the full remote_state. Co-authored-by: Isaac <no-reply@databricks.com>
The earlier projection to {expire_time} dropped value/owner as collateral, though they
normalize cleanly on cloud ([redacted]/[USERNAME]) and aren't a fidelity risk. Project to
{expire_time, value, owner} instead -- keeping those while still excluding the
fidelity-sensitive output-only fields (metastore_id, comment, timestamps).
Co-authored-by: Isaac <no-reply@databricks.com>
…e projections
jq object construction emits keys in the order listed, so {expire_time, value, owner} put
value before owner. The plan's natural order is alphabetical (owner < value), so list
{expire_time, owner, value} to match and avoid reordering the golden.
Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/secrets-normalize-prep
branch
from
September 30, 2026 11:30
694ef4e to
c46408b
Compare
denik
disabled auto-merge
October 1, 2026 09:06
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
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.
ResourceSecret'sRemapStatemappedvalue/ownerfrom the API'seffective_*fields — needed so the persisted state matches config and re-plans show no drift — but it also zeroed the output-only metadata (create_time,effective_value,metastore_id, …), dropping those fields from the plan'sremote_statedisplay.This moves the
value/ownermapping into a small in-placeremapSecretRemoteapplied inDoRead/DoCreate/DoUpdate, and stops zeroing the rest: the output-only fields are alreadyignore_remote_changes, so they cause no drift and can flow through for visibility.RemapStateis removed. Prep toward replacing hand-writtenRemapStatewith the automatic copier (#6777).This pull request and its description were written by Isaac.