direct: store a dashboard contents digest in state instead of the contents - #6105
Open
Sankalp-Mittal wants to merge 40 commits into
Open
direct: store a dashboard contents digest in state instead of the contents#6105Sankalp-Mittal wants to merge 40 commits into
Sankalp-Mittal wants to merge 40 commits into
Conversation
…ash but full content is always sent to the API
Contributor
Approval status: pending
|
Sankalp-Mittal
marked this pull request as draft
July 30, 2026 12:41
Collaborator
Integration test reportCommit: 7b5f9c7
19 interesting tests: 7 flaky, 4 SKIP, 4 FAIL, 3 RECOVERED, 1 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
Sankalp-Mittal
marked this pull request as ready for review
July 30, 2026 15:55
Empty commit to re-run the pipeline; the previous integration run failed only on gcp-linux TestFsCp* tests, which this PR does not touch (unrelated infra flake). Co-authored-by: Isaac
denik
reviewed
Aug 4, 2026
denik
left a comment
denik
left a comment
Contributor
There was a problem hiding this comment.
Can we automatically migrate old state so that there is no drift when we load state with full serialized dashboard?
It would be nice to have acc test for this case.
| "parent_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/resources", | ||
| "published": true, | ||
| "serialized_dashboard": "{\"pages\":[{\"displayName\":\"Page One\",\"name\":\"02724bf2\"}]}", | ||
| "serialized_dashboard": "sha256_hashed_in_state:[HASH]", |
Contributor
There was a problem hiding this comment.
"sha256_hashed_in_state:[HASH]"
could just be sha256:[HASH]
Also, do we need a replacement for HASH here, dopes it change?
Collaborator
Author
There was a problem hiding this comment.
- The long name was to avoid potential future conflicts in naming as right now this prefix is used to check if a field has been hashed or not
- Added the replacement for [HASH] since makes the output much more readable.
|
|
||
| // OfJSON returns a deterministic sha256 hex digest of v's JSON encoding. | ||
| // json.Marshal sorts map keys, so the digest is stable across runs for equal values. | ||
| func OfJSON(v any) (string, error) { |
Contributor
There was a problem hiding this comment.
Let's try to reduce size of this PR. We can drop this package, leave libs/cache unchanged and inline OfJSON in the place where it's used.
shreyas-goenka
left a comment
shreyas-goenka
left a comment
Contributor
There was a problem hiding this comment.
Thanks! Looks good to me mostly. Can you see whether we can avoid this showing up in the plan diff? If that's a big lift or not?
"old": "sha256_hashed_in_state:[HASH][0]",
"new": "sha256_hashed_in_state:[HASH][0]",
"remote": "sha256_hashed_in_state:[HASH][1]"
| // stateHashPrefix marks a state value that holds a content hash instead of the | ||
| // raw value. Since this is part of the on-disk state format, changing it is not | ||
| // backwards compatible. | ||
| const stateHashPrefix = "sha256_hashed_in_state:" |
Contributor
There was a problem hiding this comment.
the prefix can just be
Suggested change
| const stateHashPrefix = "sha256_hashed_in_state:" | |
| const stateHashPrefix = "sha256" |
Collaborator
Author
There was a problem hiding this comment.
The long name was to avoid potential future conflicts in naming as right now this prefix is used to check if a field has been hashed or not (a proxy for checking if the data is stored according to this new format)
|
|
||
| // TestHashStateValueIdempotent verifies re-hashing an existing placeholder returns it | ||
| // unchanged, so re-compacting an already-compact state does not double-hash. | ||
| func TestHashStateValueIdempotent(t *testing.T) { |
Contributor
There was a problem hiding this comment.
This test can be removed. We already have coverage above.
| "old": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\"}]}", | ||
| "new": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\"}]}", | ||
| "remote": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\",\"pageType\":\"PAGE_TYPE_CANVAS\"}]}" | ||
| "old": "sha256_hashed_in_state:[HASH][0]", |
Contributor
There was a problem hiding this comment.
Can we continue to show the same output in plan? Rather than the hash? Two reasons:
- The plan can be serialized to a file and be consumed again.
- Users should be able to filter out and see the exact diff.
Can you also confirm that after these changes a plan serialized and used with a --plan flag continues to work?
Contributor
There was a problem hiding this comment.
For me ensuring (1) is more important. (2) is optional and maybe compacting the plan is actually better for readibility.
Collaborator
Author
There was a problem hiding this comment.
We won't get the diff if we want to hash since there is no way to retrieve the old dashboard state from the hash anyways
| "old": "{\"pages\":[{\"name\":\"02724bf2\",\"displayName\":\"Dashboard test bundle-deploy-dashboard\"}]}\n", | ||
| "new": "{\"pages\":[{\"name\":\"02724bf2\",\"displayName\":\"Dashboard test bundle-deploy-dashboard\"}]}\n", | ||
| "remote": "{\"pages\":[{\"displayName\":\"Dashboard test bundle-deploy-dashboard\",\"name\":\"02724bf2\",\"pageType\":\"PAGE_TYPE_CANVAS\"}]}\n" | ||
| "old": "sha256_hashed_in_state:[HASH][0]", |
Contributor
|
This is a high risk change for serialized plan, so please convert dashboard tests to do EnvMatrix.READPLAN variant (ideally as a separate PR so we can confirm it all works on main). |
Reimplements the state-size fix using the approach Dennis outlined in review: instead of overwriting serialized_dashboard in state with a hash, keep the field out of the comparison entirely and carry the change signal in a new small sibling field, serialized_dashboard_hash. Overwriting the field in place forced every value entering the diff to be hashed too (saved, local config, remapped remote), since one hashed side makes any hash-vs-content comparison meaningless. That needed a generic hashed_in_state config mechanism, a CompactState helper and five call sites, and it replaced the real dashboard content in plan output with an opaque digest. Now: - DashboardState carries SerializedDashboardHash, computed in PrepareState. It lives on the state struct, not DashboardConfig, so it stays out of the bundle schema. - DoCreate/DoUpdate drop the contents after the API call, so the contents reach the API but never the state file. - serialized_dashboard is ignore_local_changes (state holds no contents) and the digest is ignore_remote_changes (the API does not return it). The digest is deliberately NOT ignore_local_changes: a saved-vs-config difference on it is the change signal. - New optional PrepareOldState adapter hook covers the paths that bypass DoCreate/DoUpdate: state written by an older CLI (the digest is derived from the contents already there, so upgrading does not trigger an update), plus bundle bind and bundle migrate, which write state directly. The digest is taken over the same rendered string sent to the API, so an inlined (map) dashboard and a path-loaded (string) one digest identically; digesting the raw config value would produce permanent phantom drift. Drops state_compaction.go, libs/hash and the hashed_in_state mechanism, and restores libs/cache to its original state, addressing both review comments. bundle plan again shows the real dashboard content diff. Co-authored-by: Isaac
Asserts that state written by an older CLI (full contents, no digest) normalizes to the same digest PrepareState computes from unchanged config, so upgrading a bundle does not report a content-less dashboard update on the next deploy. A real content edit must still differ. This is the case PrepareOldState exists for and the one no acceptance test reaches, since the acceptance harness always writes state with the current CLI. Co-authored-by: Isaac
Sankalp-Mittal
changed the title
The digest is now stored as "sha256_hashed_in_state:<hex>" rather than a bare hex string. The prefix anchors the acceptance-test masking rule, so a single [[Repls]] entry in acceptance/bundle/test.toml covers every dashboard test. A bare 64-hex pattern cannot live at that level: it also matches the content-addressed snapshot hashes that the immutable tests mask as [SNAPSHOT_HASH], and at Order = -1 it wins, rewriting their goldens. That forced the rule to be duplicated across four dashboard directories, two of which needed a test.toml created for it. With the prefix the pattern is unambiguous and those four files drop out of the diff. Co-authored-by: Isaac
…ention Rename the tests added for the contents digest to the Test<Resource><Function><Behavior> form used by the sibling resource tests (app_test.go, genie_space_test.go, job_test.go), dropping the underscore separators, and give each one a doc comment stating what it verifies and why that matters. Splits the digest test in two, since it was covering two unrelated properties: that the map and string config forms digest identically, and that the digest format and contents sensitivity hold. Extracts the repeated dashboard contents into a shared const, and replaces the inline rule-matching closure with a named helper per the Go style guide. No change in coverage. Co-authored-by: Isaac
The old name described where the hook is called from rather than what it does, and it was only accurate at one of its three call sites: `bundle bind` and `bundle migrate` pass state that is about to be written, not state read back from disk. Renames the interface method, the adapter field and wrapper, the dashboard implementation and its tests, and updates the doc comments and README, which described the hook as "normalizing state into the shape this CLI persists" — true but vague now that the name says hashing. Behaviour is unchanged. The adapter resolves the method by name through reflection (calladapt), so the string literal in initMethods moves with it; verified the hook still dispatches for dashboards and that resources without it pass state through untouched. Co-authored-by: Isaac
janniklasrose
pushed a commit
to GrantIsEaton/cli
that referenced
this pull request
Aug 5, 2026
## Changes Add `EnvMatrix.READPLAN = ["", "1"]` to the dashboard acceptance tests that deploy, and route their `bundle deploy` calls through the `readplanarg` helper so each test runs twice: once with an in-memory plan, once applying a plan saved to a file with `--plan`. Because all `EnvMatrix` variants must produce identical output files, any divergence between the two deploy paths now fails the test automatically. Tests converted: | Test | What it exercises | | --- | --- | | `resources/dashboards/change-serialized-dashboard` | edits `serialized_dashboard` → update; asserts the PATCH/POST request bodies | | `resources/dashboards/nested-folders` | reads `serialized_dashboard` back from the API after deploy | | `resources/dashboards/publish-failure-cleans-up-dashboard` | publish failure path and the cleanup DELETE | | `deployment/bind/dashboard` (+ `recreation/`) | bind writes state without going through `DoCreate`/`DoUpdate` | ## Why `bundle deploy --plan` takes a different code path than a plain deploy: everything applied comes from the serialized plan file rather than from a plan computed in memory. Only one dashboard test exercised that path, so a regression in what a saved plan carries for dashboards would have gone uncaught. This lands the coverage on its own, ahead of the `serialized_dashboard` content-hash change (databricks#6105), so that any later failure is unambiguously attributable to that change rather than to a pre-existing gap. ## Result No request or state golden changed. The only golden edits are removed `>>> [CLI] bundle deploy` trace lines — the deploys are no longer traced because the command line itself differs between variants (`--plan` vs none). That the shared goldens are byte-identical across both variants is the point: `out.patch.requests.direct.txt`, `out.post.requests.txt` and `out.state_after_bind.*.json` confirm both paths issue the same API requests and persist the same state on `main` today. ## Not converted `migrate/dashboards` is deliberately left alone. It asserts on GET requests to `//dashboards` to confirm the direct engine ran, but a saved-plan deploy skips the read phase, so no GETs are recorded and that assertion has nothing to match. Converting it would mean weakening a real assertion. Migration therefore remains uncovered for the saved-plan path — worth noting, since like bind it writes state without going through `DoCreate`/`DoUpdate`. ## Tests Acceptance-only; no production code changes. Verified green on unmodified `origin/main` before converting, so failures were attributable. `task fmt`, `task ws` and `task lint-q` are clean. This pull request and its description were written by Isaac.
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
4 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
Stop persisting a dashboard's
serialized_dashboardin the direct engine's state. State nowrecords a new
serialized_dashboard_hash— asha256_hashed_in_state:<hex>digest of thecontents, computed in
PrepareState— andDoCreate/DoUpdatedrop the contents after theAPI call, so the full payload still reaches the backend on every create and update.
The digest lives on
DashboardState, notDashboardConfig, so it stays out of the bundleschema. In
resources.yml,serialized_dashboardgainsignore_local_changes(state holdsno contents, so saved-vs-config always differs) alongside its existing
ignore_remote_changes; the digest getsignore_remote_changesonly, since the API neverreturns it. The digest is deliberately not
ignore_local_changes— a saved-vs-configdifference on it is the signal that the contents changed.
bundle bindandbundle migratewrite state without going throughDoCreate/DoUpdate,and state written by an earlier CLI holds the contents with no digest. A new optional
PrepareStateByHashingadapter hook covers both, deriving the digest from contents alreadyin state. No state version bump.
Why
serialized_dashboardholds inlined dashboard JSON, frequently multi-MB, and state onlyever compares it for equality — nothing reads a value back out of the stored copy. A short
digest answers the same question, so
resources.jsonno longer carries a copy of everydashboard on every deploy. Existing bundles do not report a dashboard update on upgrade, and
bundle planstill shows the real content diff.The digest is taken over the rendered string sent to the API, not the raw
any-typed configvalue, so a dashboard inlined as YAML and one loaded from a file digest identically —
otherwise the digest stored on create would never match the one computed from config, giving
inlined dashboards permanent phantom drift.
Tests
resources/dashboard-state-hash(renamed fromdashboard-state-sha) asserts state holdsonly the digest while the API receives the full contents, across create, a no-op re-plan, an
edit and the resulting update, for
READPLANin["", "1"].Unit tests cover digest equality across the string and map forms,
PrepareStateByHashing(including the legacy-state upgrade path), and the three
resources.ymldeclarations.Dashboard, bind and migrate goldens regenerated; the digest is masked as
[HASH]by a singlerule in
acceptance/bundle/test.toml, anchored on the prefix.Waiting on #6162, which runs the remaining dashboard tests under the
READPLANvariant, toland first so the saved-plan path is covered here too.
This pull request and its description were written by Isaac.