chore(ci): AB-2181 - otdfctl e2e ci and results integration improvements#3361
chore(ci): AB-2181 - otdfctl e2e ci and results integration improvements#3361sievdokymov-virtru merged 8 commits intomainfrom
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 enhances the end-to-end testing infrastructure for 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. Ignored Files
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 tests run fast, the logs are clear, With colors bright, we have no fear. From main to manual, the gate is set, To keep our TestRail free from fret. Footnotes
|
📝 WalkthroughWalkthroughAdded a daily schedule and workflow_dispatch input to CI; made TestRail integration and mapping-report upload conditional on schedule/dispatch events; expanded TestRail mapping JSON with additional entries; and improved TAP-to-TestRail script key normalization, reporting labels, and JSON payload construction. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub as GitHub Actions
participant E2E as otdfctl/e2e composite
participant Script as upload-bats-test-results.sh
participant TestRail as TestRail API
participant Artifacts as GitHub Artifacts
GitHub->>E2E: trigger (schedule or workflow_dispatch) + inputs
E2E->>Script: run TAP parsing & mapping (pass run name)
Script->>Script: normalize names, map to case IDs, build JSON payload
Script->>TestRail: POST results payload (create/run lookup as needed)
Script->>Artifacts: upload TAP results (always)
Script->>Artifacts: upload mapping-report (only for schedule/dispatch)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 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)
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 updates the TestRail integration by restricting specific CI steps to the main branch or manual triggers, adding several new test case mappings, and enhancing the result upload script with colored output and improved name matching. The review feedback highlights several opportunities to improve the robustness of the shell script, specifically by using printf to handle special characters in test names, utilizing jq to prevent JSON injection when building result arrays, and ensuring that prefix stripping is applied symmetrically during test name lookups.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh (1)
71-108: 🧹 Nitpick | 🔵 TrivialPrefix stripping is correct but asymmetric — consider stripping prefixes from the TAP name too.
ltrimstris applied only to the mapping key. That's fine for the stated goal (tolerate[Auto]/(Auto)prefixes in TestRail case names stored in the mapping). However, if a TAP test name ever carries an[Auto]or(Auto)prefix (e.g., future bats naming convention), the lookup will fail because$n(the lowercased TAP name) won't match a plain mapping key. If you want true symmetry, also strip the prefixes from$nbefore comparison:♻️ Optional symmetric trim
- lowercasename=$(echo "$name" | tr '[:upper:]' '[:lower:]') + lowercasename=$(echo "$name" | tr '[:upper:]' '[:lower:]') + # Strip optional auto-prefixes from the TAP name as well + lowercasename="${lowercasename#\[auto\] }" + lowercasename="${lowercasename#(auto) }"Also note: the chained
ltrimstr("[auto] ") | ltrimstr("(auto) ")strips at most one of each prefix and only in that order; a key like"(auto) [auto] foo"would only have"(auto) "removed. Acceptable given current data, but worth being aware of.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh` around lines 71 - 108, The lookup_case_id function applies ltrimstr to mapping keys but not to the TAP name ($n), causing mismatches if TAP names include “[Auto] ” or “(Auto) ”; update both jq expressions (nested and flat branches) to normalize $n as well (e.g., use ($n | ascii_downcase | ltrimstr("[auto] ") | ltrimstr("(auto) ")) in the comparison) so the TAP name has the same prefix stripping and case-folding as the mapping keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh`:
- Around line 71-108: The lookup_case_id function applies ltrimstr to mapping
keys but not to the TAP name ($n), causing mismatches if TAP names include
“[Auto] ” or “(Auto) ”; update both jq expressions (nested and flat branches) to
normalize $n as well (e.g., use ($n | ascii_downcase | ltrimstr("[auto] ") |
ltrimstr("(auto) ")) in the comparison) so the TAP name has the same prefix
stripping and case-folding as the mapping keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88882b42-36f8-447f-8829-7cd66d3858da
📒 Files selected for processing (4)
.github/workflows/checks.yamlotdfctl/e2e/action.yamlotdfctl/e2e/testrail-integration/samples-for-virtru-instance/testname-to-testrail-id.virtru.jsonotdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh
There was a problem hiding this comment.
Pull request overview
This PR improves the otdfctl E2E CI ↔ TestRail integration by enabling manual runs with a custom TestRail run name, reducing TestRail noise from non-main runs, and making TAP→case-ID mapping more tolerant and readable.
Changes:
- Add
workflow_dispatchinput plumbing so the E2E suite can be run manually with a custom TestRail run name. - Gate TestRail upload + mapping-report artifact upload to only run on
mainorworkflow_dispatch. - Improve TestRail mapping robustness (strip
[Auto]/(Auto)prefixes during lookup), add colored mapping output, and update the Virtru mapping JSON with new case IDs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh |
Adds colorized mapping output and makes name matching tolerant of [Auto] / (Auto) prefixes. |
otdfctl/e2e/testrail-integration/samples-for-virtru-instance/testname-to-testrail-id.virtru.json |
Adds new TestRail case ID mappings for additional E2E tests. |
otdfctl/e2e/action.yaml |
Gates TestRail integration + artifact upload to main branch or manual dispatch. |
.github/workflows/checks.yaml |
Adds manual dispatch input and passes it through to the composite E2E action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
otdfctl/e2e/action.yaml (1)
75-90:⚠️ Potential issue | 🟠 MajorInclude push-to-main in the TestRail guards.
These conditions only allow
scheduleandworkflow_dispatch, so normalpushruns onmainskip TestRail upload and the mapping report, contrary to the PR objective.🐛 Proposed fix
- name: Integrate Bats test results into TestRail - if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' + if: github.event_name == 'workflow_dispatch' || github.ref == 'refs/heads/main' shell: bash working-directory: otdfctl run: | cd e2e @@ - name: Upload TestRail mapping report - if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' + if: github.event_name == 'workflow_dispatch' || github.ref == 'refs/heads/main' uses: actions/upload-artifact@v4 with: name: test-cases-mapping-report🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/e2e/action.yaml` around lines 75 - 90, The workflow steps "Integrate Bats test results into TestRail" and "Upload TestRail mapping report" are gated to only run on schedule or workflow_dispatch; update their if conditions to also allow pushes to main by adding a check for github.event_name == 'push' && github.ref == 'refs/heads/main' (or equivalently include github.ref == 'refs/heads/main') so normal pushes to main execute these steps; update the if expressions for those two step names accordingly.otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh (1)
150-163:⚠️ Potential issue | 🟠 MajorEscape
RUN_NAMEbefore using workflow-dispatch input in jq/JSON.
RUN_NAMEis set from the user-controllable workflow inputtestrail-run-name-for-cli-test. Values with special characters likeQA "smoke"break the jq filter on line 153 and the JSON body on line 162. Usejq --argand--argjsonto safely interpolate these values.🛡️ Proposed fix
find_existing_run() { curl -s -u "$TESTRAIL_USER:$TESTRAIL_PASS" \ "$TESTRAIL_URL/index.php?/api/v2/get_runs/$PROJECT_ID" | - jq ".runs[] | select(.name==\"$RUN_NAME\") | .id" | head -n1 + jq --arg run_name "$RUN_NAME" '.runs[] | select(.name == $run_name) | .id' | head -n1 } create_run() { - local case_ids_json + local case_ids_json payload case_ids_json=$(printf '%s\n' "${results[@]}" | jq -s '.[].case_id' | jq -s .) + payload=$(jq -n \ + --arg name "$RUN_NAME" \ + --argjson case_ids "$case_ids_json" \ + '{name: $name, include_all: false, case_ids: $case_ids}') curl -s -u "$TESTRAIL_USER:$TESTRAIL_PASS" \ -H "Content-Type: application/json" \ - -d "{\"name\": \"$RUN_NAME\", \"include_all\": false, \"case_ids\": $case_ids_json}" \ + -d "$payload" \ "$TESTRAIL_URL/index.php?/api/v2/add_run/$PROJECT_ID" | jq .id }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh` around lines 150 - 163, The RUN_NAME value is user-controlled and not escaped, which breaks the jq filter in find_existing_run and the JSON payload in create_run; update find_existing_run to pass RUN_NAME into jq with --arg (e.g., use jq --arg run "$RUN_NAME" '.runs[] | select(.name==$run) | .id') and rebuild the create_run payload with jq -n using --arg for name and --argjson for the case_ids array (generate case_ids_json as a proper JSON array and then call jq -n --arg name "$RUN_NAME" --argjson case_ids "$case_ids_json" '{name: $name, include_all: false, case_ids: $case_ids}' to produce the -d body) so all interpolations are safely escaped; apply changes in the functions find_existing_run and create_run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@otdfctl/e2e/action.yaml`:
- Around line 75-90: The workflow steps "Integrate Bats test results into
TestRail" and "Upload TestRail mapping report" are gated to only run on schedule
or workflow_dispatch; update their if conditions to also allow pushes to main by
adding a check for github.event_name == 'push' && github.ref ==
'refs/heads/main' (or equivalently include github.ref == 'refs/heads/main') so
normal pushes to main execute these steps; update the if expressions for those
two step names accordingly.
In `@otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh`:
- Around line 150-163: The RUN_NAME value is user-controlled and not escaped,
which breaks the jq filter in find_existing_run and the JSON payload in
create_run; update find_existing_run to pass RUN_NAME into jq with --arg (e.g.,
use jq --arg run "$RUN_NAME" '.runs[] | select(.name==$run) | .id') and rebuild
the create_run payload with jq -n using --arg for name and --argjson for the
case_ids array (generate case_ids_json as a proper JSON array and then call jq
-n --arg name "$RUN_NAME" --argjson case_ids "$case_ids_json" '{name: $name,
include_all: false, case_ids: $case_ids}' to produce the -d body) so all
interpolations are safely escaped; apply changes in the functions
find_existing_run and create_run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f5b6f93-8739-4328-b2a4-1baebcc5091b
📒 Files selected for processing (3)
.github/workflows/checks.yamlotdfctl/e2e/action.yamlotdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh
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 @.github/workflows/checks.yaml:
- Around line 578-579: The TestRail run name input is passed verbatim into
TESTRAIL_CLI_RUN_NAME and later interpolated into RUN_NAME inside
upload-bats-test-results-to-testrail.sh (lines around 153 and 162), which breaks
jq filters and JSON when the name contains quotes, backslashes, or newlines; fix
by changing the script to construct jq queries and JSON safely using jq --arg
(for filters) and jq -n (for payloads) instead of string interpolation, i.e.,
replace direct interpolation of RUN_NAME into jq filters/JSON with jq --arg
runName "$RUN_NAME" '... $runName ...' and jq -n --arg runName "$RUN_NAME"
'{name: $runName, ...}' so all special characters are escaped correctly.
- Around line 21-24: The workflow-level schedule and workflow_dispatch cause
every job (including go matrix, integration, benchmark, BDD, xtest, buflint,
license, and otdfctl-test) to run on schedule/dispatch; either scope triggers to
only the CLI/TestRail job or protect non-CLI jobs with job-level guards. Fix by
moving the cron/workflow_dispatch to a dedicated workflow for the CLI
E2E/TestRail run, or add job-level if conditions (e.g., if: github.event_name ==
'pull_request' or a more specific check) on non-CLI jobs such as the go-matrix,
integration, benchmark, BDD, xtest, buflint, and license jobs so they do not run
on scheduled/manual events; ensure comment-govulncheck and comment-benchmark
retain their existing PR-specific if logic.
🪄 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: 38d1404f-f512-477a-8ee5-3581b806a38a
📒 Files selected for processing (1)
.github/workflows/checks.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:
|
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: 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/testrail-integration/upload-bats-test-results-to-testrail.sh`:
- Around line 150-153: The pipeline in find_existing_run uses jq piped to head
-n1 which can cause the script to fail under pipefail if head exits before jq
finishes; remove the external head usage and let jq return only the first match
itself (e.g., use jq's first/limit construct in the query that currently reads
'.runs[] | select(.name == $run_name) | .id'), keep the same variables
(RUN_NAME, PROJECT_ID, TESTRAIL_URL) and ensure the curl -> jq pipeline exits
cleanly without relying on head.
🪄 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: ee702087-a01c-42e8-a7e9-9d5add836c05
📒 Files selected for processing (1)
otdfctl/e2e/testrail-integration/upload-bats-test-results-to-testrail.sh
|
Summary
Number of otdfctl e2e improvements:
workflow_dispatchinput tochecks.yamlso the E2E tests can be triggered on demand manually with a custom TestRail run namechecks.yamlto run tests in the morning and save CLI test results into TestRailaction.yamlnow only run on scheduled and manually triggered runs, preventing pollution TestRail with lots of PR and merge-to-main runs.upload-bats-test-results-to-testrail.shnow strips[Auto]/(Auto)prefixes from mapping file keys before comparison, so TestRail case names can carry those prefixes without breaking the TAP→case-ID lookup.YES_MAPPING_FOUND(green) /MAPPING_NOT_FOUND(red) for easier visual scanning of the report.testname-to-testrail-id.virtru.jsonupdated with new case IDs for policy namespaces, obligation triggers, SCS namespace filters, and subject mapping namespace variants.Covers AB-2181, AB-2168 JIRA tasks.
Test plan
[Auto]/(Auto)prefixes in the mapping file are correctly matched against plain TAP names.Summary by CodeRabbit
New Features
Chores