feat(cli): Add E2E tests for namespaced policy.#3363
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 50 minutes and 5 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 (1)
📝 WalkthroughWalkthroughRefactors the namespaced migration E2E test to stop relying on planner output files and instead verify migration results via live in-cluster queries; adds namespace state and label-based "migrated_from" lookup helpers, and updates assertions and test flow accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 refactors the end-to-end testing suite for namespaced policy migrations. By shifting from output-based assertions to state-based snapshots, the tests become more robust and less susceptible to minor formatting changes in the CLI output. The changes also include improved helper functions for verifying object states and delta calculations, ensuring better validation of migration outcomes and idempotency. 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 namespace, With snapshots we track every trace. No longer we parse the output so long, Now state-based checks keep the logic strong. 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 refactors the migrate-namespaced-policy.bats end-to-end tests to verify migration outcomes by querying the live system state directly via otdfctl, removing the dependency on local JSON plan files. The changes include new helper functions for state delta assertions and resource lookups. Review feedback identifies syntax errors in jq sorting expressions, potential runtime failures when iterating over null fields, and suggests refactoring duplicated logic and sorting arrays to prevent flaky test results.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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: 2
🤖 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 423-441: The helper pairs have inconsistent parameter orders:
subject_mapping_json_by_migrated_from(namespace_filter, source_mapping_id) but
subject_mapping_id_by_migrated_from(source_mapping_id, namespace_filter); change
the id-returning helpers (subject_mapping_id_by_migrated_from,
scs_id_by_migrated_from, registered_resource_id_by_migrated_from,
obligation_trigger_id_by_migrated_from, etc.) to accept (namespace_filter,
source_mapping_id) in the same order as their corresponding *_json_* helpers,
update their internal delegation calls to pass the args in that same order, and
update all call sites to use the unified (namespace_filter, source_mapping_id)
ordering (registered_resource_value_* already matches and needs no change).
- Around line 337-421: The tests silently pass because namespace_state_json and
assert_namespace_state_delta don't check command failures or enable pipefail, so
failed ./otdfctl calls produce empty strings that make jq/--argjson fail
silently; update namespace_state_json to enable set -o pipefail (or run each
./otdfctl call via the test harness run + assert_success) and validate each
parsed total is a non-negative integer (e.g., check the exit status and that
variables actions_total, subject_mappings_total, scs_total,
registered_resources_total, obligation_triggers_total are numeric) before
calling jq -cn; if any check fails, fail fast with an error so
assert_namespace_state_delta receives valid JSON (or bail out) instead of empty
strings.
🪄 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: 3671b936-056d-496b-8fca-b2d71a0c1bc8
📒 Files selected for processing (1)
otdfctl/e2e/migrate-namespaced-policy.bats
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 337-378: The namespace_state_json function reads .pagination.total
into variables like actions_total, subject_mappings_total, scs_total,
registered_resources_total, and obligation_triggers_total but doesn't validate
they're numeric before passing to jq --argjson; add a validation step after each
extraction (for actions_total, subject_mappings_total, scs_total,
registered_resources_total, obligation_triggers_total) that checks the value
matches the regex ^[0-9]+$ and if not either set it to 0 or fail the test with a
clear message (e.g., "invalid total for <name>: <value>"), ensuring jq receives
a true JSON number and preventing silent empty JSON output from
namespace_state_json.
- Around line 535-541: The helper obligation_trigger_json_by_id has its
parameters flipped compared to the project's convention; change its signature to
accept (namespace_filter, trigger_id) and update its internals so the jq call
uses the second arg as the trigger ID (e.g., local namespace_filter="$1"; local
trigger_id="$2"), then find and update the three call sites that invoke
obligation_trigger_json_by_id to pass namespace_id first and trigger_id second
so the --namespace flag receives the namespace UUID rather than the trigger ID;
this aligns it with sibling helpers like subject_mapping_*, scs_*,
registered_resource_*, and obligation_trigger_*_by_migrated_from.
- Around line 535-541: The helper obligation_trigger_json_by_id currently calls
list --namespace and thus cannot see legacy triggers with NULL namespace_id;
change the tests to fetch source triggers by id without a namespace filter (use
the policy obligations triggers get --id <id> --json invocation) or add a new
helper (e.g., obligation_trigger_json_by_id_unfiltered) that lists/gets without
--namespace and use that helper in
assert_obligation_trigger_created_in_namespace and
assert_legacy_obligation_trigger_still_exists so comparisons use the real legacy
trigger payloads rather than an empty object.
🪄 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: d14e407a-dd18-4455-b3d7-6b2a543a0e74
📒 Files selected for processing (1)
otdfctl/e2e/migrate-namespaced-policy.bats
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
🤖 I have created a release *beep* *boop* --- ## [0.31.0](opentdf/platform@otdfctl/v0.30.0...otdfctl/v0.31.0) (2026-04-22) ### Features * **cli:** Add e2e tests and fix bugs. ([opentdf#3353](opentdf#3353)) ([213d843](opentdf@213d843)) * **cli:** Add E2E tests for namespaced policy. ([opentdf#3363](opentdf#3363)) ([e46f07d](opentdf@e46f07d)) * **cli:** Interactive confirmation ([opentdf#3360](opentdf#3360)) ([cd931d9](opentdf@cd931d9)) * **cli:** migrate otdfctl into platform monorepo ([opentdf#3205](opentdf#3205)) ([5177bec](opentdf@5177bec)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/sdk from 0.15.0 to 0.16.0 in /otdfctl ([opentdf#3357](opentdf#3357)) ([d829184](opentdf@d829184)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Krish Suchak <42231639+alkalescent@users.noreply.github.com>
2 participants
Proposed Changes
1.) Fix E2E tests to not rely on output anymore, but instead just take snapshots of the state of the policy objects in each namespace before and after commits. We validate that state just on the count, not the uniqueness of the objects within. Checking if an object has been created or already migrated is done within other checks.
Checklist
Testing Instructions
Summary by CodeRabbit