direct: Add support for secret scopes #3886
Merged
Conversation
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
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 7e1c917
17 interesting tests: 11 flaky, 4 RECOVERED, 2 SKIP
Top 22 slowest tests (at least 2 minutes):
|
shreyas-goenka
changed the title
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
commented
Nov 10, 2025
Contributor
Author
There was a problem hiding this comment.
Newly added tests give more than enough coverage for this. (see basic test)
shreyas-goenka
commented
Nov 10, 2025
| Local = true | ||
| Cloud = false | ||
| RecordRequests = true | ||
| Local = false |
Contributor
Author
There was a problem hiding this comment.
We only run it on cloud because there's ordering issues in the permissions returned by GET that are not worth dealing with (because of the different user names, there is no stable sort).
Contributor
There was a problem hiding this comment.
Ordering issues can be addressed by recording requests in env-specific file:
> out.requests.$DATABRICKS_BUNDLE_ENGINE.json
Contributor
Author
There was a problem hiding this comment.
We would need to split them into local vs. cloud. The ordering issue here is the permissions being in a different order even after sorting.
It's doable but I'd rather skip it since the cloud coverage is the more important bit to ensure.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Dec 2, 2025
| } | ||
|
|
||
| // Sort ACLs by principal for deterministic ordering | ||
| slices.SortFunc(acls, func(a, b workspace.AclItem) int { |
Contributor
| // setACLs reconciles the desired ACLs with the current state | ||
| func (r *ResourceSecretScopeAcls) setACLs(ctx context.Context, scopeName string, desiredAcls []workspace.AclItem) (string, error) { | ||
| // Get current ACLs | ||
| currentAcls, err := r.client.Secrets.ListAclsAll(ctx, workspace.ListAclsRequest{ |
Contributor
There was a problem hiding this comment.
We should not need to read ACLs from the backend, we've already done it. This is encoded in PlanEntry which we now pass to DoUpdate()
Contributor
Author
There was a problem hiding this comment.
I don't see planentry in DoUpdate. Please clarify.
Contributor
There was a problem hiding this comment.
bundle/direct/dresources/adapter.go: DoUpdate(ctx context.Context, id string, newState any, changes *deployplan.Changes) (remoteState any, e error)
we pass entry.Changes here, but we can also pass entry:
bundle/direct/bundle_apply.go: err = d.Deploy(ctx, &b.StateDB, entry.NewState.Value, at, entry.Changes)
Contributor
Author
There was a problem hiding this comment.
Good point. This can be a followup. Let's not block this PR on this.
Contributor
There was a problem hiding this comment.
Agree, could be a follow up. a comment TODO would be nice to have here.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Dec 3, 2025
andrewnester
approved these changes
Dec 4, 2025
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
approved these changes
Dec 4, 2025
| // setACLs reconciles the desired ACLs with the current state | ||
| func (r *ResourceSecretScopeAcls) setACLs(ctx context.Context, scopeName string, desiredAcls []workspace.AclItem) (string, error) { | ||
| // Get current ACLs | ||
| currentAcls, err := r.client.Secrets.ListAclsAll(ctx, workspace.ListAclsRequest{ |
Contributor
There was a problem hiding this comment.
Agree, could be a follow up. a comment TODO would be nice to have here.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: a4ab082
16 interesting tests: 8 flaky, 7 RECOVERED, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Adds direct deployment support for secret scopes.
Mock Server Fix
Fixed
libs/testserver/secret_scopes.goto automatically grant MANAGE permission to the creator when a scope is created, matching real Databricks behavior.Tests
New local and cloud acceptance tests.