feat(cli): SCS commit#3304
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Code splits and grows, New sets of rules now flow, Order starts to bloom. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the namespaced policy migration executor by modularizing the execution logic for Actions and Subject Condition Sets (SCS). It updates the Executor struct to cache target plans, renames error constants for better clarity, and introduces shared test helpers. Key feedback includes addressing potential runtime panics when accessing nested maps in the action and SCS caches, and ensuring that TargetStatusExistingStandard is correctly handled during SCS execution to maintain consistency with the action execution flow.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@otdfctl/migrations/namespacedpolicy/execute.go`:
- Around line 61-74: The Execute function is currently calling all phase helpers
unconditionally which lets planning-only loaded slices (e.g., registered
resources or obligation triggers loaded as reverse-dependency context) run;
change Execute to gate each phase dispatch by the plan scopes (or pass scopes
through) so only explicitly requested scopes run: call executeActions only if
plan.Scopes includes ScopeActions, call executeSubjectConditionSets only if
ScopeSubjectConditionSets is present, call executeSubjectMappings only if
ScopeSubjectMappings is present, and similarly guard executeRegisteredResources
and executeObligationTriggers (noting that requiresRegisteredResources() and
requiresObligationTriggers() may return true when ScopeActions is present for
planning—do not treat that as permission to execute those phases unless
plan.Scopes explicitly includes them). Ensure you reference the plan.Scopes set
(or thread scopes into per-phase APIs) and update the conditional checks around
executeActions, executeSubjectConditionSets, executeSubjectMappings,
executeRegisteredResources, and executeObligationTriggers accordingly.
In `@otdfctl/migrations/namespacedpolicy/subject_condition_sets_execute_test.go`:
- Around line 107-111: Add an assertion to this test to verify that when a
target is marked with TargetStatusAlreadyMigrated the executor cache is seeded:
after locating migratedTarget (plan.SubjectConditionSets[0].Targets[1]) and
confirming migratedTarget.Execution is nil, assert that
executor.cachedScsTargetID("scs-2", namespace1) equals the expected migrated
target ID (the same value used for the already-migrated target) so the test
covers the cache write performed for TargetStatusAlreadyMigrated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47093f49-69b4-454f-9839-7a145429ec32
📒 Files selected for processing (10)
otdfctl/migrations/namespacedpolicy/actions_execute.gootdfctl/migrations/namespacedpolicy/actions_execute_test.gootdfctl/migrations/namespacedpolicy/execute.gootdfctl/migrations/namespacedpolicy/execute_test_helpers_test.gootdfctl/migrations/namespacedpolicy/obligation_triggers_execute.gootdfctl/migrations/namespacedpolicy/plan.gootdfctl/migrations/namespacedpolicy/registered_resources_execute.gootdfctl/migrations/namespacedpolicy/subject_condition_sets_execute.gootdfctl/migrations/namespacedpolicy/subject_condition_sets_execute_test.gootdfctl/migrations/namespacedpolicy/subject_mappings_execute.go
2 participants
Proposed Changes
1.) Commit
subject-condition-sets2.) Refactor code placement to avoid large
execute.gofile size.Checklist
Testing Instructions
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests