Emit permission overlap warnings instead of discarding them#5520
Merged
Conversation
When a bundle-level permission is skipped because the resource already defines a permission for the same principal, the explanatory warnings were computed but dropped (a TODO predating logdiag). Log them via logdiag so users can see why a grant was not applied. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
Integration test reportCommit: 6665244
25 interesting tests: 13 SKIP, 10 KNOWN, 2 flaky
Top 7 slowest tests (at least 2 minutes):
|
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
janniklasrose
and removed request for
andrewnester
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
approved these changes
Jun 30, 2026
janniklasrose
left a comment
janniklasrose
left a comment
Contributor
There was a problem hiding this comment.
since it's customer facing, consider a changelog entry (although this change is pretty minor)
| @@ -0,0 +1 @@ | |||
| $CLI bundle validate -o json | jq '.resources.jobs | map_values(.permissions)' | |||
Contributor
There was a problem hiding this comment.
nit: the actual .resources.jobs[].permissions isn't really necessary for this test, we just want to see the warning. maybe we can pipe stdout to /dev/null and only print stderr (assuming the warnings go there)
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.
Why
Found during a full-repo review of the CLI. When a bundle defines top-level
permissionsand a resource already has its own permission entry for the same user, group, or service principal, the bundle-level grant is skipped for that resource. The warnings explaining why the grant was skipped were computed but never shown (a TODO that predates logdiag), so users had no way to tell why a permission they configured did not get applied.Changes
Before, the overlap warnings built in
bundle/config/mutator/resourcemutator/apply_bundle_permissions.gowere discarded; now they are logged via logdiag and show up in command output, for example:The skip behavior itself is unchanged, only the warning is now visible. The stale TODO is replaced by the actual logging, and the existing
TestWarningOnOverlapPermissionunit test now asserts the warning is emitted.Test plan
TestWarningOnOverlapPermissionto assert exactly one warning with the expected summary is emittedacceptance/bundle/validate/permissions_overlapwith overlapping top-level and resource permissions, covering both the warning and the resulting permissions (both engines)go test ./bundle/config/mutator/resourcemutator ./bundle/testsbundle/validate,bundle/resources/permissions,bundle/run_as,bundle/migrate,bundle/apps/job_permissions) to confirm no existing outputs change./task fmt-q,./task lint-q,./task checksThis pull request and its description were written by Isaac.