direct: support references to/from grants#4774
Conversation
Apply the same EmbeddedSlice pattern used for permissions to grants: - Rename GrantsState.Grants to GrantsState.EmbeddedSlice (json:"_") - Add migrateV2ToV3 to rename "grants" → "_" key in existing state - Bump state version to 3 - Add grant_ref acceptance test skeleton Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Regenerate grants plan outputs (JSON key "grants" → "_") - Add grant_ref test output with reference resolution - Add schema_with_grants and schema_grant_ref invariant configs - Update refschema to show grants[*] embedded slice fields - Update state version in migration/future_version tests - Add exclusions for new grant configs in migrate/continue_293 tests Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Version 2 should encompass both permissions and grants migrations. The v2→v3 migration function is still present but unreferenced. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The EmbeddedSlice convention is detected by Go field name, not JSON tag. Keeping "grants" as the JSON tag avoids needing a state migration entirely, since existing v2 state files already use the "grants" key. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Use "grants" instead of "__embed__" / "_" as the JSON key for grants EmbeddedSlice, and revert state version from 3 to 2 in test outputs. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…l change Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Match what permissions does: compare grant entries by principal name instead of by slice index, enabling more accurate change detection. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Remove grantKey and KeyedSlices() from ResourceGrants (out of scope) - Add migrateV2ToV3 that renames "grants" → "__embed__" in grant state entries - Bump currentStateVersion to 3 - Use slices.Sort instead of sort.Slice for privileges Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…grants key Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Grants and permissions migrations are now both part of the v1→v2 migration step. This keeps currentStateVersion at 2. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Use structpath.ParsePath + StringKey to identify migration targets instead of strings.HasSuffix - Replace json.RawMessage in oldGrantsStateV1 with []catalog.PrivilegeAssignment and use explicit copy, matching the pattern in migratePermissionsEntry Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
State version < 2 cannot contain __embed__ format, so the "might already be migrated" guards are unnecessary. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
The result is parsed again later, indentation is unnecessary. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
1af04cd to
ed528f7
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
Review (automated, 2 agents)
Verdict: Approved
0 Critical | 0 Major | 0 Gap | 2 Nit | 1 Suggestion
Clean, well-executed PR. Correctly follows the established EmbeddedSlice pattern from PR #4703. Migration is properly folded into v1->v2. The switch from strings.HasSuffix to structpath.ParsePath for migration key-matching is a correctness improvement. Test coverage is thorough.
See inline comment for the one suggestion.
bundle/direct/dstate/migrate.go
Outdated
| EmbeddedSlice: make([]catalog.PrivilegeAssignment, len(old.Grants)), | ||
| } | ||
| copy(newState.EmbeddedSlice, old.Grants) |
There was a problem hiding this comment.
[Suggestion] Simplify: direct assignment instead of make + copy
Unlike the permissions migration which transforms field names (permission_level -> level), the grants migration keeps the same inner type (catalog.PrivilegeAssignment). The make + copy is unnecessary. Since old goes out of scope immediately, a direct assignment is simpler and equivalent:
newState := dresources.GrantsState{
SecurableType: old.SecurableType,
FullName: old.FullName,
EmbeddedSlice: old.Grants,
}Found by: Both reviewers independently
old.Grants is a fresh local allocation from json.Unmarshal, no aliasing risk. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
| SecurableType string `json:"securable_type"` | ||
| FullName string `json:"full_name"` | ||
| Grants []catalog.PrivilegeAssignment `json:"grants,omitempty"` | ||
| EmbeddedSlice []catalog.PrivilegeAssignment `json:"__embed__,omitempty"` |
There was a problem hiding this comment.
Can you copy the comment about the significance of __embed__ here as well?
Any future such type must use the same, so the more comments the better.
Can be in a different PR.
There was a problem hiding this comment.
Sure, although I hope we actually limit this pattern to grants / permissions.
|
Commit: ed6f73f
41 interesting tests: 15 RECOVERED, 11 KNOWN, 9 flaky, 5 FAIL, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
## 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))
## Why The CLI's CODEOWNERS catch-all assigns 6 people to every PR outside a few narrow paths. This creates review noise and diffuses responsibility. We want targeted reviewer suggestions based on who actually worked on the changed code recently. ## Changes Before: Every PR touching core code auto-assigns all 6 CODEOWNERS. No signal about who is best suited to review. Now: A new GitHub Action analyzes git history of the changed files and posts a PR comment with two sections: - **Suggested reviewers** (1-3 people best suited based on recency-weighted git history) - **Eligible reviewers** (everyone from CODEOWNERS who could review, minus the suggested ones) This is additive only. CODEOWNERS and auto-assign stay unchanged. How it works: - Triggers on PR open, synchronize, and ready-for-review (skips drafts and fork PRs) - Classifies changed files by type (source=1.0, tests=0.3, acceptance=0.2, generated=0.0) - Scores contributors using recency-weighted commit history (half-life 150 days) - Resolves git author names to GitHub logins via the GitHub API (no hardcoded alias table to maintain) - Parses `.github/CODEOWNERS` to find eligible reviewers for the changed paths - Updates the comment in-place on re-runs (no notification churn) Implementation: a single Python script (`tools/suggest_reviewers.py`, 281 lines) and a minimal workflow YAML. ## Test plan - [x] Action ran on this PR itself and posted a comment successfully - [x] Verified script parses cleanly, passed `make checks`, passed `ruff format` - [x] Dry-run tested against 4 recent merged PRs with different characteristics: **PR #4784** (66 files, by pietern, big DABs rename): ``` ## Suggested reviewers - @denik -- recent work in `./`, `bundle/`, `cmd/bundle/generate/` Confidence: high ## Eligible reviewers @andrewnester, @anton-107, @lennartkats-db, @shreyas-goenka, @simonfaltum ``` Correctly identifies Denis as the clear top reviewer (2x second place score). All 6 CODEOWNERS shown as eligible. **PR #4782** (13 files, by denik, bundle engine priority): ``` ## Suggested reviewers - @andrewnester -- recent work in `bundle/schema/`, `bundle/internal/schema/`, `cmd/bundle/generate/` - @pietern -- recent work in `bundle/schema/`, `cmd/bundle/generate/`, `bundle/internal/schema/` - @shreyas-goenka -- recent work in `bundle/schema/`, `bundle/internal/schema/`, `cmd/bundle/` Confidence: medium ## Eligible reviewers @anton-107, @simonfaltum ``` Suggests 3 reviewers when scores are close. Remaining CODEOWNERS shown as eligible. **PR #4785** (2 files, by MarioCadenas, apps bug fix): ``` ## Suggested reviewers - @arsenyinfo -- recent work in `cmd/apps/` - @pietern -- recent work in `cmd/apps/` - @pkosiec -- recent work in `cmd/apps/` Confidence: low ## Eligible reviewers @databricks/eng-apps-devex ``` Correctly suggests apps-area contributors (not the catch-all CODEOWNERS). Shows the apps team as eligible. Low confidence since only 2 files. **PR #4774** (28 files, by denik, direct engine grants): ``` ## Suggested reviewers - @andrewnester -- recent work in `bundle/direct/dresources/`, `acceptance/bundle/invariant/` - @shreyas-goenka -- recent work in `bundle/direct/dresources/` Confidence: high ## Eligible reviewers @anton-107, @pietern, @simonfaltum ``` Correctly identifies the two main bundle/direct contributors. High confidence with clear score separation.
Follow up to #4703 but for grants.
Allows references to/from grant objects and removes grants.grants in the path.