Add per-resource SkipEntitlementsAndGrants for Sparse ACLs#119
Add per-resource SkipEntitlementsAndGrants for Sparse ACLs#119c1-dev-bot[bot] wants to merge 2 commits into
Conversation
Add a `skip_entitlements_and_grants` CEL expression field to ResourceMapping config. When the expression evaluates to true for a given resource row, the resource is annotated with SkipEntitlementsAndGrants, causing the SDK syncer to skip entitlement and grant processing for that individual resource. This enables Sparse ACLs: previously, skip was only supported at the resource-type level (all-or-nothing). Now it can be controlled per-resource based on row data, so only resources that actually have ACLs get entitlement/grant processing. Fixes: CXH-1391
|
Claude finished @c1-dev-bot[bot]'s task in 1m 20s —— View job PR Review: Add per-resource SkipEntitlementsAndGrants for Sparse ACLs
OverviewThis PR adds a Verdict: Looks good ✓The implementation is straightforward, correct, and follows existing patterns well. A few observations below, but nothing blocking. What works well
Observations1. No integration test for CEL evaluation at the resource levelThe tests validate that the config field parses correctly and that resource-type annotations are applied, but there's no test that exercises the actual CEL evaluation path in The PR description acknowledges this ("Manual testing with a YAML config..."). This is a reasonable approach given that the CEL evaluation machinery is already well-tested elsewhere, but it's worth noting as a gap. 2. Error handling on CEL evaluation failure returns a bare errorAt skip, err := s.env.EvaluateBool(ctx, mapping.SkipEntitlementsAndGrants, inputs)
if err != nil {
return err
}This is consistent with how other CEL evaluations in 3. No config validation for the CEL expressionThere's no Worth considering for a follow-up: a SummaryThe implementation is clean, minimal, follows established patterns, and the two levels of skip (resource-type vs per-resource) compose correctly via the SDK. The test coverage is adequate for parsing/config, though an integration test for the CEL evaluation path would strengthen confidence. No blocking issues. |
|
Checking lint failure details... |
|
Investigating |
|
Claude finished @c1-dev-bot[bot]'s task in 1m 44s —— View job
PR Review: Add per-resource SkipEntitlementsAndGrants for Sparse ACLsVerdict: Approve — Clean, minimal implementation that follows established patterns. One testing gap worth noting, no blocking issues. What works well
Observations1. Missing integration test for the CEL evaluation path (non-blocking)The tests validate config parsing and resource-type-level annotations, but there's no test that exercises the actual code path in This is the most important code path in the PR. The PR description acknowledges this as manual testing, which is reasonable since the CEL evaluation machinery is well-tested elsewhere and the pattern is identical to 2. Error handling is correct (confirming)At 3. Struct tag consistency is fineThe new CI Status
SummaryThe implementation is clean, minimal, and correct. The two levels of skip (resource-type vs per-resource) compose correctly through the SDK's annotation system. The only meaningful gap is the lack of an integration test for the CEL evaluation path at the resource level — everything else follows established patterns precisely. |
Summary
skip_entitlements_and_grantsCEL expression field toResourceMappingconfig, enabling per-resource Sparse ACL supportSkipEntitlementsAndGrantsannotation is applied to that individual resource, causing the SDK syncer to skip entitlement and grant processingExample Usage
In the example above, databases where
is_publicis true will skip entitlement and grant processing, while private databases will have their ACLs synced normally.Test plan
TestParse_SparseACLs) passes - validates the new field is correctly parsed from YAMLTestConfig_GetResourceTypes_SkipEntitlementsAndGrants) passes - validates resource-type-level skip still worksskip_entitlements_and_grantsCEL expression at the resource mapping levelFixes: CXH-1391
Automated PR Notice
This PR was automatically created by c1-dev-bot as a potential implementation.
This code requires: