MPIIT: Migrate Quay to new lpGA variant and Use ExitTrap for mapping - #79790
Open
oharan2 wants to merge 3 commits into
Open
MPIIT: Migrate Quay to new lpGA variant and Use ExitTrap for mapping#79790oharan2 wants to merge 3 commits into
oharan2 wants to merge 3 commits into
Conversation
Contributor
WalkthroughThis PR modernizes Quay CI E2E test workflows by standardizing shell execution with stricter flags, introducing conditional external artifact post-processing via ChangesQuay CI Test Infrastructure Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oharan2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/quay-tests/cso-qe-test/quay-tests-cso-qe-test-commands.sh`:
- Line 2: The script currently enables xtrace globally via the "set -euxo
pipefail" invocation; change that to use "set -euo pipefail" (remove the -x
flag) and keep "shopt -s inherit_errexit" as-is, and if debugging is needed
enable "set -x" only in a narrowly-scoped block or function; update the
top-level command that contains "set -euxo pipefail; shopt -s inherit_errexit"
to remove the -x flag and add local scoped tracing where required.
In
`@ci-operator/step-registry/quay-tests/deploy-quay-aws-s3/quay-tests-deploy-quay-aws-s3-commands.sh`:
- Line 2: Change the shell options initialization so tracing (-x) is not enabled
by default: replace the current global "set -euxo pipefail; shopt -s
inherit_errexit" with "set -euo pipefail; shopt -s inherit_errexit" and only
enable "set -x" locally around short debug sections when needed; ensure any
added debug blocks turn off tracing afterward (e.g., with "set +x") so scripts
default to no -x tracing while preserving inherit_errexit behavior.
In
`@ci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh`:
- Line 2: The script currently enables xtrace by using "set -euxo pipefail";
change the default to disable xtrace by using "set -euo pipefail" instead and
only add "set -x" in localized debug blocks when needed; update the
top-of-script invocation (the existing line containing "set -euxo pipefail;
shopt -s inherit_errexit") to remove the -x flag and, if required, add explicit
brief debug sections elsewhere that enable and then disable xtrace around
diagnostic commands.
- Around line 520-536: The two polling loops that set buildStatus and appStatus
currently can time out silently because the script always returns success (the
final "true"); after each loop (the buildStatus loop and the appStatus loop) add
an explicit check that the respective variable equals "True" and if not, emit a
clear error via echo or printf and exit 1; keep the existing loop logic and
sleeps, but replace the final unconditional success with these post-loop checks
so the step fails when readiness never becomes "True".
In
`@ci-operator/step-registry/quay-tests/test-quay-e2e/quay-tests-test-quay-e2e-commands.sh`:
- Line 2: The script prologue currently enables xtrace by using "set -euxo
pipefail", which violates the guideline; change this to disable default tracing
by removing the -x flag so the prologue uses "set -euo pipefail" (leave the
"shopt -s inherit_errexit" line as-is) and only enable -x where debugging is
explicitly needed.
🪄 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: 278c85b1-ffaf-4e75-9824-89b3b9fab5da
⛔ Files ignored due to path filters (1)
ci-operator/jobs/quay/quay-tests/quay-quay-tests-master-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (9)
ci-operator/config/quay/quay-tests/quay-quay-tests-master__ocp-4.22-quay-lpGA-lp-ocp-compat.yamlci-operator/step-registry/quay-tests/cso-qe-test/quay-tests-cso-qe-test-commands.shci-operator/step-registry/quay-tests/cso-qe-test/quay-tests-cso-qe-test-ref.yamlci-operator/step-registry/quay-tests/deploy-quay-aws-s3/quay-tests-deploy-quay-aws-s3-commands.shci-operator/step-registry/quay-tests/deploy-quay-aws-s3/quay-tests-deploy-quay-aws-s3-ref.yamlci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.shci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-ref.yamlci-operator/step-registry/quay-tests/test-quay-e2e/quay-tests-test-quay-e2e-commands.shci-operator/step-registry/quay-tests/test-quay-e2e/quay-tests-test-quay-e2e-ref.yaml
Comment on lines
+520
to
+536
| for ((waitIdx = 1; waitIdx <= 30; waitIdx++)); do | ||
| buildStatus="$(oc -n test-qbo get build -l buildconfig=rails-postgresql-example -o go-template='{{$x := ""}}{{range .items}}{{range .status.conditions}}{{if eq .type "Complete"}}{{if or (eq $x "") (eq .status "False")}}{{$x = .status}}{{end}}{{end}}{{end}}{{end}}{{or $x "False"}}' || true)" | ||
| if [[ "${buildStatus}" = "True" ]]; then | ||
| break | ||
| fi | ||
| echo "Build is NOT ready $_ times" | ||
| sleep 60 | ||
| done | ||
|
|
||
| for _ in {1..30}; do | ||
| app_status=$(oc -n test-qbo get pods -l deploymentconfig=rails-postgresql-example -o go-template='{{$x := ""}}{{range .items}}{{range .status.conditions}}{{if eq .type "Ready"}}{{if or (eq $x "") (eq .status "False")}}{{$x = .status}}{{end}}{{end}}{{end}}{{end}}{{or $x "False"}}') | ||
| if [ "$app_status" = "True" ]; then | ||
| echo "App pod pull image from quay successfully" | ||
| for ((waitIdx = 1; waitIdx <= 30; waitIdx++)); do | ||
| appStatus="$(oc -n test-qbo get pods -l deploymentconfig=rails-postgresql-example -o go-template='{{$x := ""}}{{range .items}}{{range .status.conditions}}{{if eq .type "Ready"}}{{if or (eq $x "") (eq .status "False")}}{{$x = .status}}{{end}}{{end}}{{end}}{{end}}{{or $x "False"}}' || true)" | ||
| if [[ "${appStatus}" = "True" ]]; then | ||
| break | ||
| fi | ||
| echo "App pod is NOT ready $_ times" | ||
| sleep 20 | ||
| done | ||
| echo "QE Test for Quay Bridge Operator is passed" | ||
|
|
||
| true |
Contributor
There was a problem hiding this comment.
Fail the step when build/app readiness never reaches True.
Both polling loops can exhaust without success, and Line 536 forces a zero exit status, so this step can pass even when the sanity flow never became ready. Add explicit post-loop checks and exit 1 on timeout.
Suggested fix
typeset buildStatus="" appStatus=""
for ((waitIdx = 1; waitIdx <= 30; waitIdx++)); do
buildStatus="$(oc -n test-qbo get build -l buildconfig=rails-postgresql-example -o go-template='{{$x := ""}}{{range .items}}{{range .status.conditions}}{{if eq .type "Complete"}}{{if or (eq $x "") (eq .status "False")}}{{$x = .status}}{{end}}{{end}}{{end}}{{end}}{{or $x "False"}}' || true)"
if [[ "${buildStatus}" = "True" ]]; then
break
fi
sleep 60
done
+if [[ "${buildStatus}" != "True" ]]; then
+ echo "Timed out waiting for build completion" 1>&2
+ exit 1
+fi
for ((waitIdx = 1; waitIdx <= 30; waitIdx++)); do
appStatus="$(oc -n test-qbo get pods -l deploymentconfig=rails-postgresql-example -o go-template='{{$x := ""}}{{range .items}}{{range .status.conditions}}{{if eq .type "Ready"}}{{if or (eq $x "") (eq .status "False")}}{{$x = .status}}{{end}}{{end}}{{end}}{{end}}{{or $x "False"}}' || true)"
if [[ "${appStatus}" = "True" ]]; then
break
fi
sleep 20
done
+if [[ "${appStatus}" != "True" ]]; then
+ echo "Timed out waiting for app readiness" 1>&2
+ exit 1
+fi
-true
+echo "QBO QE sanity checks passed"🤖 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/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh`
around lines 520 - 536, The two polling loops that set buildStatus and appStatus
currently can time out silently because the script always returns success (the
final "true"); after each loop (the buildStatus loop and the appStatus loop) add
an explicit check that the respective variable equals "True" and if not, emit a
clear error via echo or printf and exit 1; keep the existing loop logic and
sleeps, but replace the final unconditional success with these post-loop checks
so the step fails when readiness never becomes "True".
etirta
suggested changes
May 28, 2026
Contributor
|
[REHEARSALNOTIFIER]
A total of 92 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
Contributor
|
@oharan2: 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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh (1)
35-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when prerequisite readiness loops time out.
Both loops break on success, but neither checks for timeout afterward. If the Subscription never reaches
Succeededor the recreatedquay-apppod never becomesReady, the script keeps going and later failures become much harder to diagnose.Also applies to: 97-100
🤖 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/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh` around lines 35 - 43, The readiness loops that poll the Subscription installedCSV and the quay-app pod readiness (the for loop using waitIdx, the csv variable and the oc calls that check '{.status.installedCSV}' and '{.status.phase}' == "Succeeded", and the similar pod-ready loop around lines 97-100) do not fail if they time out; add an explicit timeout check immediately after each for loop that verifies success (e.g., ensure csv is non-empty and the CSV phase is "Succeeded" for the Subscription loop, and that the quay-app pod is Ready for the pod loop) and if not, print a clear error message identifying the failing resource (Subscription/quay-app) and exit with non-zero status (exit 1) so the pipeline fails fast and provides a diagnosable log entry.
🤖 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/quay-tests/deploy-quay-aws-s3/quay-tests-deploy-quay-aws-s3-commands.sh`:
- Around line 46-51: The generated Terraform file currently embeds plaintext AWS
credentials into create_aws_bucket.tf via the provider "aws" block; remove the
access_key and secret_key interpolation so the provider block does not persist
secrets, and instead rely on environment-based credentials
(AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY), an AWS profile, or Terraform
variables supplied at runtime (e.g., via a separate, non-archived tfvars or CI
secret injection). Update the code that writes create_aws_bucket.tf (the
here-doc that creates the provider "aws" block) to omit secret insertion and
ensure any alternative credential mechanism is used before packaging
terraform.tgz so no file in the archive contains credentials.
- Around line 240-256: The current subshell pipeline that posts to
"${quayRoute}/api/v1/user/initialize" and writes jq -r '.access_token' to
"${SHARED_DIR}/quay_oauth2_token" can fail the whole script on a transient
API/route race; wrap that pipeline (the curl + jq token extraction) in a retry
loop (e.g., until/for with sleep and a reasonable backoff/retry count) and
temporarily disable errexit for the pipeline so individual failures don't abort
the script, retrying until the curl + jq pipeline succeeds and then write the
token to ${SHARED_DIR}/quay_oauth2_token; keep the rest of the surrounding
behavior (set -euo pipefail) intact.
In
`@ci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh`:
- Around line 48-51: The token-generation subshell that writes
"${SHARED_DIR}/quay-access-token" can fail due to pipefail/SIGPIPE when head
exits early; change the subshell to either disable pipefail inside it (e.g., set
+o pipefail) or replace the pipeline with a single robust command that produces
40 alphanumeric chars without creating a fragile pipe (e.g., use dd/openssl to
read exact bytes then filter), ensuring the final write always succeeds;
additionally, make the bounded prerequisite waits for the QBO installedCSV and
quay-app readiness explicitly fail/exit non-zero on timeout (add explicit
timeout checks and set an error/exit path) so the script stops earlier instead
of continuing to later failures.
In
`@ci-operator/step-registry/quay-tests/test-quay-e2e/quay-tests-test-quay-e2e-commands.sh`:
- Around line 54-57: The env exports for HTTPS_PROXY/HTTP_PROXY and CYPRESS_*
are being done inside subshells with "( set +x ... )" so they do not persist to
the parent shell and thus npm run smoke won't see them; change each block to
avoid subshell-scoping by removing the surrounding parentheses, instead save the
current xtrace state, disable tracing (set +x), perform the export of
HTTPS_PROXY, HTTP_PROXY, and the CYPRESS_* variables (use the same identifiers
shown), then restore the saved xtrace state so the exports persist for later
commands like npm run smoke.
---
Outside diff comments:
In
`@ci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh`:
- Around line 35-43: The readiness loops that poll the Subscription installedCSV
and the quay-app pod readiness (the for loop using waitIdx, the csv variable and
the oc calls that check '{.status.installedCSV}' and '{.status.phase}' ==
"Succeeded", and the similar pod-ready loop around lines 97-100) do not fail if
they time out; add an explicit timeout check immediately after each for loop
that verifies success (e.g., ensure csv is non-empty and the CSV phase is
"Succeeded" for the Subscription loop, and that the quay-app pod is Ready for
the pod loop) and if not, print a clear error message identifying the failing
resource (Subscription/quay-app) and exit with non-zero status (exit 1) so the
pipeline fails fast and provides a diagnosable log entry.
🪄 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: be1f8598-1995-41cb-a87f-1f7fd3d74a34
⛔ Files ignored due to path filters (1)
ci-operator/jobs/quay/quay-tests/quay-quay-tests-master-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (5)
ci-operator/config/quay/quay-tests/quay-quay-tests-master__ocp-4.22-quay-lpGA-lp-ocp-compat.yamlci-operator/step-registry/quay-tests/cso-qe-test/quay-tests-cso-qe-test-commands.shci-operator/step-registry/quay-tests/deploy-quay-aws-s3/quay-tests-deploy-quay-aws-s3-commands.shci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.shci-operator/step-registry/quay-tests/test-quay-e2e/quay-tests-test-quay-e2e-commands.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/config/quay/quay-tests/quay-quay-tests-master__ocp-4.22-quay-lpGA-lp-ocp-compat.yaml
- ci-operator/step-registry/quay-tests/cso-qe-test/quay-tests-cso-qe-test-commands.sh
Comment on lines
+46
to
+51
| ( set +x | ||
| cat >>create_aws_bucket.tf <<EOF | ||
| provider "aws" { | ||
| region = "us-east-2" | ||
| access_key = "${QUAY_AWS_ACCESS_KEY}" | ||
| secret_key = "${QUAY_AWS_SECRET_KEY}" | ||
| access_key = "$(tr -d '\n' < /var/run/quay-qe-aws-secret/access_key)" | ||
| secret_key = "$(tr -d '\n' < /var/run/quay-qe-aws-secret/secret_key)" |
Contributor
There was a problem hiding this comment.
Don't persist AWS credentials in the generated Terraform file.
These lines bake the access key and secret key into create_aws_bucket.tf. The surrounding workspace is later archived into terraform.tgz and copied to ${SHARED_DIR}, so this turns a log-safe read into a cross-step plaintext secret leak.
Suggested direction
-( set +x
-cat >>create_aws_bucket.tf <<EOF
+(
+ set +x
+ export AWS_ACCESS_KEY_ID="$(tr -d '\n' < /var/run/quay-qe-aws-secret/access_key)"
+ export AWS_SECRET_ACCESS_KEY="$(tr -d '\n' < /var/run/quay-qe-aws-secret/secret_key)"
+ cat >>create_aws_bucket.tf <<EOF
provider "aws" {
region = "us-east-2"
- access_key = "$(tr -d '\n' < /var/run/quay-qe-aws-secret/access_key)"
- secret_key = "$(tr -d '\n' < /var/run/quay-qe-aws-secret/secret_key)"
}
EOF
-true )
+ terraform init
+ terraform apply -auto-approve
+)If you want to keep archiving the directory, make sure no file included in terraform.tgz contains the credentials.
🤖 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/quay-tests/deploy-quay-aws-s3/quay-tests-deploy-quay-aws-s3-commands.sh`
around lines 46 - 51, The generated Terraform file currently embeds plaintext
AWS credentials into create_aws_bucket.tf via the provider "aws" block; remove
the access_key and secret_key interpolation so the provider block does not
persist secrets, and instead rely on environment-based credentials
(AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY), an AWS profile, or Terraform
variables supplied at runtime (e.g., via a separate, non-archived tfvars or CI
secret injection). Update the code that writes create_aws_bucket.tf (the
here-doc that creates the provider "aws" block) to omit secret insertion and
ensure any alternative credential mechanism is used before packaging
terraform.tgz so no file in the archive contains credentials.
Comment on lines
+240
to
+256
| ( set +x | ||
| jq -cn \ | ||
| --rawfile username /var/run/quay-qe-quay-secret/username \ | ||
| --rawfile password /var/run/quay-qe-quay-secret/password \ | ||
| --rawfile email /var/run/quay-qe-quay-secret/email \ | ||
| '{ | ||
| username: ($username | rtrimstr("\n")), | ||
| password: ($password | rtrimstr("\n")), | ||
| email: ($email | rtrimstr("\n")), | ||
| access_token: true | ||
| }' | | ||
| curl -fsSk -X POST "${quayRoute}/api/v1/user/initialize" \ | ||
| --header 'Content-Type: application/json' \ | ||
| --data @- | | ||
| jq -r '.access_token' | tr -d '\n' > "${SHARED_DIR}/quay_oauth2_token" | ||
| true | ||
| ) |
Contributor
There was a problem hiding this comment.
Let the loop retry when Quay initialization is transiently unavailable.
Once the Available=True condition appears, any one-off failure from the init pipeline exits the whole script under set -euo pipefail. That makes route/API startup races fail the step and leaves ${SHARED_DIR}/quay_oauth2_token missing for downstream consumers.
Suggested fix
- ( set +x
- jq -cn \
- --rawfile username /var/run/quay-qe-quay-secret/username \
- --rawfile password /var/run/quay-qe-quay-secret/password \
- --rawfile email /var/run/quay-qe-quay-secret/email \
- '{
- username: ($username | rtrimstr("\n")),
- password: ($password | rtrimstr("\n")),
- email: ($email | rtrimstr("\n")),
- access_token: true
- }' |
- curl -fsSk -X POST "${quayRoute}/api/v1/user/initialize" \
- --header 'Content-Type: application/json' \
- --data `@-` |
- jq -r '.access_token' | tr -d '\n' > "${SHARED_DIR}/quay_oauth2_token"
- true
- )
- ArchivePodInfo
- exit 0
+ if (
+ set +x
+ jq -cn \
+ --rawfile username /var/run/quay-qe-quay-secret/username \
+ --rawfile password /var/run/quay-qe-quay-secret/password \
+ --rawfile email /var/run/quay-qe-quay-secret/email \
+ '{
+ username: ($username | rtrimstr("\n")),
+ password: ($password | rtrimstr("\n")),
+ email: ($email | rtrimstr("\n")),
+ access_token: true
+ }' |
+ curl -fsSk -X POST "${quayRoute}/api/v1/user/initialize" \
+ --header 'Content-Type: application/json' \
+ --data `@-` |
+ jq -er '.access_token' | tr -d '\n' > "${SHARED_DIR}/quay_oauth2_token"
+ ); then
+ ArchivePodInfo
+ exit 0
+ fiThat keeps the existing wait loop effective instead of turning the first failed init attempt into a hard step failure.
📝 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.
Suggested change
| ( set +x | |
| jq -cn \ | |
| --rawfile username /var/run/quay-qe-quay-secret/username \ | |
| --rawfile password /var/run/quay-qe-quay-secret/password \ | |
| --rawfile email /var/run/quay-qe-quay-secret/email \ | |
| '{ | |
| username: ($username | rtrimstr("\n")), | |
| password: ($password | rtrimstr("\n")), | |
| email: ($email | rtrimstr("\n")), | |
| access_token: true | |
| }' | | |
| curl -fsSk -X POST "${quayRoute}/api/v1/user/initialize" \ | |
| --header 'Content-Type: application/json' \ | |
| --data @- | | |
| jq -r '.access_token' | tr -d '\n' > "${SHARED_DIR}/quay_oauth2_token" | |
| true | |
| ) | |
| if ( | |
| set +x | |
| jq -cn \ | |
| --rawfile username /var/run/quay-qe-quay-secret/username \ | |
| --rawfile password /var/run/quay-qe-quay-secret/password \ | |
| --rawfile email /var/run/quay-qe-quay-secret/email \ | |
| '{ | |
| username: ($username | rtrimstr("\n")), | |
| password: ($password | rtrimstr("\n")), | |
| email: ($email | rtrimstr("\n")), | |
| access_token: true | |
| }' | | |
| curl -fsSk -X POST "${quayRoute}/api/v1/user/initialize" \ | |
| --header 'Content-Type: application/json' \ | |
| --data `@-` | | |
| jq -er '.access_token' | tr -d '\n' > "${SHARED_DIR}/quay_oauth2_token" | |
| ); then | |
| ArchivePodInfo | |
| exit 0 | |
| 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/quay-tests/deploy-quay-aws-s3/quay-tests-deploy-quay-aws-s3-commands.sh`
around lines 240 - 256, The current subshell pipeline that posts to
"${quayRoute}/api/v1/user/initialize" and writes jq -r '.access_token' to
"${SHARED_DIR}/quay_oauth2_token" can fail the whole script on a transient
API/route race; wrap that pipeline (the curl + jq token extraction) in a retry
loop (e.g., until/for with sleep and a reasonable backoff/retry count) and
temporarily disable errexit for the pipeline so individual failures don't abort
the script, retrying until the curl + jq pipeline succeeds and then write the
token to ${SHARED_DIR}/quay_oauth2_token; keep the rest of the surrounding
behavior (set -euo pipefail) intact.
Comment on lines
+48
to
+51
| ( set +x | ||
| tr -dc A-Za-z0-9 </dev/urandom | head -c 40 | tr -d '\n' > "${SHARED_DIR}/quay-access-token" | ||
| true | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -uo pipefail
bash -lc '
set -euo pipefail
tmp=$(mktemp)
trap "rm -f \"$tmp\"" EXIT
(
tr -dc A-Za-z0-9 </dev/urandom | head -c 40 | tr -d "\n" > "$tmp"
echo "after-pipeline"
)
'
rc=$?
printf 'exit=%s\n' "$rc"Repository: openshift/release
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh"
# Show the exact area around the reported lines 35-110 and 40-60 + token generation 45-55
sed -n '1,140p' "$file" | nl -ba
# Show the larger tail area in case readiness loops exist around where prior comments referenced (~520-550)
sed -n '480,620p' "$file" | nl -baRepository: openshift/release
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh"
echo "=== Head/lines ~1-120 ==="
cat -n "$file" | sed -n '1,140p'
echo "=== Token generation area ~35-70 ==="
cat -n "$file" | sed -n '35,80p'
echo "=== Later readiness/polling area ~480-620 ==="
cat -n "$file" | sed -n '480,620p'Repository: openshift/release
Length of output: 10530
Fix token generation pipeline to avoid pipefail/SIGPIPE aborts
- In
ci-operator/step-registry/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.shlines 48-51,set +xdisables tracing but the subshell still inherits the script’sset -euxo pipefail; whenhead -c 40exits early, the upstreamtrcan hit SIGPIPE and cause the pipeline to fail before the trailingtrue, aborting token creation. - The bounded prerequisite waits for QBO subscription installedCSV (lines 35-43) and
quay-appreadiness (lines 96-100) don’t fail explicitly on timeout, so the script can continue and error later in less obvious ways.
Suggested fix
-( set +x
- tr -dc A-Za-z0-9 </dev/urandom | head -c 40 | tr -d '\n' > "${SHARED_DIR}/quay-access-token"
- true
-)
+(
+ set +x
+ set +o pipefail
+ tr -dc A-Za-z0-9 </dev/urandom | head -c 40 | tr -d '\n' > "${SHARED_DIR}/quay-access-token"
+)🤖 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/quay-tests/qbo-qe-test/quay-tests-qbo-qe-test-commands.sh`
around lines 48 - 51, The token-generation subshell that writes
"${SHARED_DIR}/quay-access-token" can fail due to pipefail/SIGPIPE when head
exits early; change the subshell to either disable pipefail inside it (e.g., set
+o pipefail) or replace the pipeline with a single robust command that produces
40 alphanumeric chars without creating a fragile pipe (e.g., use dd/openssl to
read exact bytes then filter), ensuring the final write always succeeds;
additionally, make the bounded prerequisite waits for the QBO installedCSV and
quay-app readiness explicitly fail/exit non-zero on timeout (add explicit
timeout checks and set an error/exit path) so the script stops earlier instead
of continuing to later failures.
Comment on lines
+54
to
+57
| ( set +x | ||
| export HTTPS_PROXY="$(tr -d '\n' < "${SHARED_DIR}/proxy_public_url")" | ||
| export HTTP_PROXY="${HTTPS_PROXY}" | ||
| true ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect the affected blocks:"
sed -n '53,79p' ci-operator/step-registry/quay-tests/test-quay-e2e/quay-tests-test-quay-e2e-commands.sh
echo
echo "Demonstrate bash scoping:"
unset PROXY_DEMO CYPRESS_DEMO || true
( export PROXY_DEMO=1; export CYPRESS_DEMO=1 )
if [[ -n "${PROXY_DEMO:-}" || -n "${CYPRESS_DEMO:-}" ]]; then
echo "unexpected: subshell exports leaked to the parent shell"
exit 1
fi
echo "Confirmed: exports inside (...) do not persist to the parent shell."Repository: openshift/release
Length of output: 1159
Don’t export runtime env (HTTPS_PROXY/HTTP_PROXY, CYPRESS_*) from set +x subshells.
Both wiring blocks use ( set +x ... export ... ), so the exported proxy and Cypress Quay env vars don’t persist to the parent shell; npm run smoke won’t receive them. Disable tracing without subshell-scoping by save/restore xtrace state.
Suggested fix
if [ "${QUAY_PROXY}" = "true" ]; then
- ( set +x
- export HTTPS_PROXY="$(tr -d '\n' < "${SHARED_DIR}/proxy_public_url")"
- export HTTP_PROXY="${HTTPS_PROXY}"
- true )
+ had_xtrace=0
+ if [[ $- == *x* ]]; then
+ had_xtrace=1
+ set +x
+ fi
+ HTTPS_PROXY="$(tr -d '\n' < "${SHARED_DIR}/proxy_public_url")"
+ export HTTPS_PROXY
+ HTTP_PROXY="${HTTPS_PROXY}"
+ export HTTP_PROXY
+ if (( had_xtrace )); then
+ set -x
+ fi
fi
# Trigger Quay E2E testing
-( set +x
- quayHostname="$(
- oc get quayregistry quay -n quay-enterprise -o jsonpath='{.status.registryEndpoint}' 2>/dev/null |
- sed -e 's|^[^/]*//||'
- )"
- if [[ -z "${quayHostname}" ]]; then
- echo 'Quay registry endpoint not found.' 1>&2
- exit 1
- fi
- if [ "$(printf '%s\n%s' "${quayVersionThreshold}" "${QUAY_VERSION}" | sort -V | head -n1)" = "${quayVersionThreshold}" ]; then
- export CYPRESS_QUAY_ENDPOINT="${quayHostname}"
- export CYPRESS_QUAY_ENDPOINT_PROTOCOL="https"
- export CYPRESS_QUAY_PROJECT="quay-enterprise"
- export CYPRESS_OLD_UI_DISABLED=true
- else
- export CYPRESS_QUAY_ENDPOINT="${quayHostname}"
- export CYPRESS_QUAY_VERSION="${QUAY_VERSION}"
- fi
-true )
+had_xtrace=0
+if [[ $- == *x* ]]; then
+ had_xtrace=1
+ set +x
+fi
+quayHostname="$(
+ oc get quayregistry quay -n quay-enterprise -o jsonpath='{.status.registryEndpoint}' 2>/dev/null |
+ sed -e 's|^[^/]*//||'
+)"
+if [[ -z "${quayHostname}" ]]; then
+ echo 'Quay registry endpoint not found.' 1>&2
+ exit 1
+fi
+if [ "$(printf '%s\n%s' "${quayVersionThreshold}" "${QUAY_VERSION}" | sort -V | head -n1)" = "${quayVersionThreshold}" ]; then
+ export CYPRESS_QUAY_ENDPOINT="${quayHostname}"
+ export CYPRESS_QUAY_ENDPOINT_PROTOCOL="https"
+ export CYPRESS_QUAY_PROJECT="quay-enterprise"
+ export CYPRESS_OLD_UI_DISABLED=true
+else
+ export CYPRESS_QUAY_ENDPOINT="${quayHostname}"
+ export CYPRESS_QUAY_VERSION="${QUAY_VERSION}"
+fi
+if (( had_xtrace )); then
+ set -x
+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.
Suggested change
| ( set +x | |
| export HTTPS_PROXY="$(tr -d '\n' < "${SHARED_DIR}/proxy_public_url")" | |
| export HTTP_PROXY="${HTTPS_PROXY}" | |
| true ) | |
| if [ "${QUAY_PROXY}" = "true" ]; then | |
| had_xtrace=0 | |
| if [[ $- == *x* ]]; then | |
| had_xtrace=1 | |
| set +x | |
| fi | |
| HTTPS_PROXY="$(tr -d '\n' < "${SHARED_DIR}/proxy_public_url")" | |
| export HTTPS_PROXY | |
| HTTP_PROXY="${HTTPS_PROXY}" | |
| export HTTP_PROXY | |
| if (( had_xtrace )); then | |
| set -x | |
| fi | |
| fi | |
| # Trigger Quay E2E testing | |
| had_xtrace=0 | |
| if [[ $- == *x* ]]; then | |
| had_xtrace=1 | |
| set +x | |
| fi | |
| quayHostname="$( | |
| oc get quayregistry quay -n quay-enterprise -o jsonpath='{.status.registryEndpoint}' 2>/dev/null | | |
| sed -e 's|^[^/]*//||' | |
| )" | |
| if [[ -z "${quayHostname}" ]]; then | |
| echo 'Quay registry endpoint not found.' 1>&2 | |
| exit 1 | |
| fi | |
| if [ "$(printf '%s\n%s' "${quayVersionThreshold}" "${QUAY_VERSION}" | sort -V | head -n1)" = "${quayVersionThreshold}" ]; then | |
| export CYPRESS_QUAY_ENDPOINT="${quayHostname}" | |
| export CYPRESS_QUAY_ENDPOINT_PROTOCOL="https" | |
| export CYPRESS_QUAY_PROJECT="quay-enterprise" | |
| export CYPRESS_OLD_UI_DISABLED=true | |
| else | |
| export CYPRESS_QUAY_ENDPOINT="${quayHostname}" | |
| export CYPRESS_QUAY_VERSION="${QUAY_VERSION}" | |
| fi | |
| if (( had_xtrace )); then | |
| set -x | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 55-55: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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/quay-tests/test-quay-e2e/quay-tests-test-quay-e2e-commands.sh`
around lines 54 - 57, The env exports for HTTPS_PROXY/HTTP_PROXY and CYPRESS_*
are being done inside subshells with "( set +x ... )" so they do not persist to
the parent shell and thus npm run smoke won't see them; change each block to
avoid subshell-scoping by removing the surrounding parentheses, instead save the
current xtrace state, disable tracing (set +x), perform the export of
HTTPS_PROXY, HTTP_PROXY, and the CYPRESS_* variables (use the same identifiers
shown), then restore the saved xtrace state so the exports persist for later
commands like npm run smoke.
etirta
suggested changes
Jun 15, 2026
etirta
left a comment
etirta
left a comment
Contributor
There was a problem hiding this comment.
We need to reduce the unnecessary back and forth on the review. Please make sure you direct your AI Agent to follow our established Best Practices every time.
Comment on lines
58
to
97
| @@ -100,12 +96,7 @@ spec: | |||
| maxSurge: 25% | |||
| EOF | |||
Contributor
There was a problem hiding this comment.
Please follow our Best Practices for OCP Resource Creation if data marshalling is needed.
Suggested change
| { | |
| oc create -f - --dry-run=client -o json --save-config | | |
| jq -c \ | |
| --arg ns "${testNs}" \ | |
| ' | |
| .metadata.namespace=$ns | |
| ' | | |
| yq -p json -o yaml eval . | |
| } 0<<'ocEOF' | oc apply -f - | |
| apiVersion: apps/v1 | |
| kind: Deployment | |
| metadata: | |
| name: nodejs-sample | |
| namespace: '' | |
| labels: | |
| app: nodejs-sample | |
| spec: | |
| maxSurge: 25% | |
| ocEOF |
Comment on lines
46
to
+73
| @@ -78,7 +70,7 @@ | |||
| acl = "private" | |||
| } | |||
| EOF | |||
| set -x | |||
| true ) | |||
Contributor
There was a problem hiding this comment.
Please consider to limit the turning off the xtrace to minimal context as possible, to help post-mortem analysis, if needed.
Since cat do not print to STDOUT nor STDERR, in this case, we can limit the xtrace disablement as needed:
$ (set -x; cat 0<<EOF 1> /dev/null
abc=$(set +x; echo $'a\nb' tr -d '\n')
EOF
)
+ cat
++ set +x
Suggested change
| cat 0<<tfEOF 1>create_aws_bucket.tf | |
| provider "aws" { | |
| region = "us-east-2" | |
| access_key = "$(set +x; tr -d '\n' < /var/run/quay-qe-aws-secret/access_key)" | |
| secret_key = "$(set +x; tr -d '\n' < /var/run/quay-qe-aws-secret/secret_key)" | |
| } | |
| resource "aws_s3_bucket" "quayaws" { | |
| bucket = var.aws_bucket | |
| force_destroy = true | |
| } | |
| resource "aws_s3_bucket_ownership_controls" "quayaws" { | |
| bucket = aws_s3_bucket.quayaws.id | |
| rule { | |
| object_ownership = "BucketOwnerPreferred" | |
| } | |
| } | |
| resource "aws_s3_bucket_acl" "quayaws_bucket_acl" { | |
| depends_on = [aws_s3_bucket_ownership_controls.quayaws] | |
| bucket = aws_s3_bucket.quayaws.id | |
| acl = "private" | |
| } | |
| tfEOF |
Comment on lines
85
to
90
| cat <<EOF | oc apply -f - | ||
| apiVersion: v1 | ||
| kind: Namespace | ||
| metadata: | ||
| name: quay-enterprise | ||
| EOF |
Contributor
There was a problem hiding this comment.
Please follow our Best Practices for OCP Resource creation without data marshalling in an idempotent way.
Suggested change
| oc create Namespace quay-enterprise --dry-run=client -o yaml --save-config | oc apply -f - |
Do the same in all other OCP Resource creations.
Comment on lines
+240
to
+256
| ( set +x | ||
| jq -cn \ | ||
| --rawfile username /var/run/quay-qe-quay-secret/username \ | ||
| --rawfile password /var/run/quay-qe-quay-secret/password \ | ||
| --rawfile email /var/run/quay-qe-quay-secret/email \ | ||
| '{ | ||
| username: ($username | rtrimstr("\n")), | ||
| password: ($password | rtrimstr("\n")), | ||
| email: ($email | rtrimstr("\n")), | ||
| access_token: true | ||
| }' | | ||
| curl -fsSk -X POST "${quayRoute}/api/v1/user/initialize" \ | ||
| --header 'Content-Type: application/json' \ | ||
| --data @- | | ||
| jq -r '.access_token' | tr -d '\n' > "${SHARED_DIR}/quay_oauth2_token" | ||
| true | ||
| ) |
Contributor
There was a problem hiding this comment.
There is no need to wrap this in a sub-shell. Please follow our Best Practices.
| jq -er '.items[-1] | "\(.metadata.namespace) \(.status.registryEndpoint // empty)"' | ||
| ) || { echo 'No QuayRegistry found.' 1>&2; exit 1; } | ||
| if [[ -z "${registryEndpoint}" ]]; then | ||
| echo 'QuayRegistry has no registryEndpoint yet.' 1>&2 |
Contributor
There was a problem hiding this comment.
Please follow our Best Practices, no echo when xtrace is active. You got to change your paradigm and get used to with writing proper shell code with xtrace nad errexit active. All this if just bloating the code.
With xtrace and errexit active, the following code is enough.
[ -n "${registryEndpoint}" ]
Comment on lines
+48
to
+51
| ( set +x | ||
| tr -dc A-Za-z0-9 </dev/urandom | head -c 40 | tr -d '\n' > "${SHARED_DIR}/quay-access-token" | ||
| true | ||
| ) |
Contributor
There was a problem hiding this comment.
There is no need to wrap with a subshell. You need to master when to wrap and when not to wrap.
| -o jsonpath='{.items[0].metadata.name}' 2>/dev/null | ||
| )" ]] || { echo 'No quay-app pod found.' 1>&2; exit 1; } | ||
|
|
||
| ( set +x |
Contributor
There was a problem hiding this comment.
What are we protecting with subshell in here?
| operator_org = model.organization.create_organization("quay-bridge-operator", "quay-bridge-operator@localhost.local", admin_user) | ||
| operator_app = model.oauth.create_application(operator_org.id, "quay-bridge-operator", "", "") | ||
| create_user_access_token(admin_user, operator_app.client_id, scope, access_token="$token") | ||
| create_user_access_token(admin_user, operator_app.client_id, scope, access_token="$(tr -d '\n' < "${SHARED_DIR}/quay-access-token")") |
Contributor
There was a problem hiding this comment.
Instead we can limit the xtrace disablement only in this subshell.
Comment on lines
+54
to
+57
| ( set +x | ||
| export HTTPS_PROXY="$(tr -d '\n' < "${SHARED_DIR}/proxy_public_url")" | ||
| export HTTP_PROXY="${HTTPS_PROXY}" | ||
| true ) |
Contributor
There was a problem hiding this comment.
This is dead code. You are exporting in a subshell.
| fi | ||
| set -x | ||
| # Trigger Quay E2E testing | ||
| ( set +x |
Contributor
There was a problem hiding this comment.
Ditto with exporting in subshell. The recommendation is doing inline export that scope it to child only.
ENV_VAR_1=... \
ENV_VAR_2=.... \
some-command
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates Quay OCP 4.22 LP interop CI to the
lpGA-lp-ocp-compatvariant and wires MPIIT / Component Readiness junit remapping through the sharedExitTrap--PostProcessPrephelper.This replaces the legacy
ocp-4.22-quay-lp-interopvariant and per-stepREPORTPORTAL_CMP/mapTestsForComponentReadinesshandling.Changes
CI operator config (
quay-quay-tests-master)__ocp-4.22-quay-lp-interop.yaml→__ocp-4.22-quay-lpGA-lp-ocp-compat.yamlzz_generated_metadata.varianttoocp-4.22-quay-lpGA-lp-ocp-compatcr-quay-e2e-tests-aws→cr--quay--e2e-tests-awsaws-fips→quay--aws-fipsDR__RP__CR_COMP_NAME: lp-ocp-compat--Quayon CR, FIPS, and yearly AWS S3 jobsREPORTPORTAL_CMP: Quay-lp-interop(superseded byDR__RP__CR_COMP_NAME+ ExitTrap)Step registry (CR job
test:steps)For
quay-tests-deploy-quay-aws-s3,quay-tests-test-quay-e2e,quay-tests-qbo-qe-test, andquay-tests-cso-qe-test:DR__RP__CR_COMP_NAME(defaultlp-ocp-compat--Quay) andMAP_TESTS(defaultfalse) to each-ref.yamlMAP_TESTS=true, source ExitTrap--PostProcessPrep and register anEXITtrap to remap junit suite names tolp-ocp-compat--Quay--<test>mapTestsForComponentReadinessin e2e; hard-coded ReportPortal component names elsewhere)set -euxo pipefail+inherit_errexit, typed variables,set +xaround secrets, dry-run/apply for secrets, improved wait loopsquay-tests-test-quay-e2ekeeps artifact copy onEXITand chains ExitTrap +CopyArtifactswhen mapping is enabled.Summary by CodeRabbit
This PR updates Quay CI configuration and step scripts in the OpenShift CI repo to migrate Quay testing to a new lpGA variant and to centralize JUnit remapping via a shared ExitTrap helper.
What changed, in practical terms:
CI variant and job metadata
ExitTrap-based JUnit remapping
Shell/script hardening and refactors
Impact and scope:
New/modified env variables (summary):
Files/classes primarily affected: