Conversation
|
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 46 minutes and 36 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 (2)
📝 WalkthroughWalkthroughThe changes extend the namespacedpolicy migration planner to retrieve and hydrate registered resource values, filter obligation triggers by namespaced action IDs, and introduce comprehensive end-to-end tests for registered resource and obligation trigger migration scenarios. Changes
Sequence DiagramsequenceDiagram
participant Retriever
participant PolicyClient
participant RegisteredResourceAPI
participant ActionAPI
participant ObligationTriggerAPI
Retriever->>ActionAPI: ListActions (by namespace)
ActionAPI-->>Retriever: action IDs per namespace
Retriever->>RegisteredResourceAPI: ListRegisteredResources (by namespace)
RegisteredResourceAPI-->>Retriever: registered resource list
loop For each registered resource
Retriever->>RegisteredResourceAPI: ListRegisteredResourceValues (resourceID)
RegisteredResourceAPI-->>Retriever: hydrated values with metadata
end
Retriever->>ObligationTriggerAPI: ListObligationTriggers (by namespace)
ObligationTriggerAPI-->>Retriever: obligation triggers
Retriever->>Retriever: Filter triggers where action ID ∈ namespaced actions
Retriever-->>Retriever: Return hydrated resources & filtered triggers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 significant improvements to the namespaced-policy migration process by adding robust end-to-end test coverage and fixing critical bugs in the planner and retriever components. These changes ensure that registered resources and obligation triggers are correctly hydrated and planned during migration, improving the reliability and accuracy of the namespaced-policy migration flow. 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 migrates with care, New tests are added everywhere. With triggers fixed and values found, The namespaced logic is now sound. Footnotes
|
![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 extends the namespaced policy migration tool to support registered resources and obligation triggers. It introduces new retrieval logic in the planner to hydrate registered resources with their values and filters obligation triggers based on legacy action IDs. Additionally, the E2E test suite is significantly expanded with new helper functions and test cases covering various migration scenarios and idempotency. Review feedback highlights a potential N+1 performance issue during resource hydration, suggests minor optimizations for map initialization, and identifies opportunities for better consistency in the BATS test helpers.
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: 1
🤖 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 56-69: Validate and/or quote OPENTDF_DB_SCHEMA before injecting it
into the SET search_path in run_policy_db_sql: ensure the env value matches a
safe SQL identifier regex (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) and if not, fall back
to the default "opentdf_policy"; then interpolate the validated name as a quoted
identifier (wrap with double quotes) when building the SET search_path statement
so malformed values cannot break or inject SQL.
🪄 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: 8dde23c3-b912-4446-a67c-5418ffcc3784
📒 Files selected for processing (5)
otdfctl/e2e/migrate-namespaced-policy.batsotdfctl/migrations/namespacedpolicy/planner.gootdfctl/migrations/namespacedpolicy/planner_test.gootdfctl/migrations/namespacedpolicy/retrieve.gootdfctl/migrations/namespacedpolicy/retrieve_test.go
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 namespaced-policy migration of registered resources and obligation triggers, plus a simple all-scopes migration test. This also fixes namespaced-policy planner/retriever bugs uncovered by the new tests.
What changed
Why
The new e2e cases exposed two main issues:
Summary by CodeRabbit
Release Notes