feat(cli): Add skeleton of executor and action only commits#3301
feat(cli): Add skeleton of executor and action only commits#3301c-r33d merged 4 commits intoDSPX-2655-migrate-otdfctlfrom
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 introduces the core infrastructure for the namespaced policy migration workflow. It establishes the planning and execution framework, enabling the system to derive target namespaces for legacy policy objects and perform dry-run migrations. The changes focus on building a robust, deterministic planning engine that ensures migration consistency before any actual state changes are applied. 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 shifts to a new space, Legacy objects find their place. With plans in hand and dry-run done, The migration journey has begun. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements the dry-run planning and initial execution logic for migrating legacy policy objects to namespaced policy within the otdfctl tool. It introduces a comprehensive migration framework including object retrieval, dependency-based target derivation, canonical comparison for deduplication, and a structured planning phase. Feedback focuses on improving the robustness and performance of the migration process. Specifically, it is recommended to ensure the migration plan is saved even if execution fails mid-process to track progress, and to move "not implemented" scope checks to the validation phase to prevent partial migrations. Additionally, performance optimizations were identified regarding the use of linear searches in retrieval loops and inefficient JSON marshaling during object canonicalization. A suggestion was also made to refactor the executor logic to reduce code duplication when handling existing or already migrated targets.
b73159d to
b832687
Compare
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:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
| return nil | ||
| } | ||
|
|
||
| func (e *Executor) executeSubjectConditionSets(_ context.Context, plan *Plan) error { |
There was a problem hiding this comment.
depending on length it may be beneficial to have these (or some of them) live in different files to avoid a giant executor.go
There was a problem hiding this comment.
Yeah, I agree. I will refactor as the code gets longer.
| migrationLabelRun = "migration_run" | ||
| ) | ||
|
|
||
| type ExecutorHandler interface { |
There was a problem hiding this comment.
is there a handler implementation yet or is that next?
There was a problem hiding this comment.
This is more a testing nicety. It will just expand on the different CreateXx rpcs.
Proposed Changes
1.) Adds the
executorwhich is in-charge of committing a specific plan.2.) Adds commit logic for
actionsonly3.) Adds stubs for other policy constructs.
Checklist
Testing Instructions