Uh oh!
There was an error while loading. Please reload this page.
direct: grants: revoke removed principals + pass PlanEntry to DoUpdate - #4824
Merged
Conversation
Collaborator
Commit: b1224ce
17 interesting tests: 10 SKIP, 7 KNOWN
Top 20 slowest tests (at least 2 minutes):
|
denikforce-pushed
the
denik/cursor-agent-test
branch
from
March 24, 2026 12:00
0f1452f to
302af46Comparedenik
marked this pull request as ready for review
March 24, 2026 13:47
denik
requested review from
andrewnester, anton-107, pietern, shreyas-goenka and simonfaltum
as code ownersMarch 24, 2026 13:47
andrewnester
approved these changes
Mar 24, 2026
Uh oh!
There was an error while loading. Please reload this page.
| type ( | ||
| Changes = deployplan.Changes | ||
| ChangeDesc = deployplan.ChangeDesc | ||
| PlanEntry = deployplan.PlanEntry |
Contributor
There was a problem hiding this comment.
Should have the PlanEntry change been a separate PR? Or is it necessary for permissions change?
ContributorAuthor
There was a problem hiding this comment.
It's needed by grants change. Originally I tried to parse Changes which is possible but makes everything more complex compared to parsing state directly.
| schema.UpdatedAt = schema.CreatedAt | ||
| schema.CreatedBy = s.CurrentUser().UserName | ||
| schema.UpdatedBy = s.CurrentUser().UserName | ||
| schema.EffectivePredictiveOptimizationFlag = &catalog.EffectivePredictiveOptimizationFlag{ |
denikforce-pushed
the
denik/cursor-agent-test
branch
2 times, most recently
from
March 26, 2026 11:54
ea27988 to
dc135abComparedenikforce-pushed
the
denik/cursor-agent-test
branch
from
March 26, 2026 14:00
dc135ab to
9347f54Comparedenik
enabled auto-merge
March 26, 2026 14:00
Cover the case where removing a schema grant principal should stay clean under Terraform but still reproduces the stale-principal drift in the direct engine. This gives us a local and cloud repro to validate the eventual fix.
Inline the principals and use a removable comment marker plus gron-based assertions so the acceptance script is easier to follow while preserving the Terraform-pass/direct-fail repro.
Record the second-plan output for the removed-principal regression test so Terraform convergence and direct-engine drift are asserted separately.
Read current grants during direct updates so principals removed from config are explicitly revoked. Normalize the removed-principal regression output so the acceptance test stays stable while proving convergence.
Let acceptance scripts ignore array ordering without extra shell plumbing. Use the new flag in the removed-principal grants regression to keep the output stable and simpler to read.
Keep the removed-principal grants regression easy to scan by inlining the gron noindex pipeline instead of routing through a shell helper.
Build direct grants updates from the planned keyed diffs so removed principals can be revoked without rereading current grants. This keeps the reconciliation path smaller while preserving the existing privilege update semantics.
Keep the planner-driven revoke fix but drop the extra helper extraction and constant moves so the file stays closer to its original shape.
Keep the grants fix focused by inlining tiny one-use helpers instead of carrying extra wrappers. This reduces review noise without changing the revoke behavior.
Capture the plan after a previously unseen principal is added outside the bundle so both engines record the resulting grants drift. Keep the regression stable on local testserver and cloud by normalizing the saved plan output.
Keep the out-of-band principal regression easy to read by storing the grants update request as a fixture instead of generating it inline in the script.
Record a second post-reconcile plan for the out-of-band principal grants regression so both engines show whether they converged after deploy. Align the fake schema metadata with cloud responses so the direct plan goldens stay stable without local-only filtering.
Skip unparseable grant change paths during reconciliation so an unexpected planner path does not fail an otherwise valid deploy, and reuse the update path for create. Align the schema unit expectation with the current fake schema payload so the direct resource suite stays green.
Record the clean remove_principal plans in shared test output and add a second deploy/plan pass so the grants revocation test proves the bundle stays reconciled after the first update.
Simplify grants update logic by giving DoUpdate access to the full PlanEntry, which includes RemoteState. This replaces the previous approach of parsing structured diff paths to discover removed principals with a direct comparison against remote state. Co-authored-by: Isaac
The privileges slice is freshly deserialized from JSON with no shared backing array, so the defensive copy is unnecessary. Co-authored-by: Isaac
The testserver now returns additional schema fields (metastore_id, schema_id, enable_predictive_optimization) which updates golden files in affected tests. Co-authored-by: Isaac
denikforce-pushed
the
denik/cursor-agent-test
branch
from
March 26, 2026 14:39
9347f54 to
b1224ceComparedenik
disabled auto-merge
March 26, 2026 14:49
Uh oh!
There was an error while loading. Please reload this page.
eng-dev-ecosystem-bot
commented
Mar 26, 2026
Collaborator
Commit: 7498df4
167 interesting tests: 115 MISS, 17 RECOVERED, 15 FAIL, 11 KNOWN, 4 flaky, 3 PANIC, 2 SKIP
Top 50 slowest tests (at least 2 minutes):
|
deco-sdk-taggingBot
added a commit
that referenced
this pull request
Apr 8, 2026
## Release v0.296.0 ### Notable Changes * Direct deployment engine for DABs is now in Public Preview. Documentation at [docs/direct.md](docs/direct.md). ### CLI * Auth commands now error when --profile and --host conflict ([#4841](#4841)) * Add `--force-refresh` flag to `databricks auth token` to force a token refresh even when the cached token is still valid ([#4767](#4767)) ### Bundles * Deduplicate grant entries with duplicate principals or privileges during initialization ([#4801](#4801)) * Fix `bundle deployment bind` to always pull remote state before modifying ([#4892](#4892)) * engine/direct: Fix drift in grants resource due to privilege reordering ([#4794](#4794)) * engine/direct: Fix 400 error when deploying grants with ALL_PRIVILEGES ([#4801](#4801)) * engine/direct: Fix unwanted recreation of secret scopes when scope_backend_type is not set ([#4834](#4834)) * engine/direct: Fix bind and unbind for non-Terraform resources ([#4850](#4850)) * engine/direct: Fix deploying removed principals ([#4824](#4824)) * engine/direct: Fix secret scope permissions migration from Terraform to Direct engine ([#4866](#4866))
denik added a commit
that referenced
this pull request
May 20, 2026
#4824) ## Changes - When a principal is removed from grants config, deploy now revokes their privileges instead of silently leaving them in place. This matches terraform behaviour. - `DoUpdate` interface changed to receive `*PlanEntry` instead of `Changes`, giving resources access to remote state directly. ## Why New behaviour matches terraform's. Grants API is incremental, it only replaces principals mentioned in the payload and leaves other one alone (instead of wiping them, which is the incorrect assumption previous impl was built upon). Passing PlanEntry instead of just Changes simplifies grants update logic instead of parsing Changes paths like (`[principal='bob'].privileges[0]`), it compares desired state against `PlanEntry.RemoteState`. A few other resources had a need to view remote state as well, so it's generally useful. ## Tests - New acceptance tests cover the remove-principal and out-of-band-principal drift scenarios. - gron.py got new option --noindex which prints array elements as `element[]` instead of `element[1]` which allows to ignore order differences. - Updates to testserver for grants & schemas to match AWS backend better to enable local+cloud test.
denik pushed a commit
that referenced
this pull request
May 20, 2026
## Release v0.296.0 ### Notable Changes * Direct deployment engine for DABs is now in Public Preview. Documentation at [docs/direct.md](docs/direct.md). ### CLI * Auth commands now error when --profile and --host conflict ([#4841](#4841)) * Add `--force-refresh` flag to `databricks auth token` to force a token refresh even when the cached token is still valid ([#4767](#4767)) ### Bundles * Deduplicate grant entries with duplicate principals or privileges during initialization ([#4801](#4801)) * Fix `bundle deployment bind` to always pull remote state before modifying ([#4892](#4892)) * engine/direct: Fix drift in grants resource due to privilege reordering ([#4794](#4794)) * engine/direct: Fix 400 error when deploying grants with ALL_PRIVILEGES ([#4801](#4801)) * engine/direct: Fix unwanted recreation of secret scopes when scope_backend_type is not set ([#4834](#4834)) * engine/direct: Fix bind and unbind for non-Terraform resources ([#4850](#4850)) * engine/direct: Fix deploying removed principals ([#4824](#4824)) * engine/direct: Fix secret scope permissions migration from Terraform to Direct engine ([#4866](#4866))
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 freeto 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
DoUpdateinterface changed to receive*PlanEntryinstead ofChanges, giving resources access to remote state directly.Why
New behaviour matches terraform's. Grants API is incremental, it only replaces principals mentioned in the payload and leaves other one alone (instead of wiping them, which is the incorrect assumption previous impl was built upon).
Passing PlanEntry instead of just Changes simplifies grants update logic instead of parsing Changes paths like (
[principal='bob'].privileges[0]), it compares desired state againstPlanEntry.RemoteState. A few other resources had a need to view remote state as well, so it's generally useful.Tests
element[]instead ofelement[1]which allows to ignore order differences.