feat: add Data Router / ReportPortal integration for rhdh-plugin-export-overlays#78824
feat: add Data Router / ReportPortal integration for rhdh-plugin-export-overlays#78824zdrapela wants to merge 9 commits intoopenshift:mainfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds a Data Router / ReportPortal post-step to two e2e Helm test jobs; the Helm test script now exports platform metadata and JUnit results to SHARED_DIR, and a new step reads those artifacts, formats metadata and JUnit XML, and sends them to Data Router / ReportPortal. ChangesData Router / ReportPortal Integration
Sequence DiagramsequenceDiagram
participant Job as Helm Test Job
participant SharedDir as SHARED_DIR
participant DataRouter as Data Router Step
participant DRoute as droute CLI
participant ReportPortal as ReportPortal
Job->>SharedDir: write platform metadata (IS_OPENSHIFT, CONTAINER_PLATFORM, RHDH_VERSION)
Job->>SharedDir: write JUnit results (junit-results-overlay-e2e.xml)
DataRouter->>SharedDir: read metadata & JUnit files
DataRouter->>DataRouter: generate metadata JSON & process JUnit XML (attachments, properties)
DataRouter->>DRoute: droute send (with retries)
DRoute->>ReportPortal: deliver results
ReportPortal-->>DRoute: return request id
DataRouter->>DRoute: poll droute request get
DRoute->>ReportPortal: request launch status
ReportPortal-->>DRoute: return launch URL
DRoute-->>DataRouter: forward launch URL
DataRouter->>SharedDir: save STATUS_URL_REPORTPORTAL / status files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
openshift-ci
Bot
added
the
approved
zdrapela
force-pushed
the
overlay-data-router
branch
from
4312bde to
b4e2d31
Compare
![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
🧹 Nitpick comments (4)
ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml (1)
8-11: ⚡ Quick winConsider pinning
drouteto a specific image tag instead oflatest
tag: latestmeans any update to thedno/droute:latestimage can silently change CLI behaviour (flags, output format), breaking the--resultshandling, thegrep "request:"parsing, or thedroute request getoutput format without a corresponding change in this step.♻️ Proposed fix
from_image: name: droute namespace: dno - tag: latest + tag: v<version> # pin to the version validated against this script🤖 Prompt for 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. In `@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml` around lines 8 - 11, The step currently uses an unpinned image tag (from_image.name: droute, from_image.namespace: dno, from_image.tag: latest); change from_image.tag from "latest" to a specific immutable identifier (a fixed semver tag or an image digest) to prevent unexpected CLI/behavior changes—update the tag value in the same YAML block (or replace with an image digest) so the CI step always pulls a known, tested dno/droute image.ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh (3)
29-32: 💤 Low valueTypo:
TRESHOLD→THRESHOLD🔤 Proposed fix
-DATA_ROUTER_AUTO_FINALIZATION_TRESHOLD="0.9" +DATA_ROUTER_AUTO_FINALIZATION_THRESHOLD="0.9" DATA_ROUTER_PROJECT="main" METADATA_OUTPUT="data_router_metadata_output.json" -export DATA_ROUTER_AUTO_FINALIZATION_TRESHOLD DATA_ROUTER_PROJECT METADATA_OUTPUT +export DATA_ROUTER_AUTO_FINALIZATION_THRESHOLD DATA_ROUTER_PROJECT METADATA_OUTPUTAlso update line 173:
- --argjson auto_finalization_threshold "$DATA_ROUTER_AUTO_FINALIZATION_TRESHOLD" \ + --argjson auto_finalization_threshold "$DATA_ROUTER_AUTO_FINALIZATION_THRESHOLD" \🤖 Prompt for 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. In `@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh` around lines 29 - 32, Rename the misspelled environment variable DATA_ROUTER_AUTO_FINALIZATION_TRESHOLD to DATA_ROUTER_AUTO_FINALIZATION_THRESHOLD everywhere it's declared and exported (update the declaration DATA_ROUTER_AUTO_FINALIZATION_TRESHOLD and the export line), and also update any other references to the old name (e.g., the usage at the later occurrence around the previous line 173) so all code uses DATA_ROUTER_AUTO_FINALIZATION_THRESHOLD consistently.
270-271: 💤 Low valueUse a distinct variable name for the polling loop's
max_attemptsRe-declaring
local max_attempts=30in the same function scope shadows the outermax_attempts=10. While functionally safe (the first loop has already exited), it's misleading to a future reader and looks like a copy-paste error.♻️ Proposed fix
- local max_attempts=30 - local wait_seconds=2 + local max_poll_attempts=30 + local wait_seconds=2 local DATA_ROUTER_REQUEST_OUTPUT="" local REPORTPORTAL_LAUNCH_URL="" - for ((i = 1; i <= max_attempts; i++)); do - echo "Attempt ${i} of ${max_attempts}: Checking Data Router request completion..." + for ((i = 1; i <= max_poll_attempts; i++)); do + echo "Attempt ${i} of ${max_poll_attempts}: Checking Data Router request completion..." ... - if ((i < max_attempts)); then + if ((i < max_poll_attempts)); then ... done - echo "Warning: Could not retrieve ReportPortal launch URL after ${max_attempts} attempts" + echo "Warning: Could not retrieve ReportPortal launch URL after ${max_poll_attempts} attempts"🤖 Prompt for 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. In `@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh` around lines 270 - 271, The second polling loop re-declares local max_attempts=30 which shadows the earlier max_attempts=10 and is confusing; change the redeclared variable to a distinct name (e.g., max_attempts_poll2 or attempts_remaining) and update any references in that loop accordingly (also consider updating wait_seconds naming if needed) so the two loops use clearly separate variables instead of shadowing max_attempts.
229-243: ⚡ Quick winHoist the JUnit file-existence check out of the retry loop
The
junit_files_foundcheck (lines 232–238) produces the same result on every retry iteration sinceprocess_junit_filesdoesn't remove files and nothing adds new ones mid-loop. Running this traversal up to 10 times is unnecessary.Also, when no files are found the function
returns without callingsave_status_data_router_failed, so no failure marker is written despite theERROR:log message.♻️ Proposed fix
+ # Check for JUnit files once, before the retry loop + local junit_files_found=false + for file in "${SHARED_DIR}"/junit-*.xml; do + if [[ -f "$file" ]]; then + junit_files_found=true + break + fi + done + + if [[ "$junit_files_found" == false ]]; then + echo "ERROR: No JUnit results files (junit-*.xml) found in ${SHARED_DIR}" + save_status_data_router_failed true + return + fi + for ((i = 1; i <= max_attempts; i++)); do echo "Attempt ${i} of ${max_attempts} to send test results through Data Router." - local junit_files_found=false - for file in "${SHARED_DIR}"/junit-*.xml; do - if [[ -f "$file" ]]; then - junit_files_found=true - break - fi - done - - if [[ "$junit_files_found" == false ]]; then - echo "ERROR: No JUnit results files (junit-*.xml) found in ${SHARED_DIR}" - return - fi -🤖 Prompt for 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. In `@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh` around lines 229 - 243, Move the JUnit-existence traversal out of the retry loop: before the for ((i=1; i<=max_attempts; i++)) loop, check for any "${SHARED_DIR}"/junit-*.xml files (set junit_files_found) and if none are found call save_status_data_router_failed and return early; then remove the inner file-checking block inside the retry loop so retries only cover the send/process attempt (e.g., process_junit_files/send_data_router logic). Ensure you reference the same variables and functions (max_attempts, junit_files_found, SHARED_DIR, save_status_data_router_failed, process_junit_files) so the early-exit behavior and failure marker are correctly applied.
🤖 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
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`:
- Around line 36-40: The three variable assignments IS_OPENSHIFT,
CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION should use the same guarded
fallback as RHDH_VERSION so they won't be empty if the shared files are missing;
update the reads that set IS_OPENSHIFT, CONTAINER_PLATFORM, and
CONTAINER_PLATFORM_VERSION to capture failures (e.g., redirect errors and
default to a sensible value like "unknown" or "false") before exporting them so
the metadata JSON contains explicit fallback values rather than empty strings.
- Around line 245-250: The --results argument is currently passed a quoted glob
which prevents shell expansion when invoking droute send; fix by expanding the
glob into an array (e.g., results=( "${SHARED_DIR}"/junit-*.xml ) ), ensure the
array is non-empty or handle the no-match case, and then pass the expanded list
to droute as separate arguments (e.g., --results "${results[@]}") when calling
droute send; apply the same change to the other droute send invocation that uses
--results.
---
Nitpick comments:
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`:
- Around line 29-32: Rename the misspelled environment variable
DATA_ROUTER_AUTO_FINALIZATION_TRESHOLD to
DATA_ROUTER_AUTO_FINALIZATION_THRESHOLD everywhere it's declared and exported
(update the declaration DATA_ROUTER_AUTO_FINALIZATION_TRESHOLD and the export
line), and also update any other references to the old name (e.g., the usage at
the later occurrence around the previous line 173) so all code uses
DATA_ROUTER_AUTO_FINALIZATION_THRESHOLD consistently.
- Around line 270-271: The second polling loop re-declares local max_attempts=30
which shadows the earlier max_attempts=10 and is confusing; change the
redeclared variable to a distinct name (e.g., max_attempts_poll2 or
attempts_remaining) and update any references in that loop accordingly (also
consider updating wait_seconds naming if needed) so the two loops use clearly
separate variables instead of shadowing max_attempts.
- Around line 229-243: Move the JUnit-existence traversal out of the retry loop:
before the for ((i=1; i<=max_attempts; i++)) loop, check for any
"${SHARED_DIR}"/junit-*.xml files (set junit_files_found) and if none are found
call save_status_data_router_failed and return early; then remove the inner
file-checking block inside the retry loop so retries only cover the send/process
attempt (e.g., process_junit_files/send_data_router logic). Ensure you reference
the same variables and functions (max_attempts, junit_files_found, SHARED_DIR,
save_status_data_router_failed, process_junit_files) so the early-exit behavior
and failure marker are correctly applied.
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml`:
- Around line 8-11: The step currently uses an unpinned image tag
(from_image.name: droute, from_image.namespace: dno, from_image.tag: latest);
change from_image.tag from "latest" to a specific immutable identifier (a fixed
semver tag or an image digest) to prevent unexpected CLI/behavior changes—update
the tag value in the same YAML block (or replace with an image digest) so the CI
step always pulls a known, tested dno/droute image.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 45bd22c8-9148-4ef6-ab6c-ae852ac77bfb
📒 Files selected for processing (6)
ci-operator/config/redhat-developer/rhdh-plugin-export-overlays/redhat-developer-rhdh-plugin-export-overlays-main.yamlci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/ocp/helm/redhat-developer-rhdh-plugin-export-overlays-ocp-helm-commands.shci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/OWNERSci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.shci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.metadata.jsonci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml
| IS_OPENSHIFT=$(cat "$SHARED_DIR/IS_OPENSHIFT.txt") | ||
| CONTAINER_PLATFORM=$(cat "$SHARED_DIR/CONTAINER_PLATFORM.txt") | ||
| CONTAINER_PLATFORM_VERSION=$(cat "$SHARED_DIR/CONTAINER_PLATFORM_VERSION.txt") | ||
| RHDH_VERSION=$(cat "$SHARED_DIR/RHDH_VERSION.txt" 2>/dev/null || echo "unknown") | ||
| export IS_OPENSHIFT CONTAINER_PLATFORM CONTAINER_PLATFORM_VERSION RHDH_VERSION |
There was a problem hiding this comment.
Add fallbacks for IS_OPENSHIFT, CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION
RHDH_VERSION has a || echo "unknown" guard (line 39), but the three reads above it silently produce empty strings if the test step failed before writing the metadata files. With set +o errexit, there is no error; the metadata JSON is sent with empty attribute values.
♻️ Proposed fix
-IS_OPENSHIFT=$(cat "$SHARED_DIR/IS_OPENSHIFT.txt")
-CONTAINER_PLATFORM=$(cat "$SHARED_DIR/CONTAINER_PLATFORM.txt")
-CONTAINER_PLATFORM_VERSION=$(cat "$SHARED_DIR/CONTAINER_PLATFORM_VERSION.txt")
+IS_OPENSHIFT=$(cat "$SHARED_DIR/IS_OPENSHIFT.txt" 2>/dev/null || echo "unknown")
+CONTAINER_PLATFORM=$(cat "$SHARED_DIR/CONTAINER_PLATFORM.txt" 2>/dev/null || echo "unknown")
+CONTAINER_PLATFORM_VERSION=$(cat "$SHARED_DIR/CONTAINER_PLATFORM_VERSION.txt" 2>/dev/null || echo "unknown")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IS_OPENSHIFT=$(cat "$SHARED_DIR/IS_OPENSHIFT.txt") | |
| CONTAINER_PLATFORM=$(cat "$SHARED_DIR/CONTAINER_PLATFORM.txt") | |
| CONTAINER_PLATFORM_VERSION=$(cat "$SHARED_DIR/CONTAINER_PLATFORM_VERSION.txt") | |
| RHDH_VERSION=$(cat "$SHARED_DIR/RHDH_VERSION.txt" 2>/dev/null || echo "unknown") | |
| export IS_OPENSHIFT CONTAINER_PLATFORM CONTAINER_PLATFORM_VERSION RHDH_VERSION | |
| IS_OPENSHIFT=$(cat "$SHARED_DIR/IS_OPENSHIFT.txt" 2>/dev/null || echo "unknown") | |
| CONTAINER_PLATFORM=$(cat "$SHARED_DIR/CONTAINER_PLATFORM.txt" 2>/dev/null || echo "unknown") | |
| CONTAINER_PLATFORM_VERSION=$(cat "$SHARED_DIR/CONTAINER_PLATFORM_VERSION.txt" 2>/dev/null || echo "unknown") | |
| RHDH_VERSION=$(cat "$SHARED_DIR/RHDH_VERSION.txt" 2>/dev/null || echo "unknown") | |
| export IS_OPENSHIFT CONTAINER_PLATFORM CONTAINER_PLATFORM_VERSION RHDH_VERSION |
🤖 Prompt for 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.
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`
around lines 36 - 40, The three variable assignments IS_OPENSHIFT,
CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION should use the same guarded
fallback as RHDH_VERSION so they won't be empty if the shared files are missing;
update the reads that set IS_OPENSHIFT, CONTAINER_PLATFORM, and
CONTAINER_PLATFORM_VERSION to capture failures (e.g., redirect errors and
default to a sensible value like "unknown" or "false") before exporting them so
the metadata JSON contains explicit fallback values rather than empty strings.
| if output=$(droute send --metadata "$(get_metadata_output_path)" \ | ||
| --url "${DATA_ROUTER_URL}" \ | ||
| --username "${DATA_ROUTER_USERNAME}" \ | ||
| --password "${DATA_ROUTER_PASSWORD}" \ | ||
| --results "${SHARED_DIR}/junit-*.xml" \ | ||
| --verbose --wirelog 2>&1) && \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing droute send --results usages in the repo to understand
# whether glob patterns are quoted or unquoted (revealing the convention).
rg -n --type sh -A2 'droute send' | grep -E 'droute send|--results'Repository: openshift/release
Length of output: 1151
🏁 Script executed:
# Read the target file to verify the exact code at the mentioned lines
cat -n ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh | sed -n '70,80p;230,250p'Repository: openshift/release
Length of output: 1332
🏁 Script executed:
# Also check the other droute send usage mentioned in the initial results
cat -n ci-operator/step-registry/redhat-developer/rhdh/send/data-router/redhat-developer-rhdh-send-data-router-commands.sh | sed -n '280,295p'Repository: openshift/release
Length of output: 900
🏁 Script executed:
# Check the send-results-to-reportportal file as well
cat -n ci-operator/step-registry/send-results/to-reportportal/send-results-to-reportportal-commands.sh | sed -n '520,545p'Repository: openshift/release
Length of output: 1071
🏁 Script executed:
# Search for any documentation or comments about droute in the repository
rg -n 'droute' --type sh -B2 -A2 | head -100Repository: openshift/release
Length of output: 9343
--results receives a quoted glob pattern that won't expand
The issue is confirmed. Line 249 passes --results "${SHARED_DIR}/junit-*.xml" with the glob pattern fully quoted, preventing shell expansion. This contradicts lines 74 and 233 in the same file, which correctly unquote the glob ("${SHARED_DIR}"/junit-*.xml) to allow shell expansion.
The code checks for file existence with an unquoted glob at line 233, then passes a quoted glob to droute send. If droute expects actual file paths (not glob patterns), it will fail. Either way, this inconsistency is a bug.
The proposed fix is correct—extract the glob into an array and expand it:
🐛 Proposed fix
- if output=$(droute send --metadata "$(get_metadata_output_path)" \
- --url "${DATA_ROUTER_URL}" \
- --username "${DATA_ROUTER_USERNAME}" \
- --password "${DATA_ROUTER_PASSWORD}" \
- --results "${SHARED_DIR}/junit-*.xml" \
- --verbose --wirelog 2>&1) && \
+ # shellcheck disable=SC2206 # intentional word-splitting for glob expansion
+ local junit_files=( "${SHARED_DIR}"/junit-*.xml )
+ if output=$(droute send --metadata "$(get_metadata_output_path)" \
+ --url "${DATA_ROUTER_URL}" \
+ --username "${DATA_ROUTER_USERNAME}" \
+ --password "${DATA_ROUTER_PASSWORD}" \
+ --results "${junit_files[@]}" \
+ --verbose --wirelog 2>&1) && \The same issue exists at line 289 in redhat-developer-rhdh-send-data-router-commands.sh.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if output=$(droute send --metadata "$(get_metadata_output_path)" \ | |
| --url "${DATA_ROUTER_URL}" \ | |
| --username "${DATA_ROUTER_USERNAME}" \ | |
| --password "${DATA_ROUTER_PASSWORD}" \ | |
| --results "${SHARED_DIR}/junit-*.xml" \ | |
| --verbose --wirelog 2>&1) && \ | |
| # shellcheck disable=SC2206 # intentional word-splitting for glob expansion | |
| local junit_files=( "${SHARED_DIR}"/junit-*.xml ) | |
| if output=$(droute send --metadata "$(get_metadata_output_path)" \ | |
| --url "${DATA_ROUTER_URL}" \ | |
| --username "${DATA_ROUTER_USERNAME}" \ | |
| --password "${DATA_ROUTER_PASSWORD}" \ | |
| --results "${junit_files[@]}" \ | |
| --verbose --wirelog 2>&1) && \ |
🤖 Prompt for 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.
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`
around lines 245 - 250, The --results argument is currently passed a quoted glob
which prevents shell expansion when invoking droute send; fix by expanding the
glob into an array (e.g., results=( "${SHARED_DIR}"/junit-*.xml ) ), ensure the
array is non-empty or handle the no-match case, and then pass the expanded list
to droute as separate arguments (e.g., --results "${results[@]}") when calling
droute send; apply the same change to the other droute send invocation that uses
--results.
b4e2d31 to
71ddce7
Compare
|
@zdrapela, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh (2)
36-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win[Still unresolved from previous review] Missing fallbacks for
IS_OPENSHIFT,CONTAINER_PLATFORM, andCONTAINER_PLATFORM_VERSION.With
set +o errexitactive, if any of these files are absent (e.g., when the test step fails before writing them),catsilently fails and the variables are empty strings. The ReportPortal metadata JSON is then submitted with blank attribute values.RHDH_VERSIONalready has2>/dev/null || echo "unknown"— the same guard should be applied to the three reads above it.♻️ Proposed fix
-IS_OPENSHIFT=$(cat "$SHARED_DIR/IS_OPENSHIFT.txt") -CONTAINER_PLATFORM=$(cat "$SHARED_DIR/CONTAINER_PLATFORM.txt") -CONTAINER_PLATFORM_VERSION=$(cat "$SHARED_DIR/CONTAINER_PLATFORM_VERSION.txt") +IS_OPENSHIFT=$(cat "$SHARED_DIR/IS_OPENSHIFT.txt" 2>/dev/null || echo "unknown") +CONTAINER_PLATFORM=$(cat "$SHARED_DIR/CONTAINER_PLATFORM.txt" 2>/dev/null || echo "unknown") +CONTAINER_PLATFORM_VERSION=$(cat "$SHARED_DIR/CONTAINER_PLATFORM_VERSION.txt" 2>/dev/null || echo "unknown")🤖 Prompt for 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. In `@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh` around lines 36 - 40, The three variable reads IS_OPENSHIFT, CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION currently use plain cat and can produce empty strings when their files are missing; change their assignments (the lines that set IS_OPENSHIFT, CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION) to guard each cat with redirection and an or-fallback like `2>/dev/null || echo "<fallback>"` (e.g., fallback "unknown" or "false" as appropriate) so they never end up empty, then export the variables as before; ensure you only modify the assignments for IS_OPENSHIFT, CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION and leave RHDH_VERSION handling intact.
255-260:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win[Still unresolved from previous review] Quoted glob in
--resultsprevents shell expansion —droute sendreceives a literal pattern string.Line 259 passes
"${SHARED_DIR}/junit-*.xml"fully quoted, so bash does not expand the glob;droutereceives the literal string/tmp/shared/junit-*.xmlas the argument. This contradicts the unquoted globs used correctly at lines 74 ("${SHARED_DIR}"/junit-results-*.xml) and 243 ("${SHARED_DIR}"/junit-*.xml) in the same file. Ifdroutedoes not perform its own glob expansion, this will cause everydroute sendattempt to fail, exhausting all 10 retries and always writingSTATUS_DATA_ROUTER_FAILED=true.🐛 Proposed fix
+ # shellcheck disable=SC2206 # intentional word-splitting for glob expansion + local junit_files=( "${SHARED_DIR}"/junit-*.xml ) if output=$(droute send --metadata "$(get_metadata_output_path)" \ --url "${DATA_ROUTER_URL}" \ --username "${DATA_ROUTER_USERNAME}" \ --password "${DATA_ROUTER_PASSWORD}" \ - --results "${SHARED_DIR}/junit-*.xml" \ + --results "${junit_files[@]}" \ --verbose --wirelog 2>&1) && \🤖 Prompt for 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. In `@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh` around lines 255 - 260, The --results argument to droute send is passing a quoted glob so the shell won't expand it; change the invocation of droute send (the command that includes --results "${SHARED_DIR}/junit-*.xml") to allow shell globbing by quoting only the directory portion, e.g. use --results "${SHARED_DIR}"/junit-*.xml (matching the existing patterns used at the other droute calls) so the shell expands the junit-*.xml files before droute receives them.
🧹 Nitpick comments (1)
ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml (1)
8-11: ⚡ Quick winConsider pinning the
drouteimage to a specific tag or digest.
tag: latestmeans any update todno/droute:latest— including breaking CLI changes to--results,--metadata, or the output format ofdroute request get— will silently affect all jobs using this step. A pinned semver tag (e.g.,v1.2.3) or image digest would make upgrades intentional and auditable.♻️ Proposed change
from_image: name: droute namespace: dno - tag: latest + tag: v<X.Y.Z> # pin to a known-good version🤖 Prompt for 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. In `@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml` around lines 8 - 11, The from_image block in this step uses an unpinned image tag (from_image.name: droute, namespace: dno, tag: latest); change the tag to a fixed, auditable value (either a semver tag like v1.2.3 or an image digest) so jobs don't automatically pick up breaking updates—update the tag field in the from_image stanza to the chosen pinned tag or replace it with an image digest string.
🤖 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
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`:
- Around line 288-294: The current extraction assigns REPORTPORTAL_LAUNCH_URL
from DATA_ROUTER_REQUEST_OUTPUT using grep -o 'https://[^"]*' which can yield
multiple newline matches; update the pipeline that sets REPORTPORTAL_LAUNCH_URL
(the line that reads DATA_ROUTER_REQUEST_OUTPUT and creates
REPORTPORTAL_LAUNCH_URL) to select only the first match (for example pipe the
grep output to head -n1 or use awk 'NR==1') and ensure any surrounding
whitespace/newlines are trimmed before passing the value to
save_status_url_reportportal so STATUS_URL_REPORTPORTAL.txt contains a single
well-formed URL.
- Around line 250-252: The check for junit_files_found currently echoes
"ERROR..." and returns without setting status; change this to a non-error skip
by replacing the error log with a "WARNING: No JUnit..." using the existing
junit_files_found and SHARED_DIR variables and then record an explicit skipped
status (call save_status_data_router_skipped or, if that helper doesn't exist,
create and write a STATUS_DATA_ROUTER_SKIPPED marker) before returning so the
pipeline knows the step was intentionally skipped rather than failed.
---
Duplicate comments:
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`:
- Around line 36-40: The three variable reads IS_OPENSHIFT, CONTAINER_PLATFORM,
and CONTAINER_PLATFORM_VERSION currently use plain cat and can produce empty
strings when their files are missing; change their assignments (the lines that
set IS_OPENSHIFT, CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION) to guard
each cat with redirection and an or-fallback like `2>/dev/null || echo
"<fallback>"` (e.g., fallback "unknown" or "false" as appropriate) so they never
end up empty, then export the variables as before; ensure you only modify the
assignments for IS_OPENSHIFT, CONTAINER_PLATFORM, and CONTAINER_PLATFORM_VERSION
and leave RHDH_VERSION handling intact.
- Around line 255-260: The --results argument to droute send is passing a quoted
glob so the shell won't expand it; change the invocation of droute send (the
command that includes --results "${SHARED_DIR}/junit-*.xml") to allow shell
globbing by quoting only the directory portion, e.g. use --results
"${SHARED_DIR}"/junit-*.xml (matching the existing patterns used at the other
droute calls) so the shell expands the junit-*.xml files before droute receives
them.
---
Nitpick comments:
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml`:
- Around line 8-11: The from_image block in this step uses an unpinned image tag
(from_image.name: droute, namespace: dno, tag: latest); change the tag to a
fixed, auditable value (either a semver tag like v1.2.3 or an image digest) so
jobs don't automatically pick up breaking updates—update the tag field in the
from_image stanza to the chosen pinned tag or replace it with an image digest
string.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d701c66f-33e4-46c7-907f-082ebf1e0101
📒 Files selected for processing (6)
ci-operator/config/redhat-developer/rhdh-plugin-export-overlays/redhat-developer-rhdh-plugin-export-overlays-main.yamlci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/ocp/helm/redhat-developer-rhdh-plugin-export-overlays-ocp-helm-commands.shci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/OWNERSci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.shci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.metadata.jsonci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-ref.yaml
✅ Files skipped from review due to trivial changes (1)
- ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/OWNERS
| if [[ "$junit_files_found" == false ]]; then | ||
| echo "ERROR: No JUnit results files (junit-*.xml) found in ${SHARED_DIR}" | ||
| return |
There was a problem hiding this comment.
return after "ERROR:" log leaves STATUS_DATA_ROUTER_FAILED unset.
When no JUnit files exist in SHARED_DIR (e.g., tests were legitimately skipped because no workspaces changed), the script logs "ERROR: No JUnit results files" but returns without calling save_status_data_router_failed. This leaves the status file absent and the "ERROR:" prefix creates a misleading impression. Either save a failure status to make the condition explicit, or downgrade the log to "WARNING:" if absence of JUnit files is an expected non-failure condition.
♻️ Proposed fix (treat as non-error skip)
if [[ "$junit_files_found" == false ]]; then
- echo "ERROR: No JUnit results files (junit-*.xml) found in ${SHARED_DIR}"
- return
+ echo "WARNING: No JUnit results files (junit-*.xml) found in ${SHARED_DIR}. Skipping Data Router upload."
+ return 0
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ "$junit_files_found" == false ]]; then | |
| echo "ERROR: No JUnit results files (junit-*.xml) found in ${SHARED_DIR}" | |
| return | |
| if [[ "$junit_files_found" == false ]]; then | |
| echo "WARNING: No JUnit results files (junit-*.xml) found in ${SHARED_DIR}. Skipping Data Router upload." | |
| return 0 |
🤖 Prompt for 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.
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`
around lines 250 - 252, The check for junit_files_found currently echoes
"ERROR..." and returns without setting status; change this to a non-error skip
by replacing the error log with a "WARNING: No JUnit..." using the existing
junit_files_found and SHARED_DIR variables and then record an explicit skipped
status (call save_status_data_router_skipped or, if that helper doesn't exist,
create and write a STATUS_DATA_ROUTER_SKIPPED marker) before returning so the
pipeline knows the step was intentionally skipped rather than failed.
| DATA_ROUTER_REQUEST_OUTPUT=$(droute request get \ | ||
| --url "${DATA_ROUTER_URL}" \ | ||
| --username "${DATA_ROUTER_USERNAME}" \ | ||
| --password "${DATA_ROUTER_PASSWORD}" \ | ||
| "${DATA_ROUTER_REQUEST_ID}") | ||
|
|
||
| REPORTPORTAL_LAUNCH_URL=$(echo "$DATA_ROUTER_REQUEST_OUTPUT" | grep -o 'https://[^"]*') |
There was a problem hiding this comment.
grep -o 'https://[^"]*' can return multiple matches as a newline-separated string.
If droute request get output contains more than one HTTPS URL (e.g., a log line plus the actual launch URL), REPORTPORTAL_LAUNCH_URL becomes a multi-line string. The -n check passes (non-empty), and save_status_url_reportportal saves the entire multi-line value to STATUS_URL_REPORTPORTAL.txt. Tightening the pattern or taking only the first match avoids storing a malformed URL.
♻️ Proposed fix
- REPORTPORTAL_LAUNCH_URL=$(echo "$DATA_ROUTER_REQUEST_OUTPUT" | grep -o 'https://[^"]*')
+ REPORTPORTAL_LAUNCH_URL=$(echo "$DATA_ROUTER_REQUEST_OUTPUT" | grep -o 'https://[^"[:space:]]*' | head -n1)🤖 Prompt for 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.
In
`@ci-operator/step-registry/redhat-developer/rhdh-plugin-export-overlays/send/data-router/redhat-developer-rhdh-plugin-export-overlays-send-data-router-commands.sh`
around lines 288 - 294, The current extraction assigns REPORTPORTAL_LAUNCH_URL
from DATA_ROUTER_REQUEST_OUTPUT using grep -o 'https://[^"]*' which can yield
multiple newline matches; update the pipeline that sets REPORTPORTAL_LAUNCH_URL
(the line that reads DATA_ROUTER_REQUEST_OUTPUT and creates
REPORTPORTAL_LAUNCH_URL) to select only the first match (for example pipe the
grep output to head -n1 or use awk 'NR==1') and ensure any surrounding
whitespace/newlines are trimmed before passing the value to
save_status_url_reportportal so STATUS_URL_REPORTPORTAL.txt contains a single
well-formed URL.
There was a problem hiding this comment.
There is always just one URL
There was a problem hiding this comment.
@zdrapela, understood — if the droute request get output is guaranteed to contain exactly one HTTPS URL, the current pattern is fine as-is. Thanks for the clarification!
|
@zdrapela, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/pj-rehearse periodic-ci-redhat-developer-rhdh-main-e2e-ocp-operator-auth-providers-nightly periodic-ci-redhat-developer-rhdh-main-e2e-ocp-helm-upgrade-nightly |
|
@zdrapela: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@zdrapela, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@zdrapela, If the problem persists, please contact Test Platform. |
…rt-overlays Create a new send-data-router step for the overlay repo that sends JUnit test results to Data Router for ReportPortal ingestion. The overlay test step now writes platform metadata and JUnit XML to SHARED_DIR for the data-router post step to consume. Changes: - New step: redhat-developer-rhdh-plugin-export-overlays-send-data-router - Overlay test step writes IS_OPENSHIFT, CONTAINER_PLATFORM, CONTAINER_PLATFORM_VERSION, and RHDH_VERSION to SHARED_DIR - Overlay test step copies junit-results.xml to SHARED_DIR - Both e2e-ocp-helm and e2e-ocp-helm-nightly jobs now include data-router and gather post steps Jira: https://redhat.atlassian.net/browse/RHIDP-13047 Assisted-by: OpenCode
Fix process_junit_files to strip ../node_modules/.cache/ prefix from Playwright attachment paths so they resolve to the correct GCS location (artifacts/e2e-test-results/...). Derive install_method from JOB_NAME (operator vs helm-chart) instead of hardcoding, matching the rhdh data-router pattern. Add component=plugins attribute to ReportPortal launch metadata. Assisted-by: OpenCode
Align with rhdh data-router convention where the suffix identifies the test step (e.g., junit-results-showcase-ci-nightly.xml in rhdh). Assisted-by: OpenCode
Use a glob instead of a hardcoded filename so any junit-*.xml files produced by Playwright are forwarded to the data-router step. Assisted-by: OpenCode
Use junit-*.xml glob (matching droute send pattern) instead of junit-results-*.xml which would miss junit-results.xml. Assisted-by: OpenCode
Prevent validate_required_vars from killing the entire post chain when secrets are missing, allowing the gather chain to still run. Also fix comment typo (junit-results-*.xml → junit-*.xml) and add missing trailing newlines to metadata JSON files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Data-router failures should not prevent artifact gathering or fail the overall job. Add allow_best_effort_post_steps and best_effort with a 5m timeout on the send-data-router step reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add redhat-developer-rhdh-send-data-router + send-alert + gather post steps to tests that were missing them: - e2e-ocp-operator-auth-providers-nightly: main, release-1.8, release-1.9 - e2e-ocp-helm-upgrade-nightly: main, release-1.9 Also fix overlay config to use plain ref syntax (no best_effort/timeout on ref steps) matching the rhdh pattern. Assisted-by: OpenCode
zdrapela
force-pushed
the
overlay-data-router
branch
from
e1fcf17 to
ac5ac55
Compare
|
/pj-rehearse abort |
|
@zdrapela: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-redhat-developer-rhdh-main-e2e-ocp-operator-auth-providers-nightly periodic-ci-redhat-developer-rhdh-main-e2e-ocp-helm-upgrade-nightly |
|
@zdrapela: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse abort |
|
@zdrapela: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-redhat-developer-rhdh-main-e2e-ocp-operator-auth-providers-nightly |
|
@zdrapela: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-redhat-developer-rhdh-main-e2e-ocp-helm-upgrade-nightly |
|
@zdrapela: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zdrapela The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
removed
the
approved
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zdrapela The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@zdrapela, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@zdrapela, If the problem persists, please contact Test Platform. |
|
@zdrapela, If the problem persists, please contact Test Platform. |
|
@zdrapela, If the problem persists, please contact Test Platform. |
|
/retest |
|
@zdrapela: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Overlay data-router integration
redhat-developer-rhdh-plugin-export-overlays-send-data-routerstep that sends JUnit test results to Data Router for ReportPortal ingestionIS_OPENSHIFT,CONTAINER_PLATFORM,CONTAINER_PLATFORM_VERSION,RHDH_VERSION) and JUnit XML toSHARED_DIRe2e-ocp-helmande2e-ocp-helm-nightlytest definitionsjob_type,pr,job_name,rhdh_version,install_method(dynamic),cluster_type,container_platform,container_platform_version,component=pluginsrhdh data-router gap fix
redhat-developer-rhdh-send-data-routerpost step to tests that were missing it:e2e-ocp-operator-auth-providers-nightly: main, release-1.8, release-1.9e2e-ocp-helm-upgrade-nightly: main, release-1.9Depends on:
Jira: https://redhat.atlassian.net/browse/RHIDP-13047