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: 67b41b9
16 interesting tests: 7 SKIP, 7 RECOVERED, 2 flaky
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
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.
| // findEmbedField returns the value of the EmbeddedSlice field in struct v, if any. | ||
| // Returns an invalid reflect.Value if no EmbeddedSlice field exists. | ||
| func findEmbedField(v reflect.Value) reflect.Value { | ||
| if v.Kind() != reflect.Struct { |
Member
There was a problem hiding this comment.
[Agent Swarm Review] [Gap (Nit)]
findEmbedField does not validate the field is actually a slice. If someone names a non-slice field EmbeddedSlice, struct walkers would produce confusing errors.
Suggestion: Add a type check: if sf.Type.Kind() != reflect.Slice { panic(...) }
denik
force-pushed
the
denik/permissions-reference
branch
from
7824eb6 to
2b2b440
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduces the __EMBED__ JSON tag convention. When a struct field has json:"__EMBED__", struct walkers treat it as transparent and don't add its name to the path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fields with json:"__EMBED__" are walked at the parent path level, making their contents appear directly without the field name in paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fields with json:"__EMBED__" are walked at the parent path level in type traversal, consistent with Walk behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When navigating a struct and the path expects an index or key-value selector, transparently navigate through the __EMBED__ field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When setting a value at an index node and the parent is a struct, transparently navigate through the __EMBED__ field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When validating paths with index, bracket-star, or key-value nodes on a struct type, transparently navigate through __EMBED__ fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fields with json:"__EMBED__" are diffed at the parent path level, consistent with how anonymous embedding works. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This makes the permissions slice appear at the root path of PermissionsState, matching the input config schema where permissions are accessed as resources.jobs.foo.permissions[0]. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The __EMBED__ convention on PermissionsState fixes the structaccess.Set() bug that prevented the direct engine from planning permissions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
denik
force-pushed
the
denik/permissions-reference
branch
from
29d6d69 to
d0a4d17
Compare
…l duplicates Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a new invariant config testing cross-resource permission references (both level and group_name). Also fixes no_drift and migrate scripts to redirect stderr separately so warnings don't corrupt the JSON plan file passed to verify_no_drift.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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>
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
3 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])permissions.[*]→permissions[*](remove spurious dot before bracket)job_permissionsacceptance test for direct engineWhy
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.