Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
| Layer / File(s) | Summary |
|---|---|
Workflow input derivation and dispatch .github/workflows/auto-smoke-test.yml, .github/workflows/acs-smoke-test.yml |
Defines triggers and inputs, derives acs-version and operator-index-image, and conditionally calls the reusable smoke-test workflow. |
Cluster provisioning and smoke test run .github/workflows/acs-smoke-test.yml |
Creates the OpenShift cluster, runs the smoke-test scripts in a container, resets operator state between runs, deletes the cluster, and publishes the step summary. |
Smoke test helper library smoke-test/lib.sh |
Adds Bash helpers for logging, version parsing, catalog source handling, subscription management, CSV polling, install and upgrade primitives, CSV logging, and cleanup. |
Install and upgrade flows smoke-test/lib.sh |
Adds official and custom catalog install paths, custom and latest-GA upgrade paths, and reset behavior between tests. |
Latest GA upgrade test smoke-test/upgrade-latest-test.sh |
Implements the custom-install and conditional latest-GA upgrade test flow using the shared helpers. |
Estimated code review effort: 4 (Complex) | ~60 minutes
🚥 Pre-merge checks | ✅ 5
-
✅ Passed checks (5 passed)
-
Check name Status Explanation Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request. Title check ✅ Passed The title clearly and concisely identifies the operator-index upgrade test added by the pull request. Description check ✅ Passed The description accurately explains the GitHub Actions workflows and the two ACS operator upgrade test scenarios.
-
✨ Finishing Touches
-
📝 Generate docstrings
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
akurlov/ROX-29003-smoke-test-on-infra
-
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/auto-smoke-test.yml (1)
1-115: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit
permissions:block.No
permissions:is set for this workflow, soderive-inputsruns with default (often broad)GITHUB_TOKENscope, and the flagged token is what's used for thegit diff/checkout in this job.As per static analysis hints (zizmor `excessive-permissions`, lines 1-115, 26-104).🔒 Proposed fix
name: "Auto Smoke Test" + +permissions: + contents: read on:🤖 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 @.github/workflows/auto-smoke-test.yml around lines 1 - 115, Add an explicit permissions block for this workflow so the default GITHUB_TOKEN is not over-scoped. Update the workflow near the top of the file and ensure the derive-inputs job only has the minimum access needed for actions/checkout and the git diff logic, rather than relying on the broad default token permissions. Use the workflow-level permissions setting to lock this down.Source: Linters/SAST tools
.github/workflows/acs-smoke-test.yml (1)
1-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an explicit
permissions:block.No
permissions:is declared, so the workflow runs with the (often broad) defaultGITHUB_TOKENscope for every job, includingrun-smoke-testswhich also holdsGH_TOKEN: ${{ github.token }}in env. Scope this down to least privilege (e.g.contents: read), overriding per-job if a step genuinely needs more.🔒 Proposed fix
name: "ACS Smoke Test" + +permissions: + contents: read on:🤖 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 @.github/workflows/acs-smoke-test.yml around lines 1 - 43, Add an explicit permissions block to the ACS Smoke Test workflow so the default GITHUB_TOKEN scope is restricted to least privilege instead of inheriting broad access. Update the workflow-level configuration near the on/workflow_call and env sections, and keep any job-specific elevation only where a step truly needs it; the run-smoke-tests job should continue using GH_TOKEN from github.token but with the workflow permissions narrowed, such as contents: read.smoke-test/upgrade-latest-test.sh (1)
32-42: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winGraceful-skip claim doesn't hold —
wait_for_cataloghard-exits the whole script.The comment says a catalog timeout should degrade gracefully, but
wait_for_catalogis invoked directly in theifcondition (not in a subshell). Sincewait_for_catalogcallsfail(), which doesexit 1(lib.shline 11), a timeout kills the entire script instead of just falling through to theelsebranch. The&&short-circuit never gets a chance to evaluate to false because the process exits first.Wrap the call in a subshell so its internal
exitonly terminates the subshell, letting theif/elselogic work as intended:🐛 Proposed fix
-if wait_for_catalog "redhat-operators" "openshift-marketplace" 180 && +if ( wait_for_catalog "redhat-operators" "openshift-marketplace" 180 ) && latest=$(get_latest_official_minor "$ACS_MAJOR") && [[ -n "$latest" ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smoke-test/upgrade-latest-test.sh` around lines 32 - 42, The graceful-skip path in `upgrade-latest-test.sh` does not work because `wait_for_catalog` can call `fail()` and exit the whole script before the `if` can fall through. Update the `if` condition around `wait_for_catalog`, `get_latest_official_minor`, and the `LATEST_MINOR` assignment so the catalog check runs in a subshell and only affects the condition result. Keep the existing `info`/`warn` branches, but ensure a timeout or catalog failure does not terminate the script and instead reaches the upgrade-skip `else` path.
🧹 Nitpick comments (3)
.github/workflows/auto-smoke-test.yml (2)
34-37: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout.
fetch-depth: 0checkout persists theGITHUB_TOKENcredential by default; this job doesn't push anything, so disable persistence for defense-in-depth.As per static analysis hints (zizmor `artipacked`, lines 34-37).🔒 Proposed fix
- name: Check out code uses: actions/checkout@v7 with: fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/auto-smoke-test.yml around lines 34 - 37, The checkout step in the workflow currently leaves the default GitHub token credentials persisted, which is unnecessary for this read-only job. Update the actions/checkout usage in the “Check out code” step to disable credential persistence by setting persist-credentials to false alongside the existing fetch-depth setting. This is the only change needed in the checkout configuration.Source: Linters/SAST tools
106-114: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider scoping
secrets: inheritto only what's needed.
secrets: inheritpasses every repo/org secret to the called workflow, which only needsGCP_RELEASE_AUTOMATION_SA. Passing it explicitly viasecrets:would narrow the blast radius if this workflow is ever triggered from less-trusted contexts.
As per static analysis hints (zizmorsecrets-inherit, line 110).🤖 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 @.github/workflows/auto-smoke-test.yml around lines 106 - 114, The reusable workflow invocation in smoke-test currently uses secrets: inherit, which exposes all available secrets to the called workflow. Replace it with an explicit secrets mapping that passes only the secret actually needed by acs-smoke-test.yml, using the smoke-test job’s uses block and secrets configuration to narrow access.Source: Linters/SAST tools
.github/workflows/acs-smoke-test.yml (1)
71-72: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider
persist-credentials: falseon checkout.The container job checks out the repo but never pushes; persisting the token on disk is unnecessary residual exposure, especially combined with the missing
permissions:block above.🔒 Proposed fix
- name: Check out code uses: actions/checkout@v7 + with: + persist-credentials: false🤖 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 @.github/workflows/acs-smoke-test.yml around lines 71 - 72, The checkout step in the ACS smoke test workflow leaves Git credentials on disk even though the container job does not push; update the `actions/checkout` usage to disable credential persistence, and make sure the workflow’s job-level `permissions` are explicitly scoped as tightly as possible. Use the `Check out code` step in `acs-smoke-test.yml` and the surrounding job definition to apply the security hardening consistently.
🤖 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 @.github/workflows/auto-smoke-test.yml:
- Around line 72-105: The Determine operator-index image step is interpolating
inputs.operator-index-image-override directly inside the shell script, which
should be avoided. Move the override value into an env entry for this step and
read it from the environment in the run block instead of embedding the input in
the script. Keep the existing logic in the image step and update the
operator-index-image-override check to use the env-backed value so the rest of
the loop and GITHUB_OUTPUT handling stays unchanged.
- Around line 54-58: Update the NEW_VERSIONS extraction in the auto-smoke-test
workflow so it only matches indented bundle version entries, not any added key
containing version:. The current grep in the bundle diff parsing can
accidentally pick up oldest_supported_version lines; tighten the pattern in the
version-filtering pipeline (the shell snippet that uses git diff, grep, sed, and
sort) to match only lines with leading indentation before version so
support-policy updates are ignored.
- Around line 39-70: The Determine ACS version step is interpolating user/input
and GitHub context directly into the shell script, so move the acs-version
override and base ref values into env variables instead of embedding `${{
inputs.acs-version-override }}` and `${{ github.base_ref }}` in the run block.
Update the workflow step to read those values inside the script as
`$ACS_VERSION_OVERRIDE` and `$BASE_REF`, and keep the existing logic in the
version-determination flow unchanged.
---
Outside diff comments:
In @.github/workflows/acs-smoke-test.yml:
- Around line 1-43: Add an explicit permissions block to the ACS Smoke Test
workflow so the default GITHUB_TOKEN scope is restricted to least privilege
instead of inheriting broad access. Update the workflow-level configuration near
the on/workflow_call and env sections, and keep any job-specific elevation only
where a step truly needs it; the run-smoke-tests job should continue using
GH_TOKEN from github.token but with the workflow permissions narrowed, such as
contents: read.
In @.github/workflows/auto-smoke-test.yml:
- Around line 1-115: Add an explicit permissions block for this workflow so the
default GITHUB_TOKEN is not over-scoped. Update the workflow near the top of the
file and ensure the derive-inputs job only has the minimum access needed for
actions/checkout and the git diff logic, rather than relying on the broad
default token permissions. Use the workflow-level permissions setting to lock
this down.
In `@smoke-test/upgrade-latest-test.sh`:
- Around line 32-42: The graceful-skip path in `upgrade-latest-test.sh` does not
work because `wait_for_catalog` can call `fail()` and exit the whole script
before the `if` can fall through. Update the `if` condition around
`wait_for_catalog`, `get_latest_official_minor`, and the `LATEST_MINOR`
assignment so the catalog check runs in a subshell and only affects the
condition result. Keep the existing `info`/`warn` branches, but ensure a timeout
or catalog failure does not terminate the script and instead reaches the
upgrade-skip `else` path.
---
Nitpick comments:
In @.github/workflows/acs-smoke-test.yml:
- Around line 71-72: The checkout step in the ACS smoke test workflow leaves Git
credentials on disk even though the container job does not push; update the
`actions/checkout` usage to disable credential persistence, and make sure the
workflow’s job-level `permissions` are explicitly scoped as tightly as possible.
Use the `Check out code` step in `acs-smoke-test.yml` and the surrounding job
definition to apply the security hardening consistently.
In @.github/workflows/auto-smoke-test.yml:
- Around line 34-37: The checkout step in the workflow currently leaves the
default GitHub token credentials persisted, which is unnecessary for this
read-only job. Update the actions/checkout usage in the “Check out code” step to
disable credential persistence by setting persist-credentials to false alongside
the existing fetch-depth setting. This is the only change needed in the checkout
configuration.
- Around line 106-114: The reusable workflow invocation in smoke-test currently
uses secrets: inherit, which exposes all available secrets to the called
workflow. Replace it with an explicit secrets mapping that passes only the
secret actually needed by acs-smoke-test.yml, using the smoke-test job’s uses
block and secrets configuration to narrow access.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e72ae90a-9989-401c-a338-0777f2c3706a
📒 Files selected for processing (4)
.github/workflows/acs-smoke-test.yml.github/workflows/auto-smoke-test.ymlsmoke-test/lib.shsmoke-test/upgrade-latest-test.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/auto-smoke-test.yml (1)
57-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
github.base_refstill interpolated directly into shell.The
acs-version-overrideandoperator-index-image-overrideinterpolations were correctly moved intoenv:, butgithub.base_refat Line 58 is still expanded directly inside therun:script — the exact concern raised previously and still flagged by static analysis.🔒 Proposed fix
- name: Determine ACS version id: version env: ACS_VERSION_OVERRIDE: ${{ inputs.acs-version-override }} OPERATOR_INDEX_IMAGE_OVERRIDE: ${{ inputs.operator-index-image-override }} + BASE_REF: ${{ github.base_ref }} run: | ... if [ "${{ github.event_name }}" = "pull_request" ]; then - DIFF_BASE="origin/${{ github.base_ref }}" + DIFF_BASE="origin/$BASE_REF"🤖 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 @.github/workflows/auto-smoke-test.yml around lines 57 - 58, The pull_request branch logic in the auto-smoke-test workflow still interpolates github.base_ref directly inside the run script, so move that value into env like the other overrides and reference the environment variable from the shell instead. Update the DIFF_BASE assignment in the workflow job that contains the pull_request check so the shell only reads a safe env var rather than expanding github.base_ref inline.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/auto-smoke-test.yml:
- Around line 57-58: The pull_request branch logic in the auto-smoke-test
workflow still interpolates github.base_ref directly inside the run script, so
move that value into env like the other overrides and reference the environment
variable from the shell instead. Update the DIFF_BASE assignment in the workflow
job that contains the pull_request check so the shell only reads a safe env var
rather than expanding github.base_ref inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 92759a7c-bf3e-4980-a585-8e3df4768a04
📒 Files selected for processing (1)
.github/workflows/auto-smoke-test.yml
tommartensen
left a comment
tommartensen
left a comment
There was a problem hiding this comment.
Review of the auto-smoke-test workflow only.
tommartensen
left a comment
tommartensen
left a comment
There was a problem hiding this comment.
Partial review of acs-smoke-test.yml
tommartensen
left a comment
tommartensen
left a comment
There was a problem hiding this comment.
Some feedback on the scripts.
I think there is a gap with regards to handling major version bumps.
tommartensen
left a comment
tommartensen
left a comment
There was a problem hiding this comment.
I think the last comments for now.
|
@mclasmeier making you aware how this PR deploys ACS and requesting your input - is there any overlap to what roxie could handle (now or in the future)? |
mclasmeier
left a comment
mclasmeier
left a comment
There was a problem hiding this comment.
I have a general question about this:
Maybe this is simply has been an assumption on my side, so I am interested in learning about your perspective here (@kurlov, @tommartensen, @msugakov).
I kind of thought that we (ACS eng) consider it be worthwhile to move away from shell script based testing code to Go tests (easier to handle increasing complexity, typed, good testing support, often times less hacky...).
Is this something that has been considered for these tests?
I guess it would be needed to extend roxie so that it can be explicitly pointed to some new operator-index artifact. While is could be generally useful, I also see some value in doing it "the customer way" here (which is not roxie). |
|
I +1 on avoiding bash scripts, Makefiles, as well as Python and YAML where possible, and having more code implemented in Go. As compared to bash scripts, Go code:
The advantages of using Go are huge and convincing, in my opinion. |
Co-authored-by: Tom Martensen <mail@tommartensen.de>
kurlov
changed the title
tommartensen
left a comment
tommartensen
left a comment
There was a problem hiding this comment.
Few minor things.
I couldn't find any recent test runs in https://github.com/stackrox/operator-index/actions/workflows/operator-index-upgrade-test.yml, that would be sweet (both manual dispatch and pull_request trigger)
Co-authored-by: Tom Martensen <mail@tommartensen.de>
…moke-test-on-infra
1eab970 to
2580ae9
Compare
Redirects all ACS component images from registry.redhat.io to quay.io/rhacs-eng: - operator (RHEL 8 and 9) - main, scanner, scanner-db, scanner-slim, scanner-db-slim - collector, roxctl, central-db - scanner-v4, scanner-v4-db - fact This allows testing RC bundles that reference registry.redhat.io images not yet published to the Red Hat registry. Based on manual RC testing procedure.
Only include mirrors for operator bundle and operator controller images (RHEL 8 and 9 variants). We don't need Central/SecuredCluster component images since the upgrade test only verifies operator installation, not full ACS deployment. Tested locally: 4.10.10 → 4.10.11-rc.5 upgrade succeeds. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1. Increase catalog timeout from 3m to 5m - Fixes failures waiting for redhat-operators catalog 2. Show full versions in summary instead of streams - Target version: Extract from bundles.yaml (e.g., 4.10.11-rc.5) - Latest GA: Show actual GA version (e.g., 4.11.6) - From/To: Display full versions for clarity 3. Add run attempt to cluster name - Format: upg-v410-ocp412-pr496-r1 - Useful for reruns and multiple commits per PR - Max length: 28 chars (upg-v500-ocp500-pr999-r99) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
New format: upg-v410-ocp412-abc1234-1 - Use 7-char git commit hash instead of PR number - Remove 'r' prefix from run attempt (just the number) - Max length: 26 chars (upg-v500-ocp500-abc1234-99) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Test 2 always runs and installs target version fresh, then conditionally upgrades to latest GA. Summary now reflects this: **For versions behind GA (e.g., 4.10.11-rc.5):** - Install: `4.10.11-rc.5` - Then upgrade to: `4.11.6` **For versions at/ahead of GA (e.g., 5.0.0-rc.4):** - Install: `5.0.0-rc.4` - No upgrade (target is at or ahead of latest GA `4.11.6`) Also simplified summary: - Removed redundant "Path:" lines - Removed "(from redhat-operators)" and "(from custom index)" annotations Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@tommartensen two testing PR where automatic operator-upgrade-test workflow triggered based on bundles.yaml change are ✅ However I just pushed a few nitpick changes for the summary page rendering and there will be another runs in progress Upd: new runs: |
Co-authored-by: Tom Martensen <tmartens@redhat.com>
Add an automated operator-index upgrade test running on GHA for PRs which add a new ACS version(s) to the bundles.yaml file (e.g. #427).
GH actions files:
operator-index-upgrade-test- setups infra cluster and triggers smoke test for the provided operator-index image and ACS version.auto-operator-index-upgrade-test- derives ACS version(s) from diff of PR which updatesbundles.yaml. Also it waits for operator-index image. Then it builds a test matrix of ACS version(s) and operator-index image (the image is the same no matter how many ACS versions was added to the bundles.yaml file).See test for this automatic workflow below 🟢
Operator-index upgrade test
The test runs on the single infra cluster. The test consists of two scenarios:
Why
ocCLI over a Kubernetes client libraryThe test deliberately mirrors what an actual user runs to upgrade the ACS operator — applying a list of commands until ACS operator is READY.
ockeeps the test readable, which makes test failures easy to reproduce and debug outside of CI.client-go or controller-runtimewould require registering OLM-specific CRD types (CatalogSource, Subscription, PackageManifest, OperatorHub)ocbased test is easier to adopt for a future OpenShift version. Just update/add anoccommand and no need to find the the right API version in the dependencyTesting
I used two testing PRs to trigger automatic operator upgrade test CI: