SREP-4417: Add operator e2e Prow workflow with AVO as PoC#77989
Open
dustman9000 wants to merge 3 commits intoopenshift:mainfrom
Open
SREP-4417: Add operator e2e Prow workflow with AVO as PoC#77989dustman9000 wants to merge 3 commits intoopenshift:mainfrom
dustman9000 wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@dustman9000: This pull request references SREP-4417 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
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:
WalkthroughAdds CI step definitions, executable install/e2e scripts, workflow and step metadata, ownership files, TestGrid allow-list entry, and presubmit/postsubmit job and config updates to run ROSA operator E2E (provision → install → test → gather → deprovision). Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Controller
participant Prov as rosa-aws-sts-provision
participant Cluster as ROSA Cluster
participant Installer as rosa-operator-install
participant Test as rosa-operator-e2e (/usr/local/bin/e2e.test)
participant Gather as osd-gather-extra
participant Deprov as rosa-aws-sts-deprovision
CI->>Prov: start provision (pre)
Prov->>Cluster: create STS cluster
Prov-->>CI: return cluster info
CI->>Cluster: wait-ready-nodes
CI->>Installer: install operator (pre)
Installer->>Cluster: apply namespace/CRDs/ClusterPackage
Installer-->>CI: operator Available
CI->>Test: run e2e (test)
Test-->>CI: test results / JUnit
CI->>Gather: collect artifacts (post)
Gather-->>CI: artifacts stored
CI->>Deprov: deprovision cluster (post)
Deprov->>Cluster: delete resources
Deprov-->>CI: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dustman9000 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 |
Contributor
|
@dustman9000: This pull request references SREP-4417 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![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: 2
🧹 Nitpick comments (2)
ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh (1)
64-68: Make deployment wait target configurable for broader reuse.The fixed
deployment "${OPERATOR_NAME}"assumption can be brittle for other operators. Consider allowing an override (e.g.,OPERATOR_DEPLOYMENT_NAME) to keep this step reusable.♻️ Suggested change
+OPERATOR_DEPLOYMENT_NAME="${OPERATOR_DEPLOYMENT_NAME:-${OPERATOR_NAME}}" + log "Waiting for ${OPERATOR_NAME} deployment to be ready..." -oc wait deployment "${OPERATOR_NAME}" \ +oc wait deployment "${OPERATOR_DEPLOYMENT_NAME}" \ -n "${OPERATOR_NAMESPACE}" \ --for=condition=Available \ --timeout=300s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh` around lines 64 - 68, The wait currently hardcodes the deployment target as deployment "${OPERATOR_NAME}"; make the target configurable by introducing and using an override variable (e.g., OPERATOR_DEPLOYMENT_NAME) so callers can specify a different deployment name. Change the oc wait invocation to use "${OPERATOR_DEPLOYMENT_NAME:-${OPERATOR_NAME}}" (or equivalent) as the deployment argument and document/validate OPERATOR_DEPLOYMENT_NAME usage near OPERATOR_NAME so the step remains reusable across operators.ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main__periodics.yaml (1)
28-29: Prefer digest-pinned operator images for periodic stability.Using mutable
:latesttags can cause hard-to-triage drift/flakes in a daily signal. Pinning by digest will make failures more attributable.♻️ Suggested change
- OPERATOR_PKO_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko:latest" - OPERATOR_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator:latest" + OPERATOR_PKO_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko@sha256:<digest>" + OPERATOR_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator@sha256:<digest>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main__periodics.yaml` around lines 28 - 29, The OPERATOR_PKO_IMAGE and OPERATOR_IMAGE environment values currently use mutable :latest tags; replace both "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko:latest" and "quay.io/redhat-services-prod/openshift/aws-vpce-operator:latest" with digest-pinned references (quay.io/...@sha256:<digest>) so the periodic job uses immutable images; obtain the correct sha256 digests from the registry (e.g., quay/skopeo/docker manifest inspection) and update OPERATOR_PKO_IMAGE and OPERATOR_IMAGE accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-periodics.yaml`:
- Around line 46-48: The PodSpec mounts a volume named "gcs-credentials"
(mountPath: /secrets/gcs) but no corresponding volume is defined under
spec.volumes; add a volume entry named "gcs-credentials" (e.g., a secret or
projected volume depending on how GCS credentials are provided) to the job's
spec.volumes so the mount has a valid source and the PodSpec becomes valid,
ensuring the name exactly matches "gcs-credentials" and the secret/secretKeyRef
or relevant volume type contains the GCS credentials.
In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh`:
- Around line 53-60: The test invocation currently expands unquoted GINKGO_FLAGS
which can split arguments incorrectly; change the script to build flags as an
array (e.g., GINKGO_FLAGS_ARRAY) and push each flag element (including the
label-filter when set) into that array, then invoke /usr/local/bin/e2e.test with
the array expanded as "${GINKGO_FLAGS_ARRAY[@]}" instead of unquoted
${GINKGO_FLAGS}; update references to GINKGO_FLAGS in the surrounding code to
use the array variable so arguments with spaces are preserved.
---
Nitpick comments:
In
`@ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main__periodics.yaml`:
- Around line 28-29: The OPERATOR_PKO_IMAGE and OPERATOR_IMAGE environment
values currently use mutable :latest tags; replace both
"quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko:latest" and
"quay.io/redhat-services-prod/openshift/aws-vpce-operator:latest" with
digest-pinned references (quay.io/...@sha256:<digest>) so the periodic job uses
immutable images; obtain the correct sha256 digests from the registry (e.g.,
quay/skopeo/docker manifest inspection) and update OPERATOR_PKO_IMAGE and
OPERATOR_IMAGE accordingly.
In
`@ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh`:
- Around line 64-68: The wait currently hardcodes the deployment target as
deployment "${OPERATOR_NAME}"; make the target configurable by introducing and
using an override variable (e.g., OPERATOR_DEPLOYMENT_NAME) so callers can
specify a different deployment name. Change the oc wait invocation to use
"${OPERATOR_DEPLOYMENT_NAME:-${OPERATOR_NAME}}" (or equivalent) as the
deployment argument and document/validate OPERATOR_DEPLOYMENT_NAME usage near
OPERATOR_NAME so the step remains reusable across operators.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae495b78-7488-4efc-b1ae-650d044d3f6c
📒 Files selected for processing (15)
ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main__periodics.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-periodics.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-presubmits.yamlci-operator/step-registry/rosa/operator/e2e-workflow/OWNERSci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.jsonci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yamlci-operator/step-registry/rosa/operator/e2e/OWNERSci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.shci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.jsonci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yamlci-operator/step-registry/rosa/operator/install/OWNERSci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.shci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.jsonci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yamlcore-services/testgrid-config-generator/_allow-list.yaml
Comment on lines
+46
to
+48
| - mountPath: /secrets/gcs | ||
| name: gcs-credentials | ||
| readOnly: true |
Contributor
There was a problem hiding this comment.
Missing gcs-credentials volume makes the PodSpec invalid.
At Line 46, the container mounts name: gcs-credentials, but no matching volume is defined under spec.volumes (Lines 59-74). This prevents the periodic job from starting.
🔧 Proposed fix
volumes:
- name: boskos
secret:
items:
- key: credentials
path: credentials
secretName: boskos-credentials
+ - name: gcs-credentials
+ secret:
+ secretName: gcs-credentials
- name: manifest-tool-local-pusher
secret:
secretName: manifest-tool-local-pusherAlso applies to: 59-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-periodics.yaml`
around lines 46 - 48, The PodSpec mounts a volume named "gcs-credentials"
(mountPath: /secrets/gcs) but no corresponding volume is defined under
spec.volumes; add a volume entry named "gcs-credentials" (e.g., a secret or
projected volume depending on how GCS credentials are provided) to the job's
spec.volumes so the mount has a valid source and the PodSpec becomes valid,
ensuring the name exactly matches "gcs-credentials" and the secret/secretKeyRef
or relevant volume type contains the GCS credentials.
Comment on lines
+53
to
+60
| GINKGO_FLAGS="--ginkgo.junit-report=${JUNIT_REPORT} --ginkgo.v" | ||
|
|
||
| if [[ -n "${GINKGO_LABEL_FILTER:-}" ]]; then | ||
| GINKGO_FLAGS="${GINKGO_FLAGS} --ginkgo.label-filter=${GINKGO_LABEL_FILTER}" | ||
| fi | ||
|
|
||
| log "Running ${OPERATOR_NAME} e2e tests..." | ||
| /usr/local/bin/e2e.test ${GINKGO_FLAGS} || { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: confirm risky expansion exists.
rg -n 'GINKGO_FLAGS=|GINKGO_FLAGS\+=\"|e2e\.test \$\{GINKGO_FLAGS\}' ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.shRepository: openshift/release
Length of output: 216
🏁 Script executed:
cat -n ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh | sed -n '50,65p'Repository: openshift/release
Length of output: 669
🏁 Script executed:
rg -n 'GINKGO_FLAGS' ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.shRepository: openshift/release
Length of output: 258
Fix argument splitting in test invocation.
At line 60, unquoted ${GINKGO_FLAGS} can split arguments incorrectly when GINKGO_LABEL_FILTER contains spaces. Build flags as an array and expand with "${...[@]}".
Proposed fix
JUNIT_REPORT="${ARTIFACT_DIR}/junit-${OPERATOR_NAME}-e2e.xml"
-GINKGO_FLAGS="--ginkgo.junit-report=${JUNIT_REPORT} --ginkgo.v"
+GINKGO_FLAGS=("--ginkgo.junit-report=${JUNIT_REPORT}" "--ginkgo.v")
if [[ -n "${GINKGO_LABEL_FILTER:-}" ]]; then
- GINKGO_FLAGS="${GINKGO_FLAGS} --ginkgo.label-filter=${GINKGO_LABEL_FILTER}"
+ GINKGO_FLAGS+=("--ginkgo.label-filter=${GINKGO_LABEL_FILTER}")
fi
log "Running ${OPERATOR_NAME} e2e tests..."
-/usr/local/bin/e2e.test ${GINKGO_FLAGS} || {
+/usr/local/bin/e2e.test "${GINKGO_FLAGS[@]}" || {🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 60-60: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh`
around lines 53 - 60, The test invocation currently expands unquoted
GINKGO_FLAGS which can split arguments incorrectly; change the script to build
flags as an array (e.g., GINKGO_FLAGS_ARRAY) and push each flag element
(including the label-filter when set) into that array, then invoke
/usr/local/bin/e2e.test with the array expanded as "${GINKGO_FLAGS_ARRAY[@]}"
instead of unquoted ${GINKGO_FLAGS}; update references to GINKGO_FLAGS in the
surrounding code to use the array variable so arguments with spaces are
preserved.
dustman9000
force-pushed
the
srep-4417/avo-operator-e2e-poc
branch
from
bef98bb to
b4bac7c
Compare
Contributor
|
@dustman9000: This pull request references SREP-4417 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh (1)
53-60:⚠️ Potential issue | 🟠 MajorFix unsafe flag expansion in test invocation.
At Line 60, unquoted
${GINKGO_FLAGS}can split/glob unexpectedly (especially whenGINKGO_LABEL_FILTERincludes spaces). Build flags as an array and execute with"${...[@]}".🔧 Suggested fix
-JUNIT_REPORT="${ARTIFACT_DIR}/junit-${OPERATOR_NAME}-e2e.xml" -GINKGO_FLAGS="--ginkgo.junit-report=${JUNIT_REPORT} --ginkgo.v" +JUNIT_REPORT="${ARTIFACT_DIR}/junit-${OPERATOR_NAME}-e2e.xml" +GINKGO_FLAGS=("--ginkgo.junit-report=${JUNIT_REPORT}" "--ginkgo.v") if [[ -n "${GINKGO_LABEL_FILTER:-}" ]]; then - GINKGO_FLAGS="${GINKGO_FLAGS} --ginkgo.label-filter=${GINKGO_LABEL_FILTER}" + GINKGO_FLAGS+=("--ginkgo.label-filter=${GINKGO_LABEL_FILTER}") fi log "Running ${OPERATOR_NAME} e2e tests..." -/usr/local/bin/e2e.test ${GINKGO_FLAGS} || { +/usr/local/bin/e2e.test "${GINKGO_FLAGS[@]}" || { log "Tests failed. JUnit report at ${JUNIT_REPORT}" exit 1 }#!/bin/bash # Read-only verification: confirm unquoted expansion and string-based flag construction. rg -n 'GINKGO_FLAGS=|GINKGO_FLAGS\+=\"|e2e\.test \$\{GINKGO_FLAGS\}' ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh sed -n '52,61p' ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh` around lines 53 - 60, The test invocation expands GINKGO_FLAGS unsafely; change GINKGO_FLAGS from a string to an array (e.g., GINKGO_FLAGS=( "--ginkgo.junit-report=${JUNIT_REPORT}" "--ginkgo.v" )), append the label-filter element conditionally with GINKGO_FLAGS+=( "--ginkgo.label-filter=${GINKGO_LABEL_FILTER}" ) when non-empty, and call the test binary with the array expansion "/usr/local/bin/e2e.test" "${GINKGO_FLAGS[@]}" to avoid word-splitting/globbing issues; ensure any other uses of GINKGO_FLAGS are updated to the array form.
🧹 Nitpick comments (1)
ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yaml (1)
66-67: Prefer digest-pinned operator images over mutable:latest.Line 66 and Line 67 use
:latest, which makes postsubmit behavior drift over time and can introduce non-reproducible failures unrelated to the merged commit.🔧 Suggested change
- OPERATOR_PKO_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko:latest" - OPERATOR_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator:latest" + OPERATOR_PKO_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko@sha256:<pko-digest>" + OPERATOR_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator@sha256:<operator-digest>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yaml` around lines 66 - 67, The two environment variables OPERATOR_PKO_IMAGE and OPERATOR_IMAGE are pinned to the mutable :latest tag; replace them with digest-pinned image references (quay image@sha256:<digest>) to ensure reproducible postsubmit runs. Update OPERATOR_PKO_IMAGE and OPERATOR_IMAGE to the specific quay image digests for the desired builds (or switch to a parameter/CI variable that resolves to a specific digest), and verify the digests pull successfully in CI; ensure both entries use the exact same digest format so they remain immutable across runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh`:
- Around line 53-60: The test invocation expands GINKGO_FLAGS unsafely; change
GINKGO_FLAGS from a string to an array (e.g., GINKGO_FLAGS=(
"--ginkgo.junit-report=${JUNIT_REPORT}" "--ginkgo.v" )), append the label-filter
element conditionally with GINKGO_FLAGS+=(
"--ginkgo.label-filter=${GINKGO_LABEL_FILTER}" ) when non-empty, and call the
test binary with the array expansion "/usr/local/bin/e2e.test"
"${GINKGO_FLAGS[@]}" to avoid word-splitting/globbing issues; ensure any other
uses of GINKGO_FLAGS are updated to the array form.
---
Nitpick comments:
In
`@ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yaml`:
- Around line 66-67: The two environment variables OPERATOR_PKO_IMAGE and
OPERATOR_IMAGE are pinned to the mutable :latest tag; replace them with
digest-pinned image references (quay image@sha256:<digest>) to ensure
reproducible postsubmit runs. Update OPERATOR_PKO_IMAGE and OPERATOR_IMAGE to
the specific quay image digests for the desired builds (or switch to a
parameter/CI variable that resolves to a specific digest), and verify the
digests pull successfully in CI; ensure both entries use the exact same digest
format so they remain immutable across runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57edfe8d-1957-4fe5-8d2f-ffea302dc7b9
📒 Files selected for processing (14)
ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-postsubmits.yamlci-operator/step-registry/rosa/operator/e2e-workflow/OWNERSci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.jsonci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yamlci-operator/step-registry/rosa/operator/e2e/OWNERSci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.shci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.jsonci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yamlci-operator/step-registry/rosa/operator/install/OWNERSci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.shci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.jsonci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yamlcore-services/testgrid-config-generator/_allow-list.yaml
✅ Files skipped from review due to trivial changes (10)
- ci-operator/step-registry/rosa/operator/e2e/OWNERS
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.json
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.json
- ci-operator/step-registry/rosa/operator/e2e-workflow/OWNERS
- ci-operator/step-registry/rosa/operator/install/OWNERS
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.json
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yaml
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yaml
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yaml
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- core-services/testgrid-config-generator/_allow-list.yaml
dustman9000
force-pushed
the
srep-4417/avo-operator-e2e-poc
branch
from
b4bac7c to
cf025df
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh (1)
53-60:⚠️ Potential issue | 🟠 MajorPrevent flag splitting in test invocation (still unresolved).
At Line 60,
${GINKGO_FLAGS}is unquoted and can split--ginkgo.label-filterexpressions containing spaces. Build flags as an array and pass with"${...[@]}".Proposed fix
JUNIT_REPORT="${ARTIFACT_DIR}/junit-${OPERATOR_NAME}-e2e.xml" -GINKGO_FLAGS="--ginkgo.junit-report=${JUNIT_REPORT} --ginkgo.v" +GINKGO_FLAGS=("--ginkgo.junit-report=${JUNIT_REPORT}" "--ginkgo.v") if [[ -n "${GINKGO_LABEL_FILTER:-}" ]]; then - GINKGO_FLAGS="${GINKGO_FLAGS} --ginkgo.label-filter=${GINKGO_LABEL_FILTER}" + GINKGO_FLAGS+=("--ginkgo.label-filter=${GINKGO_LABEL_FILTER}") fi log "Running ${OPERATOR_NAME} e2e tests..." -/usr/local/bin/e2e.test ${GINKGO_FLAGS} || { +/usr/local/bin/e2e.test "${GINKGO_FLAGS[@]}" || {#!/bin/bash set -euo pipefail file="ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh" echo "[check] unresolved unquoted scalar expansion:" rg -n 'e2e\.test\s+\$\{GINKGO_FLAGS\}' "$file" && { echo "FAIL: unquoted scalar expansion remains." exit 1 } || true echo "[check] array-based invocation present:" rg -n 'e2e\.test\s+"\$\{[A-Z_]+\[@\]\}"' "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh` around lines 53 - 60, The test invocation uses an unquoted scalar GINKGO_FLAGS which can split flags containing spaces; change the script to build GINKGO_FLAGS as a bash array (e.g., GINKGO_FLAGS=() and append elements) and invoke the test with the array expansion quoted as "${GINKGO_FLAGS[@]}", updating the variable references around the e2e.test call and any places that append/modify GINKGO_FLAGS so they push into the array rather than concat strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh`:
- Around line 53-60: The test invocation uses an unquoted scalar GINKGO_FLAGS
which can split flags containing spaces; change the script to build GINKGO_FLAGS
as a bash array (e.g., GINKGO_FLAGS=() and append elements) and invoke the test
with the array expansion quoted as "${GINKGO_FLAGS[@]}", updating the variable
references around the e2e.test call and any places that append/modify
GINKGO_FLAGS so they push into the array rather than concat strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd798765-9c6a-46a1-97af-fa7e4aa6f2fe
📒 Files selected for processing (15)
ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-postsubmits.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-presubmits.yamlci-operator/step-registry/rosa/operator/e2e-workflow/OWNERSci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.jsonci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yamlci-operator/step-registry/rosa/operator/e2e/OWNERSci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.shci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.jsonci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yamlci-operator/step-registry/rosa/operator/install/OWNERSci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.shci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.jsonci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yamlcore-services/testgrid-config-generator/_allow-list.yaml
✅ Files skipped from review due to trivial changes (11)
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.json
- ci-operator/step-registry/rosa/operator/e2e/OWNERS
- ci-operator/step-registry/rosa/operator/e2e-workflow/OWNERS
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.json
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.json
- core-services/testgrid-config-generator/_allow-list.yaml
- ci-operator/step-registry/rosa/operator/install/OWNERS
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yaml
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yaml
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yaml
- ci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-postsubmits.yaml
dustman9000
force-pushed
the
srep-4417/avo-operator-e2e-poc
branch
from
cf025df to
f28d044
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh (1)
53-60:⚠️ Potential issue | 🟠 MajorUse an argument array for Ginkgo flags to prevent split/glob bugs.
At Line 60,
${GINKGO_FLAGS}is expanded unquoted after being built as a string. This can break--ginkgo.label-filtervalues containing spaces/special characters.Proposed fix
JUNIT_REPORT="${ARTIFACT_DIR}/junit-${OPERATOR_NAME}-e2e.xml" -GINKGO_FLAGS="--ginkgo.junit-report=${JUNIT_REPORT} --ginkgo.v" +GINKGO_FLAGS_ARRAY=("--ginkgo.junit-report=${JUNIT_REPORT}" "--ginkgo.v") if [[ -n "${GINKGO_LABEL_FILTER:-}" ]]; then - GINKGO_FLAGS="${GINKGO_FLAGS} --ginkgo.label-filter=${GINKGO_LABEL_FILTER}" + GINKGO_FLAGS_ARRAY+=("--ginkgo.label-filter=${GINKGO_LABEL_FILTER}") fi log "Running ${OPERATOR_NAME} e2e tests..." -/usr/local/bin/e2e.test ${GINKGO_FLAGS} || { +/usr/local/bin/e2e.test "${GINKGO_FLAGS_ARRAY[@]}" || { log "Tests failed. JUnit report at ${JUNIT_REPORT}" exit 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh` around lines 53 - 60, GINKGO_FLAGS is built as a single string and then expanded unquoted into the /usr/local/bin/e2e.test command which can break when values contain spaces or glob characters; change to an argument array (e.g., GINKGO_ARGS or GINKGO_FLAGS_ARRAY) where you append each flag as a separate element (first push "--ginkgo.junit-report=${JUNIT_REPORT}" and "--ginkgo.v", and conditionally push "--ginkgo.label-filter=${GINKGO_LABEL_FILTER}" when set), then invoke /usr/local/bin/e2e.test with the array expansion form so each flag is passed as its own argument to avoid splitting/globbing issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yaml`:
- Around line 66-67: The presubmit/postsubmit jobs are using mutable :latest
image tags for OPERATOR_PKO_IMAGE and OPERATOR_IMAGE which breaks
reproducibility; update the YAML so OPERATOR_PKO_IMAGE and OPERATOR_IMAGE use
immutable image references (SHA256 digests or commit-correlated release tags)
instead of :latest, and make the same change for both occurrences referenced in
the config (the two OPERATOR_PKO_IMAGE and OPERATOR_IMAGE entries used by the
avo-e2e-presubmit and avo-e2e-postsubmit jobs); ensure the digest/tag strings
are correct and tested so CI pulls the exact intended image.
---
Duplicate comments:
In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh`:
- Around line 53-60: GINKGO_FLAGS is built as a single string and then expanded
unquoted into the /usr/local/bin/e2e.test command which can break when values
contain spaces or glob characters; change to an argument array (e.g.,
GINKGO_ARGS or GINKGO_FLAGS_ARRAY) where you append each flag as a separate
element (first push "--ginkgo.junit-report=${JUNIT_REPORT}" and "--ginkgo.v",
and conditionally push "--ginkgo.label-filter=${GINKGO_LABEL_FILTER}" when set),
then invoke /usr/local/bin/e2e.test with the array expansion form so each flag
is passed as its own argument to avoid splitting/globbing issues.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: caf20170-ae19-43d4-9c28-fa06e906e8b9
📒 Files selected for processing (16)
ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-postsubmits.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-presubmits.yamlci-operator/step-registry/rosa/operator/OWNERSci-operator/step-registry/rosa/operator/e2e-workflow/OWNERSci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.jsonci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yamlci-operator/step-registry/rosa/operator/e2e/OWNERSci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.shci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.jsonci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yamlci-operator/step-registry/rosa/operator/install/OWNERSci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.shci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.jsonci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yamlcore-services/testgrid-config-generator/_allow-list.yaml
✅ Files skipped from review due to trivial changes (12)
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.json
- ci-operator/step-registry/rosa/operator/e2e-workflow/OWNERS
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.json
- ci-operator/step-registry/rosa/operator/e2e/OWNERS
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.json
- core-services/testgrid-config-generator/_allow-list.yaml
- ci-operator/step-registry/rosa/operator/OWNERS
- ci-operator/step-registry/rosa/operator/install/OWNERS
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yaml
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yaml
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yaml
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-postsubmits.yaml
- ci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-presubmits.yaml
Comment on lines
+66
to
+67
| OPERATOR_PKO_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko:latest" | ||
| OPERATOR_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator:latest" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how OPERATOR_IMAGE / OPERATOR_PKO_IMAGE are consumed by the workflow scripts.
# Expectation: confirm whether only external pullspecs are supported, or if PR-correlated images can be used.
set -euo pipefail
rg -n -C3 'OPERATOR_IMAGE|OPERATOR_PKO_IMAGE|ClusterPackage|rosa-operator-install|rosa-operator-e2e' ci-operator/step-registryRepository: openshift/release
Length of output: 50373
🏁 Script executed:
#!/bin/bash
# Get the config file being reviewed
cat ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yamlRepository: openshift/release
Length of output: 2457
🏁 Script executed:
#!/bin/bash
# Check if there's any image promotion or substitution mechanism for aws-vpce-operator
# Also look for how the operator builds and where the images come from
# 1. Check if aws-vpce-operator builds its own images
fd -t f "aws-vpce-operator" ci-operator/config/ --exec grep -l "docker\|image" {} \;
# 2. Search for ci-operator image building patterns that might auto-promote
rg -n "aws-vpce-operator" ci-operator/config/openshift/ --max-count=20
# 3. Check the aws-vpce-operator project to understand image publish strategy
rg -n "OPERATOR_IMAGE|OPERATOR_PKO_IMAGE" ci-operator/step-registry/rosa/operator/ -A2 -B2 --max-count=30Repository: openshift/release
Length of output: 5265
Presubmit/postsubmit workflows are pinned to mutable external :latest images, not immutable test targets.
The avo-e2e-presubmit and avo-e2e-postsubmit jobs (lines 66–67 and 77–78) use external :latest tags for OPERATOR_PKO_IMAGE and OPERATOR_IMAGE. This breaks reproducibility—the :latest tag can change between test runs—and for the presubmit, it means the test is not validating the code changes since it always pulls whatever external image happens to be tagged :latest at that moment. Pin these to immutable digests (SHA256) or commit-correlated release tags.
Example fix
- OPERATOR_PKO_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko:latest"
- OPERATOR_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator:latest"
+ OPERATOR_PKO_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator-pko@sha256:<pko-digest>"
+ OPERATOR_IMAGE: "quay.io/redhat-services-prod/openshift/aws-vpce-operator@sha256:<operator-digest>"Also applies to: 77–78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yaml`
around lines 66 - 67, The presubmit/postsubmit jobs are using mutable :latest
image tags for OPERATOR_PKO_IMAGE and OPERATOR_IMAGE which breaks
reproducibility; update the YAML so OPERATOR_PKO_IMAGE and OPERATOR_IMAGE use
immutable image references (SHA256 digests or commit-correlated release tags)
instead of :latest, and make the same change for both occurrences referenced in
the config (the two OPERATOR_PKO_IMAGE and OPERATOR_IMAGE entries used by the
avo-e2e-presubmit and avo-e2e-postsubmit jobs); ensure the digest/tag strings
are correct and tested so CI pulls the exact intended image.
f28d044 to
3ae2f2e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh (1)
53-60:⚠️ Potential issue | 🟠 MajorFix argv construction for Ginkgo flags to prevent split/misparsed arguments.
At Line 56 and Line 60, building flags as a single string and expanding unquoted can break when
GINKGO_LABEL_FILTERcontains spaces/operators.Proposed fix
JUNIT_REPORT="${ARTIFACT_DIR}/junit-${OPERATOR_NAME}-e2e.xml" -GINKGO_FLAGS="--ginkgo.junit-report=${JUNIT_REPORT} --ginkgo.v" +GINKGO_FLAGS_ARRAY=("--ginkgo.junit-report=${JUNIT_REPORT}" "--ginkgo.v") if [[ -n "${GINKGO_LABEL_FILTER:-}" ]]; then - GINKGO_FLAGS="${GINKGO_FLAGS} --ginkgo.label-filter=${GINKGO_LABEL_FILTER}" + GINKGO_FLAGS_ARRAY+=("--ginkgo.label-filter=${GINKGO_LABEL_FILTER}") fi log "Running ${OPERATOR_NAME} e2e tests..." -/usr/local/bin/e2e.test ${GINKGO_FLAGS} || { +/usr/local/bin/e2e.test "${GINKGO_FLAGS_ARRAY[@]}" || { log "Tests failed. JUnit report at ${JUNIT_REPORT}" exit 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh` around lines 53 - 60, The GINKGO_FLAGS string is being built and expanded unquoted which can split or misparse arguments when values like GINKGO_LABEL_FILTER contain spaces or shell metacharacters; change construction to use a bash array (e.g., GINKGO_FLAGS as an array), push each flag as a separate element (including the label-filter when set), and invoke the test binary with the array expansion quoted (use "${GINKGO_FLAGS[@]}") so each flag remains a single argument; update the code that sets GINKGO_FLAGS and the /usr/local/bin/e2e.test ${GINKGO_FLAGS} invocation accordingly.
🧹 Nitpick comments (1)
ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh (1)
25-27: Use a smallread_profile_filehelper for credential reads.At Line 25–27, repeated
cat ... 2>/dev/null || trueis harder to maintain and less consistent with existing step-registry scripts. Consider adopting a reusable helper pattern (as used inci-operator/step-registry/osd/grant/cluster-editor/osd-grant-cluster-editor-commands.sh, Line 19 onward).Refactor sketch
+read_profile_file() { + local file="${1}" + if [[ -f "${CLUSTER_PROFILE_DIR}/${file}" ]]; then + cat "${CLUSTER_PROFILE_DIR}/${file}" + fi +} + - SSO_CLIENT_ID=$(cat "${CLUSTER_PROFILE_DIR}/sso-client-id" 2>/dev/null || true) - SSO_CLIENT_SECRET=$(cat "${CLUSTER_PROFILE_DIR}/sso-client-secret" 2>/dev/null || true) - OCM_TOKEN=$(cat "${CLUSTER_PROFILE_DIR}/ocm-token" 2>/dev/null || true) + SSO_CLIENT_ID=$(read_profile_file "sso-client-id") + SSO_CLIENT_SECRET=$(read_profile_file "sso-client-secret") + OCM_TOKEN=$(read_profile_file "ocm-token")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh` around lines 25 - 27, Replace the repeated "cat ... 2>/dev/null || true" calls by adding a small helper function (e.g., read_profile_file) at top of rosa-operator-e2e-commands.sh that accepts a filename, silences errors, and returns the file contents (or empty string) and then use it to initialize SSO_CLIENT_ID, SSO_CLIENT_SECRET, and OCM_TOKEN; specifically, define read_profile_file similar to the helper used in osd-grant-cluster-editor-commands.sh (accepts a path and echoes content or nothing) and update the three assignments (SSO_CLIENT_ID, SSO_CLIENT_SECRET, OCM_TOKEN) to call that helper instead of inlining the cat+redir pattern so credential reads are consistent and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh`:
- Around line 63-68: Replace the hardcoded wait on "${OPERATOR_NAME}" with a
configurable deployment target: add/expect an environment variable (e.g.,
OPERATOR_DEPLOYMENT_NAME) that defaults to "${OPERATOR_NAME}" and use that
variable in the oc wait command and log; update the log line to reference
"${OPERATOR_DEPLOYMENT_NAME}" and the oc wait invocation to wait on deployment
"${OPERATOR_DEPLOYMENT_NAME}" in namespace "${OPERATOR_NAMESPACE}" so the
workflow can target a different deployment name when required.
- Around line 3-16: The script currently uses set -o nounset so expanding
${SHARED_DIR} in the test if [[ -f "${SHARED_DIR}/kubeconfig" ]] can fail when
SHARED_DIR is unset; change the conditional to guard the expansion, e.g. test
that SHARED_DIR is non-empty and then the file exists (for example: if [[ -n
"${SHARED_DIR:-}" && -f "${SHARED_DIR}/kubeconfig" ]]; then) or use the safe
expansion in the -f test (if [[ -f "${SHARED_DIR:-}/kubeconfig" ]]; then), then
export KUBECONFIG as before; reference SHARED_DIR and KUBECONFIG in your patch.
---
Duplicate comments:
In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh`:
- Around line 53-60: The GINKGO_FLAGS string is being built and expanded
unquoted which can split or misparse arguments when values like
GINKGO_LABEL_FILTER contain spaces or shell metacharacters; change construction
to use a bash array (e.g., GINKGO_FLAGS as an array), push each flag as a
separate element (including the label-filter when set), and invoke the test
binary with the array expansion quoted (use "${GINKGO_FLAGS[@]}") so each flag
remains a single argument; update the code that sets GINKGO_FLAGS and the
/usr/local/bin/e2e.test ${GINKGO_FLAGS} invocation accordingly.
---
Nitpick comments:
In `@ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.sh`:
- Around line 25-27: Replace the repeated "cat ... 2>/dev/null || true" calls by
adding a small helper function (e.g., read_profile_file) at top of
rosa-operator-e2e-commands.sh that accepts a filename, silences errors, and
returns the file contents (or empty string) and then use it to initialize
SSO_CLIENT_ID, SSO_CLIENT_SECRET, and OCM_TOKEN; specifically, define
read_profile_file similar to the helper used in
osd-grant-cluster-editor-commands.sh (accepts a path and echoes content or
nothing) and update the three assignments (SSO_CLIENT_ID, SSO_CLIENT_SECRET,
OCM_TOKEN) to call that helper instead of inlining the cat+redir pattern so
credential reads are consistent and maintainable.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a358031e-0501-4068-a319-ce5e44e99a5c
📒 Files selected for processing (16)
ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-postsubmits.yamlci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-presubmits.yamlci-operator/step-registry/rosa/operator/OWNERSci-operator/step-registry/rosa/operator/e2e-workflow/OWNERSci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.jsonci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yamlci-operator/step-registry/rosa/operator/e2e/OWNERSci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-commands.shci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.jsonci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yamlci-operator/step-registry/rosa/operator/install/OWNERSci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.shci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.jsonci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yamlcore-services/testgrid-config-generator/_allow-list.yaml
✅ Files skipped from review due to trivial changes (12)
- ci-operator/step-registry/rosa/operator/OWNERS
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.metadata.json
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.metadata.json
- ci-operator/step-registry/rosa/operator/e2e-workflow/OWNERS
- ci-operator/step-registry/rosa/operator/e2e/OWNERS
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.metadata.json
- core-services/testgrid-config-generator/_allow-list.yaml
- ci-operator/step-registry/rosa/operator/install/rosa-operator-install-ref.yaml
- ci-operator/step-registry/rosa/operator/e2e-workflow/rosa-operator-e2e-workflow-workflow.yaml
- ci-operator/step-registry/rosa/operator/install/OWNERS
- ci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-presubmits.yaml
- ci-operator/step-registry/rosa/operator/e2e/rosa-operator-e2e-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/config/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main.yaml
- ci-operator/jobs/openshift/aws-vpce-operator/openshift-aws-vpce-operator-main-postsubmits.yaml
Comment on lines
+3
to
+16
| set -o nounset | ||
| set -o errexit | ||
| set -o pipefail | ||
|
|
||
| trap 'CHILDREN=$(jobs -p); if test -n "${CHILDREN}"; then kill ${CHILDREN} && wait; fi' TERM | ||
|
|
||
| log(){ | ||
| echo -e "\033[1m$(date "+%d-%m-%YT%H:%M:%S") " "${*}\033[0m" >&2 | ||
| } | ||
|
|
||
| # Use shared kubeconfig from provision step if available | ||
| if [[ -f "${SHARED_DIR}/kubeconfig" ]]; then | ||
| export KUBECONFIG="${SHARED_DIR}/kubeconfig" | ||
| fi |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Repro current behavior: fails with "unbound variable"
unset SHARED_DIR || true
bash -o nounset -c 'if [[ -f "${SHARED_DIR}/kubeconfig" ]]; then echo ok; fi' || echo "Current code path fails as expected"
# Verify guarded behavior: does not fail when SHARED_DIR is unset
bash -o nounset -c 'if [[ -n "${SHARED_DIR:-}" && -f "${SHARED_DIR}/kubeconfig" ]]; then echo ok; fi'
echo "Guarded check succeeds without unbound-variable failure"Repository: openshift/release
Length of output: 212
Guard SHARED_DIR expansion under nounset.
With set -o nounset enabled on line 2, the expansion of ${SHARED_DIR} on line 15 will cause the script to exit with an "unbound variable" error if the variable is not set, before the conditional check is evaluated. This contradicts the intent of the "if available" comment.
Proposed fix
-# Use shared kubeconfig from provision step if available
-if [[ -f "${SHARED_DIR}/kubeconfig" ]]; then
- export KUBECONFIG="${SHARED_DIR}/kubeconfig"
+# Use shared kubeconfig from provision step if available
+if [[ -n "${SHARED_DIR:-}" && -f "${SHARED_DIR}/kubeconfig" ]]; then
+ export KUBECONFIG="${SHARED_DIR}/kubeconfig"
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 -o nounset | |
| set -o errexit | |
| set -o pipefail | |
| trap 'CHILDREN=$(jobs -p); if test -n "${CHILDREN}"; then kill ${CHILDREN} && wait; fi' TERM | |
| log(){ | |
| echo -e "\033[1m$(date "+%d-%m-%YT%H:%M:%S") " "${*}\033[0m" >&2 | |
| } | |
| # Use shared kubeconfig from provision step if available | |
| if [[ -f "${SHARED_DIR}/kubeconfig" ]]; then | |
| export KUBECONFIG="${SHARED_DIR}/kubeconfig" | |
| fi | |
| set -o nounset | |
| set -o errexit | |
| set -o pipefail | |
| trap 'CHILDREN=$(jobs -p); if test -n "${CHILDREN}"; then kill ${CHILDREN} && wait; fi' TERM | |
| log(){ | |
| echo -e "\033[1m$(date "+%d-%m-%YT%H:%M:%S") " "${*}\033[0m" >&2 | |
| } | |
| # Use shared kubeconfig from provision step if available | |
| if [[ -n "${SHARED_DIR:-}" && -f "${SHARED_DIR}/kubeconfig" ]]; then | |
| export KUBECONFIG="${SHARED_DIR}/kubeconfig" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh`
around lines 3 - 16, The script currently uses set -o nounset so expanding
${SHARED_DIR} in the test if [[ -f "${SHARED_DIR}/kubeconfig" ]] can fail when
SHARED_DIR is unset; change the conditional to guard the expansion, e.g. test
that SHARED_DIR is non-empty and then the file exists (for example: if [[ -n
"${SHARED_DIR:-}" && -f "${SHARED_DIR}/kubeconfig" ]]; then) or use the safe
expansion in the -f test (if [[ -f "${SHARED_DIR:-}/kubeconfig" ]]; then), then
export KUBECONFIG as before; reference SHARED_DIR and KUBECONFIG in your patch.
Comment on lines
+63
to
+68
| # Wait for the operator deployment to be available | ||
| log "Waiting for ${OPERATOR_NAME} deployment to be ready..." | ||
| oc wait deployment "${OPERATOR_NAME}" \ | ||
| -n "${OPERATOR_NAMESPACE}" \ | ||
| --for=condition=Available \ | ||
| --timeout=300s |
Contributor
There was a problem hiding this comment.
Make deployment readiness target configurable for reuse.
Waiting only on deployment ${OPERATOR_NAME} is brittle for a generic workflow; some operators use a different deployment name. This can create false negatives.
💡 Proposed fix
OPERATOR_NAMESPACE="${OPERATOR_NAMESPACE:-openshift-${OPERATOR_NAME}}"
+OPERATOR_DEPLOYMENT_NAME="${OPERATOR_DEPLOYMENT_NAME:-${OPERATOR_NAME}}"
+OPERATOR_READY_TIMEOUT="${OPERATOR_READY_TIMEOUT:-300s}"
@@
-log "Waiting for ${OPERATOR_NAME} deployment to be ready..."
-oc wait deployment "${OPERATOR_NAME}" \
+log "Waiting for deployment ${OPERATOR_DEPLOYMENT_NAME} to be ready..."
+oc wait deployment "${OPERATOR_DEPLOYMENT_NAME}" \
-n "${OPERATOR_NAMESPACE}" \
--for=condition=Available \
- --timeout=300s
+ --timeout="${OPERATOR_READY_TIMEOUT}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/rosa/operator/install/rosa-operator-install-commands.sh`
around lines 63 - 68, Replace the hardcoded wait on "${OPERATOR_NAME}" with a
configurable deployment target: add/expect an environment variable (e.g.,
OPERATOR_DEPLOYMENT_NAME) that defaults to "${OPERATOR_NAME}" and use that
variable in the oc wait command and log; update the log line to reference
"${OPERATOR_DEPLOYMENT_NAME}" and the oc wait invocation to wait on deployment
"${OPERATOR_DEPLOYMENT_NAME}" in namespace "${OPERATOR_NAMESPACE}" so the
workflow can target a different deployment name when required.
0051b56 to
fcd5c9a
Compare
openshift-ci
Bot
added
the
needs-rebase
openshift-merge-bot
Bot
added
the
rehearsals-ack
dustman9000
force-pushed
the
srep-4417/avo-operator-e2e-poc
branch
from
fcd5c9a to
39a8608
Compare
openshift-ci
Bot
removed
the
needs-rebase
openshift-merge-bot
Bot
removed
the
rehearsals-ack
Create reusable step registry components for operator e2e testing: - rosa-operator-install: installs operator via PKO ClusterPackage - rosa-operator-e2e: runs Ginkgo e2e binary (ephemeral or persistent cluster) - rosa-operator-e2e-workflow: provisions ROSA Classic STS, installs operator via PKO, runs tests, deprovisions Add AVO as the first consumer with: - Presubmit: runs on PRs that change operator/test code - Postsubmit: runs on every merge, GitHub commit status gates SAPM promotion to stage/production No osde2e dependency -- runs the Ginkgo binary directly. Jira: SREP-4417
- Guard SHARED_DIR expansion with nounset-safe check - Use bash array for Ginkgo args to prevent word splitting - Add configurable OPERATOR_DEPLOYMENT_NAME for operators whose deployment name differs from the operator name
dustman9000
force-pushed
the
srep-4417/avo-operator-e2e-poc
branch
from
5b573de to
2a44d65
Compare
Member
Author
|
/pj-rehearse auto-ack |
Contributor
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Contributor
|
@dustman9000: 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. |
Member
Author
|
/retest |
The rosa-operator-e2e-workflow provision chain requires rosa-aws-cli and release:latest image stream tags. Without these, ci-operator fails with "steps are missing dependencies".
Member
Author
|
/retest |
Contributor
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
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
Architecture
The workflow provisions an ephemeral ROSA Classic STS cluster, installs the operator via PKO ClusterPackage (matching production install method), runs the e2e tests, and deprovisions. JUnit results flow to GCS for Sippy ingestion.
Companion PR in openshift/aws-vpce-operator adds the Containerfile that builds the e2e binary on rosa-aws-cli base.
Adding more operators
To add another operator (e.g. route-monitor-operator), create a new ci-operator periodics config with the same workflow and different OPERATOR_NAME/OPERATOR_IMAGE/OPERATOR_PKO_IMAGE env vars.
Jira
Summary by CodeRabbit