feat(cli): Namespaced migration tests.#3324
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 enhances test infrastructure by introducing conditional policy fixture provisioning and splitting test execution into tagged phases. A comprehensive new BATS test suite validates namespaced policy migration across three scopes. Changes also update git ignores and workflow configurations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
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 end-to-end testing for the namespaced policy migration process. To ensure test reliability, the CI pipeline has been modified to execute these migration tests separately from the main parallel test suite, preventing potential conflicts with unnamespaced policy fixtures. 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. Tests run in a line, so clean and so bright, Migration of policies, done in the light. No parallel chaos to cloud up the view, A robust new suite, for the work that we do. 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 introduces a new end-to-end test suite for namespaced-policy migration and updates the CI workflow to execute these tests in isolation. The isolation prevents conflicts between the migration planner's global object discovery and other parallel tests. The review feedback highlights several opportunities to improve the robustness of the shell scripts and BATS tests, including enabling pipefail in CI, avoiding word-splitting issues in command wrappers, ensuring proper error handling for CLI calls within tests, and using more secure methods for temporary directory creation.
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:
|
73dba6c to
6475ea0
Compare
3e7b640 to
2cbaa72
Compare
2cbaa72 to
33fcda0
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:
|
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:
|
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new end-to-end test suite for namespaced-policy migration and updates the CI configuration to run these tests in isolation. Key changes include updates to .gitignore, a new BATS test file with extensive helper functions, and a toggle for provisioning policy fixtures in the test platform. Feedback focuses on improving the robustness of the CI script to ensure all tests run even if the migration suite fails, and refining the BATS test helpers by removing unnecessary sh -c wrappers and utilizing BATS's run mechanism for better error reporting during CLI calls.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/action.yaml`:
- Around line 50-63: The namespaced_policy_migration pipeline must not
short-circuit the rest of the job under pipefail; wrap the migration pipeline so
its exit code is captured without propagating (e.g. temporarily disable pipefail
or use PIPESTATUS), run the remainder of the suite regardless, capture its exit
code too, and then fail the step if either phase failed. Concretely: around the
migration command "bats --tap e2e --filter-tags namespaced_policy_migration |
tee e2e/bats-results.tap" temporarily disable pipefail (or run then read
PIPESTATUS[0]) into a variable like migration_exit, run the remaining commands
("bats --tap e2e --filter-tags '!namespaced_policy_migration' ... | tee -a
e2e/bats-results.tap") capturing remainder_exit, then at the end check "if [
$migration_exit -ne 0 ] || [ $remainder_exit -ne 0 ]; then exit 1; fi" so both
phases always execute but the step still fails if either failed.
In `@otdfctl/e2e/migrate-namespaced-policy.bats`:
- Around line 718-720: Replace the ephemeral inline note ("! We should change
this to RR and Obligation Trigger...") with a trackable TODO referencing a
ticket/issue ID and, if not already created, open a follow-up issue describing
the intended switch from three subject mappings to Registered Resources +
Obligation Triggers for the actions-scope test fixtures; update the comment to
TODO(<ticket-number>): describe the change briefly and link the issue so
reviewers can find and close it later, and leave the existing
create_legacy_subject_mapping calls (create_legacy_subject_mapping) unchanged
until the follow-up is implemented.
- Around line 627-628: The extraction using jq -r '.id' into NS_A_ID combined
with assert_not_equal "$NS_A_ID" "" can silently accept the literal string
"null"; update every ID-extraction site (e.g., where NS_A_ID is set) to fail
fast by using jq in "error-on-missing" mode or a small helper (suggested name:
jq_required) that returns non-zero when the field is missing or equals "null",
and then assert the returned value; alternatively, add an explicit
assert_not_equal "$NS_A_ID" "null" check alongside the empty-string check so any
missing JSON field causes the test to fail immediately.
- Around line 667-685: The teardown function silently swallows errors for
deletions (subject-mappings, scs, actions) via "|| true", which can hide
transient failures and leave FK-linked resources undeleted; update teardown to
capture each delete command's stdout/stderr and exit code (for the commands
invoked with ./otdfctl using TRACKED_SUBJECT_MAPPING_IDS, TRACKED_SCS_IDS,
TRACKED_ACTION_IDS), and if a delete returns non-zero, emit a clear error/log
message including the resource id, the command output, and the exit code so
failures are visible (do not just ignore them), optionally retry transient
errors a few times before giving up.
- Around line 16-30: The helpers run_otdfctl_migrate, run_otdfctl_action,
run_otdfctl_sm and run_otdfctl_scs currently embed "$*" into a double-quoted sh
-c string which forces re-splitting and breaks args with spaces; update each
helper to invoke the command with an argv-safe form (e.g. avoid embedding
expanded args into a single sh -c string and instead pass arguments through
position parameters or use "$@" directly so quoting is preserved) so that
./otdfctl receives the original arguments intact (refer to the functions
run_otdfctl_migrate, run_otdfctl_action, run_otdfctl_sm, run_otdfctl_scs and
replace the sh -c "$... $*" pattern with a form that forwards $HOST,
$WITH_CREDS, the subcommand (migrate|policy actions|policy
subject-mappings|policy scs) and "$@" without re-splitting).
🪄 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: fb408110-f33e-485e-84e4-01aba706164a
📒 Files selected for processing (5)
.github/workflows/checks.yaml.gitignoreotdfctl/e2e/action.yamlotdfctl/e2e/migrate-namespaced-policy.batstest/start-up-with-containers/action.yaml
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
Proposed Changes
1.) Add E2E tests for migrating namespaced policy.
2.) Breakup namespaced policy migration tests from original suite invocation, due to parallelization complications and migration logic.
3.) Add the ability to choose whether or not the startup action should provision fixtures or not. Default to true, but require false for otdfctl tests.
Checklist
Testing Instructions
Summary by CodeRabbit
Tests
Chores