feat(cli): Build migration plan#3297
Merged
c-r33d merged 4 commits intoDSPX-2655-migrate-otdfctlfrom Apr 13, 2026
Merged
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:
📝 WalkthroughWalkthroughThis PR implements the previously-unimplemented Changes
Suggested labels
Suggested reviewers
Poem
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
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 the dry-run planning capability 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. Ignored Files
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 home, Where namespaces flourish and logic will roam. A dry-run is planned to keep data secure, With migration tools that are robust and pure. Footnotes
|
![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 migrates the otdfctl CLI into the opentdf/platform monorepo, including necessary updates to build scripts, Dockerfiles, and documentation. Several bugs were identified in the CLI command handlers where stale objects were being used in success responses and potential nil pointer dereferences existed in command mounting logic.
c-r33d
force-pushed
the
step-3-migrate-policy
branch
from
fad3c39 to
69593e3
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
c-r33d
force-pushed
the
step-3-migrate-policy
branch
from
69593e3 to
fa5c65d
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
c-r33d
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/cmd/migrate/namespacedPolicy.go`:
- Around line 41-47: The CLI currently accepts but ignores the --interactive
flag; add the same fail-fast behavior used for --commit by reading the
"interactive" flag from cmd.InheritedFlags() (similar to how `commit, err :=
cmd.InheritedFlags().GetBool("commit")` is handled) and, if true, call
cli.ExitWithError with a clear message like "--interactive is not supported yet"
and a matching error; place this check in the same namespaced-policy handling
block (where `commit` is checked) so both unsupported flags are rejected
consistently.
- Around line 67-81: The writeNamespacedPolicyPlan function currently defers
file.Close() which swallows any error from the final file flush; change the
logic to capture the error from encoder.Encode(plan) then explicitly close the
file and return the close error if present (otherwise return the encode error).
Update writeNamespacedPolicyPlan so it checks and returns errors from both
json.Encoder.Encode and file.Close instead of only returning encoder.Encode's
result with a deferred close.
In `@otdfctl/docs/man/migrate/namespaced-policy.md`:
- Around line 19-27: The documentation for the namespaced-policy migrate dry-run
still includes a copyable example that uses the unsupported --commit flag;
update the namespaced-policy man page by removing the example invocation that
includes --commit (or rewrite it to use only supported flags like --output and
--scope) so the examples match the note that "--commit is not implemented yet
for namespaced-policy" and the parent migrate's shared flags; search for the
example command string containing "--commit" in namespaced-policy.md and delete
or replace that example accordingly.
In `@otdfctl/migrations/namespacedpolicy/canonical.go`:
- Around line 220-229: The current sortByJSON function computes a separate keys
slice once then calls sort.SliceStable on items, but keys are not moved with
items during swaps, making the sort incorrect; fix by pairing each item with its
JSON key (e.g., create a slice of structs like {key string; value T}), sort that
slice by key using sort.SliceStable, and then rewrite the original items slice
in the new order so that the JSON keys stay attached to their elements; refer to
sortByJSON, keys, and items to locate and implement this change.
In `@otdfctl/migrations/namespacedpolicy/finalize_plan.go`:
- Around line 469-479: The addActionIssue function currently deduplicates by
scanning f.unresolved.Actions (checking issue.Source.GetId(),
sameNamespace(issue.Namespace, namespace) and issue.Reason) which is O(n);
replace this linear scan with a map-based set keyed by a stable tuple (sourceID,
namespaceID, reason) to achieve O(1) dedupe: introduce a map[string]struct{}
field on planFinalizer (or inside f.unresolved) and compute a key from
action.GetId(), namespace.GetId() (or use sameNamespace canonical id) and
reason; check/insert into that map before appending the new ActionIssue to
f.unresolved.Actions (and apply the same pattern to other similar add*Issue
methods such as where ActionIssue is constructed).
In `@otdfctl/migrations/namespacedpolicy/plan.go`:
- Around line 237-248: orderPlan currently omits deterministic sorting for
SubjectMappings, RegisteredResources, and ObligationTriggers leading to
non-deterministic JSON; update orderPlan (in function orderPlan) to invoke
ordering helpers for these sections (e.g.,
orderSubjectMappings(plan.SubjectMappings, namespacePositions),
orderRegisteredResources(plan.RegisteredResources, namespacePositions),
orderObligationTriggers(plan.ObligationTriggers, namespacePositions)) and
implement those helpers (or extend existing ones) to sort their top-level slices
and their nested target/binding slices using the same comparisons used by
orderActionPlans and orderSubjectConditionSetPlans (use namespacePositions for
any namespace-based ordering) so all nested collections are deterministically
ordered.
In `@otdfctl/migrations/namespacedpolicy/reduce_test.go`:
- Line 132: The test TODO should be replaced with a unit test that exercises the
ScopeObligationTriggers branch of reduceActions: create a fixture that includes
a policy/action with obligation triggers, call reduceActions(ctx,
ScopeObligationTriggers, ...) (or the test helper that invokes reduceActions)
and assert the expected reduced output and side effects (e.g., obligations
present/modified) so the ScopeObligationTriggers path in
otdfctl/migrations/namespacedpolicy/reduce.go is covered; reuse existing fixture
patterns in reduce_test.go to construct inputs and assertions to match other
tests and run go test ./... to verify.
In `@otdfctl/migrations/namespacedpolicy/reduce.go`:
- Around line 7-16: reduceDependencies currently mutates the input Retrieved by
updating retrieved.Candidates in-place which can surprise callers; either
document that behavior on the reduceDependencies function or make a defensive
copy before modifying. To fix by copying, create a shallow copy of the Retrieved
(e.g., newRet := *retrieved) and ensure you deep-copy the Candidates value (or
construct a new Candidates value) then call reduceActions(scopes,
newRet.Candidates) and reduceSubjectConditionSets(scopes, newRet.Candidates),
assign results to newRet.Candidates and return &newRet; alternatively add a
clear doc comment on reduceDependencies stating it mutates its input so callers
know the side-effect.
In `@otdfctl/migrations/namespacedpolicy/retrieve.go`:
- Around line 184-188: The paged collectors append every row from offset
pagination which can produce duplicates; in the loop that inspects mapping
(where you check isLegacyNamespace(mapping.GetNamespace()) and append to
candidates), add the same guard used elsewhere: skip entries with empty GetId()
or where hasObject(candidates, mapping.GetId()) is true before appending. Apply
the identical GetId()=="" || hasObject(...) check to the registered-resource and
obligation-trigger pagination loops referenced (same pattern used in
retrieveActions/retrieveSubjectConditionSets) at the other occurrences noted.
In `@otdfctl/migrations/namespacedpolicy/scopes.go`:
- Around line 128-134: The predicates requiresRegisteredResources and
requiresObligationTriggers currently return true when ScopeActions is present,
causing registered resources and obligation triggers to be loaded whenever
actions are requested; update these functions so each only checks for its own
scope (requiresRegisteredResources should return s.has(ScopeRegisteredResources)
and requiresObligationTriggers should return s.has(ScopeObligationTriggers)) so
the dependency direction matches expandScopes() and resources are only loaded
when explicitly requested.
🪄 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: ab7dcf60-40d2-4c48-8adf-43878c14f033
📒 Files selected for processing (21)
otdfctl/cmd/migrate/namespacedPolicy.gootdfctl/docs/man/migrate/namespaced-policy.mdotdfctl/migrations/namespacedpolicy/canonical.gootdfctl/migrations/namespacedpolicy/canonical_test.gootdfctl/migrations/namespacedpolicy/derived.gootdfctl/migrations/namespacedpolicy/derived_test.gootdfctl/migrations/namespacedpolicy/finalize_plan.gootdfctl/migrations/namespacedpolicy/finalize_plan_test.gootdfctl/migrations/namespacedpolicy/plan.gootdfctl/migrations/namespacedpolicy/plan_test.gootdfctl/migrations/namespacedpolicy/planner.gootdfctl/migrations/namespacedpolicy/planner_test.gootdfctl/migrations/namespacedpolicy/reduce.gootdfctl/migrations/namespacedpolicy/reduce_test.gootdfctl/migrations/namespacedpolicy/resolved.gootdfctl/migrations/namespacedpolicy/resolved_test.gootdfctl/migrations/namespacedpolicy/retrieve.gootdfctl/migrations/namespacedpolicy/retrieve_test.gootdfctl/migrations/namespacedpolicy/scopes.gootdfctl/migrations/namespacedpolicy/scopes_test.gootdfctl/migrations/namespacedpolicy/test_helpers_test.go
Contributor
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
elizabethhealy
approved these changes
Apr 13, 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.
Summary
Implements the dry-run planner for
migrate namespacedpolicy, the unified CLI entrypoint for migrating legacy (unnamespaced) policy objects into target namespaces.The planner builds a full graph plan before any writes, covering actions, subject condition sets, subject mappings, registered resources, and obligation triggers. It runs a staged pipeline:
CLI surface
migrate namespacedpolicy --scope=<csv> --output=<path>— writes the plan as JSONmigrate prune namespacedpolicy --scope=<csv>— command scaffold, not yet implemented--scopeacceptsactions,subject-condition-sets,subject-mappings,registered-resources,obligation-triggers--commitand--interactiveflags are wired but not yet implementedKey design decisions
subject-mappingspulls inactionsandsubject-condition-sets;registered-resourcesandobligation-triggerspull inactionsNot yet implemented
migrate prune namespacedpolicylive-graph evaluationSummary by CodeRabbit
New Features
namespaced-policymigration workflow with dry-run planning supportDocumentation
--commitis not yet implemented