direct: Fix permissions state path to match input config schema#4703
Merged
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
…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
Collaborator
|
Commit: 9767900
55 interesting tests: 16 RECOVERED, 14 flaky, 13 FAIL, 11 KNOWN, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 18, 2026
Follow up to #4703 but for grants. Allows references to/from grant objects and removes grants.grants in the path. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
deco-sdk-tagging bot
added a commit
that referenced
this pull request
Mar 18, 2026
## Release v0.295.0 ### Notable Changes * Databricks Asset Bundles have been renamed to Declarative Automation Bundles (DABs). This is a non-breaking change; no code or configuration modifications are required. See the [FAQ](https://docs.databricks.com/aws/en/dev-tools/bundles/faqs#why-was-databricks-asset-bundles-renamed-to-declarative-automation-bundles). * Add `bundle.engine` config setting to select the deployment engine (`terraform` or [`direct`](https://docs.databricks.com/aws/en/dev-tools/bundles/direct)). The `bundle.engine` setting takes precedence over the `DATABRICKS_BUNDLE_ENGINE` environment variable. When the configured engine doesn't match existing deployment state, a warning is issued and the existing engine is used ([#4749](#4749), [#4782](#4782)) ### CLI * Add `databricks auth switch` command for setting the default profile ([#4651](#4651)) * Add positional argument support to `auth logout` ([#4744](#4744)) * Strip trailing slash from host in `auth login`, `auth token`, and `configure` commands ([#4633](#4633)) ### Bundles * Standardize `personal_schemas` enum across bundle templates ([#4401](#4401)) * engine/direct: Fix permanent drift on experiment name field ([#4627](#4627)) * engine/direct: Fix permissions state path to match input config schema ([#4703](#4703)) * Add default project name and success message to default-scala template ([#4661](#4661)) * Skip enum validation for unresolved variable references ([#4752](#4752)) * engine/direct: Support references to/from grants ([#4774](#4774))
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