Fix connected OVE job#79903
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGate sourcing of proxy-conf.sh on a new ChangesOVE Baremetal Installation and Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact-konflux |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact-konflux |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![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 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/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh`:
- Around line 13-19: The if-test directly expands DISCONNECTED and will
hard-fail under set -o nounset; update the conditional in
agent-qe-baremetal-install-ove-commands.sh to use a defaulted expansion for the
DISCONNECTED variable (use the ${DISCONNECTED:-...} form) so the test does not
error when the variable is unset, leaving the rest of the block (checking
SHARED_DIR/proxy-conf.sh and sourcing it) unchanged.
In
`@ci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh`:
- Around line 3-6: The script currently overwrites NO_PROXY with only
"localhost,127.0.0.1" which causes Redfish/BMC curl calls to go through the
proxy; instead, read the proxy into HTTP_PROXY/HTTPS_PROXY as you already do,
then build NO_PROXY by starting from any existing NO_PROXY value and appending
"localhost,127.0.0.1" plus the BMC hostnames/IPs parsed from
${SHARED_DIR}/hosts.yaml (extract the host entries used by the curl calls) and
export that combined NO_PROXY before the curl calls; update the NO_PROXY
assignment in baremetal-lab-pre-serial-console-commands.sh to preserve existing
NO_PROXY (e.g.,
NO_PROXY="${NO_PROXY:+$NO_PROXY,}localhost,127.0.0.1,${BMC_LIST}") where
BMC_LIST is populated by parsing ${SHARED_DIR}/hosts.yaml, then ensure the curl
commands (the ones using https://$bmc_address) run with this updated NO_PROXY.
🪄 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: 14741f88-00fa-40fb-ad74-318685af0f85
📒 Files selected for processing (8)
ci-operator/config/openshift-eng/agent-qe-infra/openshift-eng-agent-qe-infra-release-4.21__amd64-nightly.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-ref.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-ref.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-ref.yamlci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh
✅ Files skipped from review due to trivial changes (1)
- ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-ref.yaml
- ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-ref.yaml
- ci-operator/config/openshift-eng/agent-qe-infra/openshift-eng-agent-qe-infra-release-4.21__amd64-nightly.yaml
| if [ "${DISCONNECTED}" == "true" ]; then | ||
| [ ! -f "${SHARED_DIR}/proxy-conf.sh" ] && { | ||
| echo "Proxy conf file is not found. Failing." | ||
| exit 1 | ||
| } | ||
| source "${SHARED_DIR}/proxy-conf.sh" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh"
if [ ! -f "$FILE" ]; then
echo "MISSING: $FILE"
exit 1
fi
echo "---- Relevant lines ($FILE) ----"
nl -ba "$FILE" | sed -n '1,80p'
echo "---- grep DISCONNECTED occurrences ----"
rg -n "DISCONNECTED" "$FILE" || trueRepository: openshift/release
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh"
if [ ! -f "$FILE" ]; then
echo "MISSING: $FILE"
exit 1
fi
echo "---- First 120 lines ($FILE) ----"
awk 'NR<=120 { printf "%5d\t%s\n", NR, $0 }' "$FILE"
echo "---- DISCONNECTED occurrences ($FILE) ----"
grep -n "DISCONNECTED" "$FILE" || trueRepository: openshift/release
Length of output: 3301
Guard DISCONNECTED against nounset hard-fail.
ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh enables set -o nounset and expands ${DISCONNECTED} directly in if [ "${DISCONNECTED}" == "true" ]; then, which fails when DISCONNECTED is unset. Use a defaulted expansion.
Suggested patch
-if [ "${DISCONNECTED}" == "true" ]; then
+if [ "${DISCONNECTED:-false}" == "true" ]; then📝 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 [ "${DISCONNECTED}" == "true" ]; then | |
| [ ! -f "${SHARED_DIR}/proxy-conf.sh" ] && { | |
| echo "Proxy conf file is not found. Failing." | |
| exit 1 | |
| } | |
| source "${SHARED_DIR}/proxy-conf.sh" | |
| fi | |
| if [ "${DISCONNECTED:-false}" == "true" ]; then | |
| [ ! -f "${SHARED_DIR}/proxy-conf.sh" ] && { | |
| echo "Proxy conf file is not found. Failing." | |
| exit 1 | |
| } | |
| source "${SHARED_DIR}/proxy-conf.sh" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 18-18: Not following: ./proxy-conf.sh was not specified as input (see shellcheck -x).
(SC1091)
🤖 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/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh`
around lines 13 - 19, The if-test directly expands DISCONNECTED and will
hard-fail under set -o nounset; update the conditional in
agent-qe-baremetal-install-ove-commands.sh to use a defaulted expansion for the
DISCONNECTED variable (use the ${DISCONNECTED:-...} form) so the test does not
error when the variable is unset, leaving the rest of the block (checking
SHARED_DIR/proxy-conf.sh and sourcing it) unchanged.
| proxy="$(<"${CLUSTER_PROFILE_DIR}/proxy")" | ||
| export HTTP_PROXY=${proxy} | ||
| export HTTPS_PROXY=${proxy} | ||
| export NO_PROXY="localhost,127.0.0.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant script with line numbers
target="ci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh"
echo "=== File: $target ==="
nl -ba "$target" | sed -n '1,140p'
# Check for other proxy/no_proxy handling in the step registry (nearby steps too)
echo "=== rg: NO_PROXY / HTTP_PROXY / HTTPS_PROXY usage in step-registry ==="
rg -n --hidden --no-ignore-vcs 'NO_PROXY|HTTP_PROXY|HTTPS_PROXY|curl .*https://\$bmc_address|bmc_address|redfish' ci-operator/step-registry/baremetal/lab/pre/serial-console -S || true
echo "=== rg: hosts.yaml / bmc_address parsing references ==="
rg -n --hidden --no-ignore-vcs 'hosts\.yaml|bmc_address|CLUSTER_PROFILE_DIR|SHARED_DIR' ci-operator/step-registry/baremetal/lab -S || true
# Show any hosts.yaml template/example if present (to validate suggested patch assumptions)
echo "=== Find hosts.yaml occurrences/files ==="
fd -a 'hosts\.yaml' ci-operator/step-registry/baremetal/lab || trueRepository: openshift/release
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="ci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh"
echo "=== Check file exists ==="
ls -la "$target" || true
echo "=== Show file (line-numbered via cat -n) ==="
if [ -f "$target" ]; then
cat -n "$target" | sed -n '1,160p'
fi
echo "=== rg: Proxy/NO_PROXY usage in relevant directory ==="
rg -n --hidden --no-ignore-vcs 'NO_PROXY|HTTP_PROXY|HTTPS_PROXY' "ci-operator/step-registry/baremetal/lab/pre/serial-console" -S || true
echo "=== rg: Redfish/BMC curl usage and bmc_address references in relevant directory ==="
rg -n --hidden --no-ignore-vcs 'curl .*https://\$bmc_address|bmc_address|redfish' "ci-operator/step-registry/baremetal/lab/pre/serial-console" -S || true
echo "=== rg: Any hosts.yaml parsing in this step subtree ==="
rg -n --hidden --no-ignore-vcs 'hosts\.yaml|\.bmc_address|bmc_address' "ci-operator/step-registry/baremetal/lab" -S || true
echo "=== Find any step-registry command scripts containing `export NO_PROXY=` globally ==="
rg -n --hidden --no-ignore-vcs 'export NO_PROXY=' ci-operator/step-registry -S || trueRepository: openshift/release
Length of output: 25659
Fix NO_PROXY handling for Redfish/BMC requests
baremetal-lab-pre-serial-console-commands.sh sets HTTP_PROXY/HTTPS_PROXY from CLUSTER_PROFILE_DIR/proxy but overwrites NO_PROXY to only localhost,127.0.0.1 (line 6). The script then runs curl -k ... https://$bmc_address/... for Redfish/iDRAC (lines 22, 23, 29, 35, 41), which can be routed through the proxy and break lab serial-console setup.
Update NO_PROXY to preserve existing entries and include the BMC hostnames from ${SHARED_DIR}/hosts.yaml (instead of replacing it with localhost-only).
🤖 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/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh`
around lines 3 - 6, The script currently overwrites NO_PROXY with only
"localhost,127.0.0.1" which causes Redfish/BMC curl calls to go through the
proxy; instead, read the proxy into HTTP_PROXY/HTTPS_PROXY as you already do,
then build NO_PROXY by starting from any existing NO_PROXY value and appending
"localhost,127.0.0.1" plus the BMC hostnames/IPs parsed from
${SHARED_DIR}/hosts.yaml (extract the host entries used by the curl calls) and
export that combined NO_PROXY before the curl calls; update the NO_PROXY
assignment in baremetal-lab-pre-serial-console-commands.sh to preserve existing
NO_PROXY (e.g.,
NO_PROXY="${NO_PROXY:+$NO_PROXY,}localhost,127.0.0.1,${BMC_LIST}") where
BMC_LIST is populated by parsing ${SHARED_DIR}/hosts.yaml, then ensure the curl
commands (the ones using https://$bmc_address) run with this updated NO_PROXY.
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact-konflux |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh (1)
13-13:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
DISCONNECTEDagainstnounsethard-fail.With
set -o nounsetenabled, the expansion${DISCONNECTED}will fail if the variable is unset. Use a defaulted expansion to prevent script failure.🛡️ Proposed fix
-if [ "${DISCONNECTED}" == "true" ]; then +if [ "${DISCONNECTED:-false}" == "true" ]; then🤖 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/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh` at line 13, The if-test using ${DISCONNECTED} can hard-fail under set -o nounset; update the conditional in agent-qe-baremetal-install-ove-commands.sh to use a defaulted expansion (e.g. ${DISCONNECTED:-false} or ${DISCONNECTED:-}) so the test safely handles an unset variable, replacing the current line that reads if [ "${DISCONNECTED}" == "true" ]; then with a defaulted form that compares the safe expansion to "true".
🤖 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/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.sh`:
- Line 22: The conditional using ${DISCONNECTED} will hard-fail under set -o
nounset; update the if check that references DISCONNECTED to use a defaulted
expansion (e.g. use ${DISCONNECTED:-false} or ${DISCONNECTED:-} in the
conditional) so the test does not error when DISCONNECTED is unset; locate the
conditional line that reads if [ "${DISCONNECTED}" == "true" ]; then and replace
the bare expansion with a defaulted one to safely handle unset variables.
In
`@ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.sh`:
- Line 13: The guard using ${DISCONNECTED} can hard-fail under set -o nounset;
update the conditional to use a defaulted expansion (e.g., ${DISCONNECTED:-}) so
the test doesn't throw when DISCONNECTED is unset—locate the if test that
references DISCONNECTED (the line starting with if [ "${DISCONNECTED}" == "true"
]; then) and replace the unguarded expansion with a defaulted form like
${DISCONNECTED:-false} or ${DISCONNECTED:-} to safely evaluate the condition.
---
Duplicate comments:
In
`@ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh`:
- Line 13: The if-test using ${DISCONNECTED} can hard-fail under set -o nounset;
update the conditional in agent-qe-baremetal-install-ove-commands.sh to use a
defaulted expansion (e.g. ${DISCONNECTED:-false} or ${DISCONNECTED:-}) so the
test safely handles an unset variable, replacing the current line that reads if
[ "${DISCONNECTED}" == "true" ]; then with a defaulted form that compares the
safe expansion to "true".
🪄 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: 6e6160b3-b35b-40c2-8bb0-97fbd17e44d4
📒 Files selected for processing (8)
ci-operator/config/openshift-eng/agent-qe-infra/openshift-eng-agent-qe-infra-release-4.21__amd64-nightly.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-ref.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-ref.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-ref.yamlci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh
✅ Files skipped from review due to trivial changes (3)
- ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-ref.yaml
- ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-ref.yaml
- ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/config/openshift-eng/agent-qe-infra/openshift-eng-agent-qe-infra-release-4.21__amd64-nightly.yaml
- ci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact-konflux |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact-konflux |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh (1)
13-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
DISCONNECTEDfornounsetsafety.With
set -o nounset(Line 6),${DISCONNECTED}can hard-fail when unset. Use a defaulted expansion.Suggested patch
-if [ "${DISCONNECTED}" == "true" ]; then +if [ "${DISCONNECTED:-false}" == "true" ]; then [ ! -f "${SHARED_DIR}/proxy-conf.sh" ] && { echo "Proxy conf file is not found. Failing." exit 1 } source "${SHARED_DIR}/proxy-conf.sh" fi#!/bin/bash set -euo pipefail FILE="ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh" nl -ba "$FILE" | sed -n '1,30p' echo "--- nounset + unguarded DISCONNECTED check ---" rg -n 'set -o nounset|"\$\{DISCONNECTED\}"' "$FILE"🤖 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/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh` around lines 13 - 19, The check if [ "${DISCONNECTED}" == "true" ] can fail under set -o nounset when DISCONNECTED is unset; change the condition to use a defaulted expansion like ${DISCONNECTED:-} (i.e., reference DISCONNECTED with a fallback) so the shell won't error on nounset. Update the conditional that references DISCONNECTED in the installation script (the if block that sources proxy-conf.sh and checks SHARED_DIR/proxy-conf.sh) to use the defaulted expansion and keep the rest of the logic unchanged.ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.sh (1)
22-28:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse defaulted expansion for
DISCONNECTED.
set -o nounsetmakes${DISCONNECTED}unsafe if unset; guard it with a default.Suggested patch
-if [ "${DISCONNECTED}" == "true" ]; then +if [ "${DISCONNECTED:-false}" == "true" ]; then [ ! -f "${SHARED_DIR}/proxy-conf.sh" ] && { echo "Proxy conf file is not found. Failing." exit 1 } source "${SHARED_DIR}/proxy-conf.sh" fi#!/bin/bash set -euo pipefail FILE="ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.sh" nl -ba "$FILE" | sed -n '1,40p' echo "--- nounset + unguarded DISCONNECTED check ---" rg -n 'set -o nounset|"\$\{DISCONNECTED\}"' "$FILE"🤖 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/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.sh` around lines 22 - 28, The check against DISCONNECTED is unsafe under set -o nounset; update the conditional to use defaulted expansion so an unset variable doesn't error. Replace the existing if test (referencing DISCONNECTED) with a guarded form like if [ "${DISCONNECTED:-}" = "true" ]; then (or use "${DISCONNECTED:-false}") and keep sourcing "${SHARED_DIR}/proxy-conf.sh" unchanged so the script continues to work when DISCONNECTED is unset.ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.sh (1)
13-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPrevent
nounsetfailure in DISCONNECTED check.Direct
${DISCONNECTED}expansion is unsafe withset -o nounset; default it in the condition.Suggested patch
-if [ "${DISCONNECTED}" == "true" ]; then +if [ "${DISCONNECTED:-false}" == "true" ]; then [ ! -f "${SHARED_DIR}/proxy-conf.sh" ] && { echo "Proxy conf file is not found. Failing." exit 1 } source "${SHARED_DIR}/proxy-conf.sh" fi#!/bin/bash set -euo pipefail FILE="ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.sh" nl -ba "$FILE" | sed -n '1,30p' echo "--- nounset + unguarded DISCONNECTED check ---" rg -n 'set -o nounset|"\$\{DISCONNECTED\}"' "$FILE"🤖 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/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.sh` around lines 13 - 19, The conditional directly expands DISCONNECTED which will fail under nounset; update the if test around DISCONNECTED to use a default/empty expansion (use the ${DISCONNECTED:-} form) and ensure the POSIX test operator is used (use = rather than ==) so the guard becomes safe under set -o nounset; keep the rest of the block (checking SHARED_DIR/proxy-conf.sh and sourcing it) unchanged and still quoted.
🤖 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.
Duplicate comments:
In
`@ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.sh`:
- Around line 13-19: The check if [ "${DISCONNECTED}" == "true" ] can fail under
set -o nounset when DISCONNECTED is unset; change the condition to use a
defaulted expansion like ${DISCONNECTED:-} (i.e., reference DISCONNECTED with a
fallback) so the shell won't error on nounset. Update the conditional that
references DISCONNECTED in the installation script (the if block that sources
proxy-conf.sh and checks SHARED_DIR/proxy-conf.sh) to use the defaulted
expansion and keep the rest of the logic unchanged.
In
`@ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.sh`:
- Around line 22-28: The check against DISCONNECTED is unsafe under set -o
nounset; update the conditional to use defaulted expansion so an unset variable
doesn't error. Replace the existing if test (referencing DISCONNECTED) with a
guarded form like if [ "${DISCONNECTED:-}" = "true" ]; then (or use
"${DISCONNECTED:-false}") and keep sourcing "${SHARED_DIR}/proxy-conf.sh"
unchanged so the script continues to work when DISCONNECTED is unset.
In
`@ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.sh`:
- Around line 13-19: The conditional directly expands DISCONNECTED which will
fail under nounset; update the if test around DISCONNECTED to use a
default/empty expansion (use the ${DISCONNECTED:-} form) and ensure the POSIX
test operator is used (use = rather than ==) so the guard becomes safe under set
-o nounset; keep the rest of the block (checking SHARED_DIR/proxy-conf.sh and
sourcing it) unchanged and still quoted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8e8ccc2c-a8a9-474f-b731-64c7d33e583d
📒 Files selected for processing (8)
ci-operator/config/openshift-eng/agent-qe-infra/openshift-eng-agent-qe-infra-release-4.21__amd64-nightly.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-ref.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-ref.yamlci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-commands.shci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-ref.yamlci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh
✅ Files skipped from review due to trivial changes (3)
- ci-operator/step-registry/agent-qe/baremetal/install/ove/agent-qe-baremetal-install-ove-ref.yaml
- ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/assisted-ui/agent-qe-baremetal-install-ove-disconnected-assisted-ui-ref.yaml
- ci-operator/step-registry/agent-qe/baremetal/install/ove/disconnected/agent-tui/agent-qe-baremetal-install-ove-disconnected-agent-tui-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/config/openshift-eng/agent-qe-infra/openshift-eng-agent-qe-infra-release-4.21__amd64-nightly.yaml
- ci-operator/step-registry/baremetal/lab/pre/serial-console/baremetal-lab-pre-serial-console-commands.sh
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-eng-agent-qe-infra-release-4.21-amd64-nightly-baremetal-ove-compact-konflux |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhanss, pamoedom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/pj-rehearse ack |
|
@mhanss: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
2 participants
Summary by CodeRabbit
This PR updates OpenShift CI configuration and ci-operator step scripts to fix a failing connected OVE (Open Virtualization Environment) baremetal job by making proxy handling conditional on whether the run is disconnected.
What changed (practical impact)
Concrete edits
Why this fixes the problem
Notes for reviewers