Jf envar test#78870
Conversation
WalkthroughA new OpenShift ROSA E2E CI job configuration file is added that runs a test validating ChangesROSA E2E Job Link Test Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jfrazierRedHat 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 |
![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
🧹 Nitpick comments (1)
ci-operator/config/openshift-online/rosa-e2e/openshift-online-rosa-e2e-main__job-link-test.yaml (1)
49-54: ⚡ Quick win
env -i bashinjects unintended variables (SHLVL,PWD) into the env fileEven with
env -iand--norc --noprofile, bash automatically exportsSHLVL=1and setsPWD=<cwd>. Both will appear in theenvoutput and end up in/tmp/podman.env, forwarding them as unexpected variables into the nested container. This makes the "All environment variables" dump in Step 5 noisier than intended and the mechanism less obvious to readers.A direct
printfapproach produces only the intended variables:♻️ Simpler alternative for Step 3
- env -i bash --norc --noprofile << EOF > /tmp/podman.env - export TEST_VAR="hello from env file" - export JOB_LINK="${JOB_LINK}" - env | grep -v '^_=' - EOF + printf 'JOB_LINK=%s\nTEST_VAR=%s\n' \ + "${JOB_LINK}" \ + "hello from env file" \ + > /tmp/podman.env🤖 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/config/openshift-online/rosa-e2e/openshift-online-rosa-e2e-main__job-link-test.yaml` around lines 49 - 54, The heredoc using "env -i bash --norc --noprofile << EOF > /tmp/podman.env" injects unintended exports like SHLVL and PWD; replace that block with a direct writer that emits only the intended variables (e.g., write /tmp/podman.env via printf or cat with explicit "export TEST_VAR=...; export JOB_LINK=\"${JOB_LINK}\"") so JOB_LINK is substituted but no shell-startup variables are captured, and remove the "env | grep -v '^_='" line used inside the heredoc.
🤖 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/config/openshift-online/rosa-e2e/openshift-online-rosa-e2e-main__job-link-test.yaml`:
- Around line 40-45: The JOB_LINK construction contains unreachable PULL_NUMBER
logic and an incorrect hardcoded repo name; either remove the dead if-block
around PULL_NUMBER in the JOB_LINK assignment inside the JOB_LINK="..." section,
or (preferably) add a presubmit job so PULL_NUMBER can be set and exercise the
pr-logs path, and while doing so fix the pr-logs path to use the correct
org_repo token "openshift-online_rosa-e2e" instead of "openshift_release" in the
branch that builds pr-logs; locate the JOB_LINK block and the conditional that
references PULL_NUMBER and update the repo token or add a presubmit trigger
alongside the existing periodic trigger to make that branch reachable.
---
Nitpick comments:
In
`@ci-operator/config/openshift-online/rosa-e2e/openshift-online-rosa-e2e-main__job-link-test.yaml`:
- Around line 49-54: The heredoc using "env -i bash --norc --noprofile << EOF >
/tmp/podman.env" injects unintended exports like SHLVL and PWD; replace that
block with a direct writer that emits only the intended variables (e.g., write
/tmp/podman.env via printf or cat with explicit "export TEST_VAR=...; export
JOB_LINK=\"${JOB_LINK}\"") so JOB_LINK is substituted but no shell-startup
variables are captured, and remove the "env | grep -v '^_='" line used inside
the heredoc.
🪄 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: ed1f7bfd-17b9-4132-aa51-d567961fd748
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift-online/rosa-e2e/openshift-online-rosa-e2e-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (1)
ci-operator/config/openshift-online/rosa-e2e/openshift-online-rosa-e2e-main__job-link-test.yaml
| JOB_LINK="https://prow.ci.openshift.org/view/gs/test-platform-results/" | ||
| if [ -n "${PULL_NUMBER:-}" ]; then | ||
| JOB_LINK="${JOB_LINK}pr-logs/pull/openshift_release/${PULL_NUMBER}/${JOB_NAME}/${BUILD_ID}" | ||
| else | ||
| JOB_LINK="${JOB_LINK}logs/${JOB_NAME}/${BUILD_ID}" | ||
| fi |
There was a problem hiding this comment.
PULL_NUMBER branch is unreachable, and the hardcoded repo name is wrong
Two issues in this block:
-
Dead code: This job has
interval: 24hmaking it periodic-only. Periodic Prow jobs never havePULL_NUMBERset, so theif [ -n "${PULL_NUMBER:-}" ]branch can never execute. Thepr-logsconstruction logic is untested. -
Wrong repo name in the
pr-logspath: The GCSpr-logspath format ispr-logs/pull/<org>_<repo>/.... Since this config lives inopenshift-online/rosa-e2e, the path should useopenshift-online_rosa-e2e, not the hardcodedopenshift_release.
If testing both link-construction paths is the goal, a presubmit trigger is needed in addition to the periodic one.
🐛 Proposed fixes
- if [ -n "${PULL_NUMBER:-}" ]; then
- JOB_LINK="${JOB_LINK}pr-logs/pull/openshift_release/${PULL_NUMBER}/${JOB_NAME}/${BUILD_ID}"
- else
- JOB_LINK="${JOB_LINK}logs/${JOB_NAME}/${BUILD_ID}"
- fi
+ if [ -n "${PULL_NUMBER:-}" ]; then
+ JOB_LINK="${JOB_LINK}pr-logs/pull/openshift-online_rosa-e2e/${PULL_NUMBER}/${JOB_NAME}/${BUILD_ID}"
+ else
+ JOB_LINK="${JOB_LINK}logs/${JOB_NAME}/${BUILD_ID}"
+ fiTo also exercise the pr-logs path, add a presubmit entry alongside the periodic trigger in the test configuration.
🤖 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/config/openshift-online/rosa-e2e/openshift-online-rosa-e2e-main__job-link-test.yaml`
around lines 40 - 45, The JOB_LINK construction contains unreachable PULL_NUMBER
logic and an incorrect hardcoded repo name; either remove the dead if-block
around PULL_NUMBER in the JOB_LINK assignment inside the JOB_LINK="..." section,
or (preferably) add a presubmit job so PULL_NUMBER can be set and exercise the
pr-logs path, and while doing so fix the pr-logs path to use the correct
org_repo token "openshift-online_rosa-e2e" instead of "openshift_release" in the
branch that builds pr-logs; locate the JOB_LINK block and the conditional that
references PULL_NUMBER and update the repo token or add a presubmit trigger
alongside the existing periodic trigger to make that branch reachable.
|
@jfrazierRedHat: all tests passed! 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. |
|
/pj-rehearse periodic-ci-openshift-online-rosa-e2e-main-job-link-test-job-link-envar-test |
|
@jfrazierRedHat: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1 participant
Summary by CodeRabbit