Conversation
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 refactors the namespaced-policy migration plan to improve efficiency and clarity. By removing redundant data and simplifying the structure of the emitted plan JSON, the changes align the artifact more closely with the actual execution contract, resulting in a smaller and more maintainable plan format. 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. The plan was heavy, filled with bloat, We trimmed the sails to help the boat. Now leaner, cleaner, swift and light, The migration runs with all its might. Footnotes
|
📝 WalkthroughWalkthroughThis pull request refactors the namespaced-policy migration data model, replacing plural Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 migration planning and execution logic for namespaced policies by simplifying the Plan structure. Key changes include replacing Targets slices with a single Target for subject mappings, registered resources, and obligation triggers, and substituting complex object bindings with string IDs. The Unused and Unresolved sections were removed, and the ExecutorHandler interface now includes a GetRegisteredResource method. Feedback indicates that the validation logic for registered resources should be encapsulated within a Validate method on the Plan struct for better maintainability.
| for _, resource := range plan.RegisteredResources { // ? This should be a function withint the plan.go file | ||
| if resource != nil && resource.Unresolved != "" { | ||
| return fmt.Errorf("%w: finalized plan contains unresolved registered resources", ErrPlanNotExecutable) | ||
| } | ||
| } |
There was a problem hiding this comment.
As your comment on line 97 suggests, this validation logic would be better encapsulated as a method on the Plan struct. This improves maintainability by keeping the validation logic coupled with the data structure it validates.
Consider adding a Validate() method to the Plan struct in plan.go:
// In plan.go
func (p *Plan) Validate() error {
for _, resource := range p.RegisteredResources {
if resource != nil && resource.Unresolved != "" {
return fmt.Errorf("%w: finalized plan contains unresolved registered resources", ErrPlanNotExecutable)
}
}
return nil
}Then, you can replace this loop with a call to plan.Validate().
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdfctl/migrations/namespacedpolicy/registered_resources_execute.go (1)
61-89: 🧹 Nitpick | 🔵 TrivialMinor: redundant
var err errorshadow on line 70.The outer
errfrom line 61 is guaranteednilafter theif err != nilreturn at lines 62-68, so the innervar err erroron line 70 is an unnecessary shadow. A plain=reuse would be clearer and avoid a potentialgovet/shadowfinding.♻️ Proposed simplification
if !hasExistingParent { - var err error created, err = e.handler.CreateRegisteredResource( ctx, namespace, plan.Source.GetName(), nil, metadataForCreate( plan.Source.GetId(), metadataLabels(plan.Source.GetMetadata()), e.runID, ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/migrations/namespacedpolicy/registered_resources_execute.go` around lines 61 - 89, The inner shadowing of err is unnecessary: in existingRegisteredResource call you already handled non-nil err, so in the block that calls e.handler.CreateRegisteredResource (function CreateRegisteredResource) remove the `var err error` declaration and reuse the outer `err` by assigning with `=` when calling CreateRegisteredResource (i.e., `created, err = e.handler.CreateRegisteredResource(...)`) so there is no shadowed variable and subsequent error handling that sets target.Execution and returns uses the same err and runID.
🤖 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 97-101: The current check only scans RegisteredResources for
Unresolved but you must preflight all finalized targets before mutating; add a
helper function planHasUnresolvedTargets(plan *Plan) that iterates and returns
true if any target has status TargetStatusUnresolved across ActionPlan.Targets,
SubjectConditionSetPlan.Targets, SubjectMappingPlan.Target,
RegisteredResourcePlan.Unresolved and RegisteredResourcePlan.Target, and
ObligationTriggerPlan.Target; call this from validatePlan (or where you
currently check RegisteredResources in execute.go) and return fmt.Errorf("%w:
finalized plan contains unresolved targets", ErrPlanNotExecutable) if it finds
any unresolved so execution fails before any phase mutates state.
In `@otdfctl/migrations/namespacedpolicy/registered_resources_execute.go`:
- Around line 136-146: The helper existingRegisteredResource currently returns
(nil, true, err) when GetRegisteredResource fails, which makes the "has
existing" boolean ambiguous on error; change the return on error to (nil, false,
err) so that when e.handler.GetRegisteredResource(ctx, target.ExistingID, "",
"") fails the function returns false for the existence flag (and the error)
while keeping the normal successful-return semantics for
(*policy.RegisteredResource, true, nil).
---
Outside diff comments:
In `@otdfctl/migrations/namespacedpolicy/registered_resources_execute.go`:
- Around line 61-89: The inner shadowing of err is unnecessary: in
existingRegisteredResource call you already handled non-nil err, so in the block
that calls e.handler.CreateRegisteredResource (function
CreateRegisteredResource) remove the `var err error` declaration and reuse the
outer `err` by assigning with `=` when calling CreateRegisteredResource (i.e.,
`created, err = e.handler.CreateRegisteredResource(...)`) so there is no
shadowed variable and subsequent error handling that sets target.Execution and
returns uses the same err and runID.
🪄 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: 87662b4b-1272-4be9-8eae-e73f438ead65
📒 Files selected for processing (15)
otdfctl/e2e/migrate-namespaced-policy.batsotdfctl/migrations/namespacedpolicy/actions_execute_test.gootdfctl/migrations/namespacedpolicy/execute.gootdfctl/migrations/namespacedpolicy/execute_test_helpers_test.gootdfctl/migrations/namespacedpolicy/finalize_plan.gootdfctl/migrations/namespacedpolicy/finalize_plan_test.gootdfctl/migrations/namespacedpolicy/obligation_triggers_execute.gootdfctl/migrations/namespacedpolicy/obligation_triggers_execute_test.gootdfctl/migrations/namespacedpolicy/plan.gootdfctl/migrations/namespacedpolicy/planner_test.gootdfctl/migrations/namespacedpolicy/registered_resources_execute.gootdfctl/migrations/namespacedpolicy/registered_resources_execute_test.gootdfctl/migrations/namespacedpolicy/subject_condition_sets_execute_test.gootdfctl/migrations/namespacedpolicy/subject_mappings_execute.gootdfctl/migrations/namespacedpolicy/subject_mappings_execute_test.go
| func (e *Executor) existingRegisteredResource(ctx context.Context, target *RegisteredResourceTargetPlan) (*policy.RegisteredResource, bool, error) { | ||
| if target == nil || target.ExistingID == "" { | ||
| return nil, false, nil | ||
| } | ||
|
|
||
| resource, err := e.handler.GetRegisteredResource(ctx, target.ExistingID, "", "") | ||
| if err != nil { | ||
| return nil, true, err | ||
| } | ||
| return resource, true, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: returning hasExistingParent=true together with a non-nil error is ambiguous.
When GetRegisteredResource fails, the helper returns (nil, true, err). The current caller checks err first so this is harmless, but semantically "we have an existing parent" should be false when we failed to load it. Returning (nil, false, err) would make the contract safer against future caller refactors that might forget to check the error before branching on the bool.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@otdfctl/migrations/namespacedpolicy/registered_resources_execute.go` around
lines 136 - 146, The helper existingRegisteredResource currently returns (nil,
true, err) when GetRegisteredResource fails, which makes the "has existing"
boolean ambiguous on error; change the return on error to (nil, false, err) so
that when e.handler.GetRegisteredResource(ctx, target.ExistingID, "", "") fails
the function returns false for the existence flag (and the error) while keeping
the normal successful-return semantics for (*policy.RegisteredResource, true,
nil).
|
1 participant
Summary
This PR simplifies the namespaced-policy migration plan so the emitted plan JSON is the primary output, rather than a richer artifact wrapper. It removes unused aggregate plan fields, collapses multi-target shapes where only a single target is valid, and trims bindings down to
the source identifiers execution actually needs.
No prune behavior is changed in this PR.
Changes
Why
The previous plan shape carried review-oriented and duplicate data that execution did not need. This change makes the finalized plan smaller, clearer, and closer to the actual execution contract.
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor