Conversation
Contributor
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 introduces an interactive confirmation and review workflow for 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. The policy moves to a brand new space, With interactive steps to keep the pace. No files to write, just text on the screen, A safer migration, clean and serene. Footnotes
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRefactors namespaced-policy migration: removes legacy registered-resources and artifact packages; reshapes plan models (Targets slices → single Target, pointer objects → ID strings); adds interactive backup-and-target review; introduces textual summary rendering; updates execution to treat skipped/unresolved targets as no-ops and extends ExecutorHandler with registered-resource lookup; removes CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Planner
participant Prompter
participant Executor
participant Handler
User->>CLI: migrate namespaced-policy --interactive --commit
CLI->>Planner: Build migration plan
Planner->>Handler: List resources/actions/mappings
Handler-->>Planner: Plan inputs
Planner-->>CLI: Plan (with Targets)
rect rgba(76, 175, 80, 0.5)
CLI->>Prompter: ConfirmNamespacedPolicyBackup()
Prompter->>User: Show backup warning
User-->>Prompter: confirm/abort
Prompter-->>CLI: result
end
rect rgba(33, 150, 243, 0.5)
CLI->>Prompter: ReviewNamespacedPolicyInteractiveCommit(plan)
loop per create-target
Prompter->>User: present target details
User-->>Prompter: confirm / skip / abort
alt skip
Prompter->>CLI: mark target skipped (propagate)
else abort
Prompter-->>CLI: ErrInteractiveReviewAborted
CLI-->>User: exit
end
end
Prompter-->>CLI: updated plan
end
rect rgba(255, 152, 0, 0.5)
CLI->>Executor: Execute approved plan
Executor->>Handler: create/migrate resources
Handler-->>Executor: execution results (created IDs)
Executor-->>CLI: execution summary
end
CLI->>CLI: RenderNamespacedPolicySummaryWithResult(plan,result)
CLI-->>User: display summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the namespaced-policy migration command to replace JSON file output with human-readable summaries and implements an interactive commit workflow. Key changes include simplifying the migration plan structure by moving from multiple targets to a single target for several policy objects, adding a backup confirmation step, and allowing users to skip or abort specific operations during interactive execution. Feedback highlights logic errors in the summary rendering where unresolved registered resources and subject mappings are omitted, and suggests moving validation logic to the Plan struct to improve maintainability.
Contributor
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: 6
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/plan.go (1)
142-151: 🧹 Nitpick | 🔵 Trivial
RegisteredResourceActionBinding.SourceActionIDlacksomitempty.Minor inconsistency with surrounding ID-field JSON tags (
existing_id,omitempty,action_source_id,omitempty,action_source_ids,omitempty). If an action-less binding is never emitted in practice, the current tag is fine; otherwise consideromitemptyfor symmetry and smaller plan artifacts.- SourceActionID string `json:"source_action_id"` + SourceActionID string `json:"source_action_id,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/migrations/namespacedpolicy/plan.go` around lines 142 - 151, The JSON tag for RegisteredResourceActionBinding.SourceActionID is missing omitempty; update the struct field SourceActionID in RegisteredResourceActionBinding to use `json:"source_action_id,omitempty"` (matching surrounding ID-field tags like existing_id and action_source_id) so empty/zero-value action IDs are omitted from serialized plans.
🤖 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/e2e/migrate-namespaced-policy.bats`:
- Around line 1211-1212: The test suite's setup() currently skips, but it still
calls the removed CLI flag --output "$output_file", which will break when
unskipped; update the helper/tests that call the CLI to stop using --output and
instead capture the CLI's stdout (or another supported artifact) and assert
against the new rendered summary format. Locate occurrences of --output
"$output_file" and the BATS assertions that read the JSON file, replace them
with code that runs the command without --output, collects stdout (or uses the
CLI's summary output hook), and adapt the assertions in the test helper (and any
functions referenced in setup()) to parse and validate the rendered summary
rather than the old JSON file.
In `@otdfctl/migrations/namespacedpolicy/execute.go`:
- Around line 97-101: Fix the typo in the comment ("withint" → "within"),
extract the loop that checks plan.RegisteredResources into a helper on the plan
type (e.g., Plan.ValidateNoUnresolvedRegisteredResources or similar in plan.go)
so execute.go stays orchestration-only, and improve the error returned when
finding an unresolved resource: return ErrPlanNotExecutable wrapped with a
message that includes the specific resource identifier (use the appropriate
field on RegisteredResourcePlan such as resource.Source.GetName() or
resource.Name) and make the wording singular (e.g., "finalized plan contains
unresolved registered resource: <id>").
In `@otdfctl/migrations/namespacedpolicy/finalize_plan.go`:
- Around line 245-271: In newSubjectMappingTarget, avoid allocating and
populating ActionSourceIDs and SubjectConditionSetSourceID before the status
switch; move the creation of ActionSourceIDs (make([]string,...)), the loop that
appends action.GetId(), and the assignment to SubjectConditionSetSourceID to
after the switch and only execute them for the NeedsCreate branch; keep the
target struct allocation and setting of TargetStatusAlreadyMigrated/ExistingID
for the AlreadyMigrated branch but return early without populating dependency
IDs so already-migrated plans do not receive unused IDs.
In `@otdfctl/migrations/namespacedpolicy/obligation_triggers_execute.go`:
- Around line 41-42: Add a table test row to TestExecuteObligationTriggers
covering the new TargetStatusSkipped branch: add an entry with
TargetStatusSkipped as the target status and assert that no handler is invoked
and the execution result is not mutated (e.g., expectedCalls = 0,
expectedChanged = false). Update the test's loop expectations to verify the
handler call count remains zero and the ExecutionResult (or equivalent result
struct used in TestExecuteObligationTriggers) is unchanged after running the
executor for TargetStatusSkipped so the test mirrors the pattern used for other
status cases.
In `@otdfctl/migrations/namespacedpolicy/registered_resources_execute.go`:
- Around line 42-43: Add a unit test in registered_resources_execute_test.go
that exercises the TargetStatusSkipped branch: create a registered-resource
target that returns TargetStatusSkipped from the executor, invoke the same
execution entrypoint used by other tests (the test harness calling into the code
handling registered resources), assert that no parent or value create functions
are called (use existing mocks/spies used by other tests) and assert execution
state remains unchanged (no new entries/side-effects). Specifically target the
TargetStatusSkipped case to ensure the case in registered_resources_execute.go
(the `case TargetStatusSkipped: return nil` branch) is covered.
In `@otdfctl/migrations/namespacedpolicy/summary.go`:
- Around line 215-217: The loop over plan.RegisteredResources is skipping
entries with resource.Target == nil, which hides
RegisteredResourcePlan.Unresolved items; update the loop in summary.go (the
iteration over plan.RegisteredResources) to only skip when resource == nil or
resource.Source == nil, and treat entries with resource.Target == nil as valid
unresolved items: render them in the summary and increment the unresolved
counter (or mark them) when RegisteredResourcePlan.Unresolved is set so
unresolved registered resources are shown even if Target is nil.
---
Outside diff comments:
In `@otdfctl/migrations/namespacedpolicy/plan.go`:
- Around line 142-151: The JSON tag for
RegisteredResourceActionBinding.SourceActionID is missing omitempty; update the
struct field SourceActionID in RegisteredResourceActionBinding to use
`json:"source_action_id,omitempty"` (matching surrounding ID-field tags like
existing_id and action_source_id) so empty/zero-value action IDs are omitted
from serialized plans.
🪄 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: 6de337c6-c562-4117-81b6-0ea93e89e9fe
📒 Files selected for processing (35)
otdfctl/cmd/migrate/migrate.gootdfctl/cmd/migrate/namespaced_policy.gootdfctl/cmd/migrate/registeredResources.gootdfctl/docs/man/migrate/namespaced-policy.mdotdfctl/e2e/migrate-namespaced-policy.batsotdfctl/migrations/artifact/artifact.gootdfctl/migrations/artifact/artifact_test.gootdfctl/migrations/artifact/metadata/metadata.gootdfctl/migrations/artifact/v1/schema.gootdfctl/migrations/artifact/v1/schema_test.gootdfctl/migrations/namespacedpolicy/actions_execute.gootdfctl/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/interactive_commit.gootdfctl/migrations/namespacedpolicy/interactive_commit_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/retrieve.gootdfctl/migrations/namespacedpolicy/retrieve_test.gootdfctl/migrations/namespacedpolicy/subject_condition_sets_execute.gootdfctl/migrations/namespacedpolicy/subject_condition_sets_execute_test.gootdfctl/migrations/namespacedpolicy/subject_mappings_execute.gootdfctl/migrations/namespacedpolicy/subject_mappings_execute_test.gootdfctl/migrations/namespacedpolicy/summary.gootdfctl/migrations/namespacedpolicy/summary_test.gootdfctl/migrations/registered-resources.gootdfctl/migrations/registered-resources_test.gootdfctl/migrations/styles.go
💤 Files with no reviewable changes (9)
- otdfctl/cmd/migrate/migrate.go
- otdfctl/migrations/artifact/v1/schema_test.go
- otdfctl/migrations/artifact/artifact_test.go
- otdfctl/migrations/registered-resources_test.go
- otdfctl/cmd/migrate/registeredResources.go
- otdfctl/migrations/artifact/v1/schema.go
- otdfctl/migrations/artifact/artifact.go
- otdfctl/migrations/artifact/metadata/metadata.go
- otdfctl/migrations/registered-resources.go
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/subject_condition_sets_execute.go (1)
78-94: 🧹 Nitpick | 🔵 TrivialStale
//nolint:exhaustiverationale.The directive is still warranted (only
TargetStatusExistingStandardfalls todefault), but the comment text no longer reflects the code —TargetStatusSkippedandTargetStatusUnresolvedare now handled explicitly. Consider updating the rationale so future readers aren't misled.📝 Suggested tweak
- //nolint:exhaustive // SCS execution only handles create and already-migrated explicitly; all other statuses are unsupported. + //nolint:exhaustive // SCS execution handles create, already-migrated, skipped, and unresolved explicitly; existing-standard is not a valid SCS status and falls through to the default "unsupported" branch. switch target.Status {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/migrations/namespacedpolicy/subject_condition_sets_execute.go` around lines 78 - 94, Update the stale nolint comment above the switch handling TargetStatus in executeSubjectConditionSets (or the surrounding switch block) to accurately describe why //nolint:exhaustive is required: list the statuses that are handled explicitly (TargetStatusAlreadyMigrated, TargetStatusSkipped, TargetStatusCreate, TargetStatusUnresolved) and state that other statuses (e.g., TargetStatusExistingStandard and any future statuses) are intentionally unsupported and routed to default; replace the existing rationale text so it matches the current cases and clarifies that only the listed statuses are handled and all others fall through to the default error branch.
🤖 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/summary.go`:
- Around line 354-360: The obligation_value is rendering only GetId(), so
FQN-only obligation values show up blank; update
formatObligationTriggerCreatedLine (and the similar block at lines 432–437) to
use the same ID-or-FQN fallback as attribute_value by passing
trigger.Source.GetObligationValue() through valueFQN and rendering with
styles.Namespace().Render(valueFQN(...)) instead of obligationValueID/GetId(),
ensuring obligation_value displays either ID or FQN.
---
Outside diff comments:
In `@otdfctl/migrations/namespacedpolicy/subject_condition_sets_execute.go`:
- Around line 78-94: Update the stale nolint comment above the switch handling
TargetStatus in executeSubjectConditionSets (or the surrounding switch block) to
accurately describe why //nolint:exhaustive is required: list the statuses that
are handled explicitly (TargetStatusAlreadyMigrated, TargetStatusSkipped,
TargetStatusCreate, TargetStatusUnresolved) and state that other statuses (e.g.,
TargetStatusExistingStandard and any future statuses) are intentionally
unsupported and routed to default; replace the existing rationale text so it
matches the current cases and clarifies that only the listed statuses are
handled and all others fall through to the default error branch.
🪄 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: 9763b77a-ccfb-4a41-9b42-0f339479b2e1
📒 Files selected for processing (14)
otdfctl/go.modotdfctl/migrations/namespacedpolicy/actions_execute.gootdfctl/migrations/namespacedpolicy/actions_execute_test.gootdfctl/migrations/namespacedpolicy/execute.gootdfctl/migrations/namespacedpolicy/obligation_triggers_execute.gootdfctl/migrations/namespacedpolicy/obligation_triggers_execute_test.gootdfctl/migrations/namespacedpolicy/registered_resources_execute.gootdfctl/migrations/namespacedpolicy/registered_resources_execute_test.gootdfctl/migrations/namespacedpolicy/subject_condition_sets_execute.gootdfctl/migrations/namespacedpolicy/subject_condition_sets_execute_test.gootdfctl/migrations/namespacedpolicy/subject_mappings_execute.gootdfctl/migrations/namespacedpolicy/subject_mappings_execute_test.gootdfctl/migrations/namespacedpolicy/summary.gootdfctl/migrations/namespacedpolicy/summary_test.go
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
elizabethhealy
approved these changes
Apr 22, 2026
alkalescent
approved these changes
Apr 22, 2026
This was referenced Apr 22, 2026
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.
Proposed Changes
1.) Cleanup some of the plan artifact to only include what is needed.
2.) Remove the output file from
namespaced-policyand print summary of information to the screen3.) Add confirmation prompt before
commit4.) Add option to
skip,abort, 'commit' a specific object. Skipping an object that another requires causes both to be skipped.5.) Cleanup unused files
6.) Skip integration tests for now
Example screen shot of aborted plan
Summary by CodeRabbit
Removed Features
registered-resourcesmigration subcommand from the migrate command--outputflag from thenamespaced-policycommandNew Features
Changed Behavior
Documentation