chore(cli): Namespaced-Policy bats prune tests and docs updates #3482
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 implements and verifies the prune workflow for namespaced policy migration. It ensures that legacy objects can be safely removed after migration by validating scopes, verifying dependency safety, and confirming idempotency. The changes include significant updates to the documentation and the addition of a comprehensive suite of end-to-end tests to ensure the reliability of the prune command. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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 legacy objects fade from view, As namespaced targets start anew. With prune in hand, the path is clear, Removing all the ghosts of yesteryear. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds single-scope namespaced-policy prune docs (scope validation, safety outcomes, examples, best practices) and extends the migrate namespaced-policy E2E BATS suite with teardown untracking, namespaced fixture helpers, prune-specific assertions, a prune-run helper, and per-scope prune tests validating deletion/retention and idempotency. ChangesNamespaced-policy prune documentation and E2E test coverage
Sequence DiagramsequenceDiagram
participant otdfctl as otdfctl CLI
participant PrunePlanner as PrunePlanner
participant ServerAPI as Server API
participant DB as Database
otdfctl->>PrunePlanner: request prune plan (--scope, --commit/--interactive)
PrunePlanner->>ServerAPI: request candidate legacy/target info
ServerAPI->>DB: query legacy objects and migrated targets
DB-->>ServerAPI: returns objects + labels (migrated_from, unlabeled)
ServerAPI-->>PrunePlanner: candidate classification (delete/blocked/unresolved)
PrunePlanner-->>otdfctl: plan summary and interactive confirmations
alt commit
otdfctl->>ServerAPI: delete confirmed legacy objects
ServerAPI->>DB: perform deletes
DB-->>ServerAPI: ack deletions
ServerAPI-->>otdfctl: prune results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 implements documentation and comprehensive end-to-end tests for the migrate prune namespaced-policy command. The documentation updates include detailed sections on prerequisites, delete safety, and best practices for pruning legacy policy objects. The E2E test suite is expanded with new helpers for untracking IDs, assertion functions for verifying pruned resources, and test cases covering various pruning scenarios across all policy scopes. Review feedback identified a bug in the untrack_* bash helpers where command substitution strips required newlines, noted potential global variable leakage in tests due to missing local declarations, and recommended replacing hardcoded connection strings with existing environment variables for better maintainability.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdfctl/docs/man/migrate/prune/namespaced-policy.md`:
- Around line 60-61: The example showing "otdfctl migrate prune
namespaced-policy --scope=obligation-triggers --interactive --commit" is
inconsistent with the documented shared apply flag (--commit only); either
remove "--interactive" from the example or explicitly document that the prune
subcommand supports an interactive mode. Update the namespaced-policy prune
examples to match the actual behavior of the prune command (edit the example to
use only "otdfctl migrate prune namespaced-policy --scope=obligation-triggers
--commit" if interactive is unsupported), or add a note in the prune
documentation and the shared flags section describing the "--interactive" flag
and how it interacts with "--commit" if interactive is supported.
In `@otdfctl/e2e/migrate-namespaced-policy.bats`:
- Around line 2228-2231: The test currently calls
untrack_action_id("$delete_a_id") and untrack_action_id("$delete_b_id") before
verifying prune success, which can leak fixtures if the prune assertions fail;
change the order so that assert_legacy_custom_action_pruned "$delete_a_id" and
assert_legacy_custom_action_pruned "$delete_b_id" run first and only
untrack_action_id for each ID after its corresponding prune assertion passes,
and apply the same reorder pattern for the other similar blocks (the ones
referencing untrack_action_id and assert_legacy_custom_action_pruned in the
file).
🪄 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: c0c6a324-91b5-4d29-9b58-2a7e1940f065
📒 Files selected for processing (4)
otdfctl/docs/man/migrate/namespaced-policy.mdotdfctl/docs/man/migrate/prune/_index.mdotdfctl/docs/man/migrate/prune/namespaced-policy.mdotdfctl/e2e/migrate-namespaced-policy.bats
💤 Files with no reviewable changes (1)
- otdfctl/docs/man/migrate/prune/_index.md
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:
|
|
2 participants
Summary
Adds end-to-end coverage for otdfctl migrate prune namespaced-policy --commit in otdfctl/e2e/migrate-namespaced-policy.bats.
The new tests cover:
Verification
Ran against a live local platform stack:
bats --tap e2e/migrate-namespaced-policy.bats --filter 'prune namespaced-policy'
All prune tests passed.
Summary by CodeRabbit
Documentation
migrate namespaced-policyis non-destructive; cleanup is performed viamigrate prune namespaced-policy.--scopeusage,--commit/--interactivebehavior, and updated examples.Tests