direct: Fix permissions state path to match input config schema#4703
Open
direct: Fix permissions state path to match input config schema#4703
Conversation
denik
requested review from
andrewnester,
anton-107,
pietern,
shreyas-goenka and
simonfaltum
as code owners
Collaborator
|
Commit: 79d09ab
18 interesting tests: 9 SKIP, 7 KNOWN, 1 flaky, 1 RECOVERED
Top 20 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/permissions-reference
branch
from
66d736a to
cf0178e
Compare
denik
force-pushed
the
denik/permissions-reference
branch
from
849df2a to
7824eb6
Compare
simonfaltum
previously requested changes
Mar 12, 2026
Member
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
[Agent Swarm Review] Verdict: Not ready yet
- 1 Critical
- 2 Major
- 2 Gap
- 2 Nit
- 2 Suggestion
The core idea (EmbeddedSlice convention for struct walkers) is sound and well-implemented across libs/structs/. However, there is a critical backward-compatibility issue: the JSON tag json:"_,omitempty" on PermissionsState.EmbeddedSlice means old state files using the "permissions" key will silently load as empty, erasing all permission state. The fix is straightforward: change to json:"permissions,omitempty" since the EmbeddedSlice convention is driven by Go field name, not JSON tag.
See inline comments for details.
| return err | ||
| } | ||
| *p = PermissionsState(raw) | ||
| migratePermissionLevel(p.EmbeddedSlice) |
Member
There was a problem hiding this comment.
[Agent Swarm Review] [Critical]
State backward compatibility: JSON key change from "permissions" to "_" breaks old state files.
EmbeddedSlice uses json:"_,omitempty", so old state files containing "permissions": [...] will silently load as empty. The custom UnmarshalJSON only migrates permission_level to level within elements, but does NOT handle the outer key name change. This erases all permission state on the first plan/deploy after upgrading.
Both reviewers independently found this issue and confirmed it in cross-review.
Suggestion: Change to json:"permissions,omitempty". The EmbeddedSlice convention is driven entirely by the Go field name, not the JSON tag. This preserves backward compat, produces readable JSON output, and eliminates the need for key-name migration.
Contributor
Author
There was a problem hiding this comment.
I do want to rename permissions to _. The breakage will indeed cause a drift for all permissions resources. The drift will be removed on next "bundle deploy". However, given that direct engine is well adopted, I think it makes sense to migrate the state properly.
I bumped the state version to 2 and added migration function that takes old struct and produces a new one.
denik
force-pushed
the
denik/permissions-reference
branch
from
7824eb6 to
2b2b440
Compare
denik
force-pushed
the
denik/permissions-reference
branch
from
29d6d69 to
d0a4d17
Compare
Cross-resource permission references don't work in terraform mode:
databricks_job doesn't expose permissions as output attributes, so
${databricks_job.job_b.permissions[0].level} can't be resolved by Terraform.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…comment Tests both directions of cross-resource references: - permission fields referencing job tag values (job tag -> permission group_name) - permission fields referencing another job's permissions (permission -> permission) Also expands the comment explaining why permission references don't work in terraform mode (interpolator converts paths but databricks_job has no permissions output attributes). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds infrastructure to verify the current CLI can deploy on top of state produced by an older version: - continuity/prepare.py: run with a version (e.g. v0.293.0) to deploy each invariant config using that CLI version and save the resulting state files to continuity/<version>/<config>.state.json. Uses -forcerun to bypass the Local=false guard so it only runs when explicitly invoked. - continuity/prepare/: acceptance test that does the deploy+save; excluded from normal CI via Local=false/Cloud=false. - continuity/v0.293.0/: continuity test that loads the committed state file, deploys with the current CLI, then verifies no drift. - 24 state files generated from v0.293.0 (released 2026-03-11). Resources with name-based IDs unrecognised by the mock server are excluded from the test (dashboard, database_catalog, secret_scope, synced_database_table). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…CLI at test time Instead of pre-generating state files from v0.293.0, the test runner now always downloads and caches v0.293.0 and exposes it as $CLI_293. The continue_293 test deploys using $CLI_293 against the mock server, then deploys with the current CLI, verifying no drift. This ensures the state is always consistent with the mock server. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Group names (viewers, admins) in job_permission_ref and job_cross_resource_ref configs don't exist in real workspaces, causing 404 errors on cloud. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace 'viewers' (non-existent in real workspaces) with 'users' (built-in Databricks group). This allows the job_permission_ref and job_cross_resource_ref configs to run on cloud without requiring custom group setup. Remove cloud exclusion from no_drift test that was added to work around the non-existent group names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ature not in v0.293.0 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nuity The new config deploys a job with a permission entry (group 'users' with CAN_VIEW) and is included in the continue_293 test to verify that the current CLI can deploy on top of state produced by v0.293.0 without drift. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… variable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Running an older CLI on state produced by a newer CLI is unsupported; require an upgrade rather than silently treating it as up-to-date. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…te/permission_level_migration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
These are separate sub-resource adapters with their own entries; walking them from the parent produced duplicate paths with different types. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduce embedFieldIndex to cache the field index per reflect.Type, so both findEmbedField and findEmbedFieldType share a single sync.Map lookup and field access becomes an O(1) v.Field(idx) call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
denik
force-pushed
the
denik/permissions-reference
branch
from
334d8b3 to
79d09ab
Compare
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
5 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
EmbeddedSlicefield name convention to struct walkers inlibs/structs/— when a struct field is namedEmbeddedSlice, walkers treat it as transparent (no path segment added), so its elements appear directly at the parent pathPermissionsState: renamePermissionsfield toEmbeddedSlice, making state paths likeresources.jobs.foo.permissions[0]match input config paths (previouslyresources.jobs.foo.permissions.permissions[0])Why
The direct deployment engine's permissions state used a wrapper struct that added an extra
permissionssegment to paths. This caused a mismatch with input config paths, preventing dependency tracking between permissions and their parent resources. With this fix, state and config paths are consistent.Tests