NO-ISSUE: rebase-main-nightly_amd64-2026-04-16-172425_arm64-2026-04-16-232425#6520
Conversation
|
@pacevedom: This pull request explicitly references no jira issue. 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
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:
WalkthroughThis pull request advances OCP references from 4.22 → 5.0 across manifests and configs, swaps image repositories from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/components/service-ca/deployment.yaml`:
- Around line 40-42: The env entry OPERATOR_IMAGE_VERSION is declared without a
value and will be empty at runtime; either remove this env var if it's unused or
set a concrete source—add a value: "1.18.1" (or the correct operator version) or
a valueFrom that references the release payload/Helm value used by your charts
(e.g., replace the nonexistent {{ .ReleaseImage.service_ca_operator_version }}
with the correct template key like a .Values or other release variable); update
the Deployment spec where OPERATOR_IMAGE_VERSION is set so the operator receives
a non-empty value.
🪄 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: Pro Plus
Run ID: 9882be4e-6815-470b-922f-6b32ea14dc87
⛔ Files ignored due to path filters (2)
etcd/go.sumis excluded by!**/*.sumetcd/vendor/modules.txtis excluded by!**/vendor/**
📒 Files selected for processing (20)
Makefile.version.aarch64.varMakefile.version.x86_64.varassets/components/multus/kustomization.aarch64.yamlassets/components/multus/kustomization.x86_64.yamlassets/components/multus/release-multus-aarch64.jsonassets/components/multus/release-multus-x86_64.jsonassets/components/service-ca/clusterrole.yamlassets/components/service-ca/deployment.yamlassets/optional/operator-lifecycle-manager/kustomization.aarch64.yamlassets/optional/operator-lifecycle-manager/kustomization.x86_64.yamlassets/optional/operator-lifecycle-manager/release-olm-aarch64.jsonassets/optional/operator-lifecycle-manager/release-olm-x86_64.jsonassets/release/release-aarch64.jsonassets/release/release-x86_64.jsonetcd/go.modpackaging/crio.conf.d/10-microshift_amd64.confpackaging/crio.conf.d/10-microshift_arm64.confscripts/auto-rebase/changelog.txtscripts/auto-rebase/commits.txtscripts/auto-rebase/last_rebase.sh
| env: | ||
| - name: OPERATOR_IMAGE_VERSION | ||
| terminationMessagePolicy: FallbackToLogsOnError |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find consumers of OPERATOR_IMAGE_VERSION in vendored service-ca-operator, and any templating of this env elsewhere in assets.
rg -nP -C2 '\bOPERATOR_IMAGE_VERSION\b'
fd -t f 'deployment.yaml' assets/components | xargs rg -nP -C2 'OPERATOR_IMAGE_VERSION|ReleaseImage'Repository: openshift/microshift
Length of output: 3732
🏁 Script executed:
cat assets/components/service-ca/deployment.yamlRepository: openshift/microshift
Length of output: 2146
🏁 Script executed:
rg -n 'ReleaseImage' assets/ -A 1 -B 1 | head -50Repository: openshift/microshift
Length of output: 3792
🏁 Script executed:
rg -n 'service_ca_operator' assets/ -A 2 -B 2Repository: openshift/microshift
Length of output: 515
🏁 Script executed:
rg -n 'OPERATOR_IMAGE_VERSION' --type yaml assets/Repository: openshift/microshift
Length of output: 250
🏁 Script executed:
find . -name "*.go" -o -name "*.py" | xargs rg -l 'OPERATOR_IMAGE_VERSION' 2>/dev/null | head -5Repository: openshift/microshift
Length of output: 46
OPERATOR_IMAGE_VERSION env var has no value (or valueFrom)—resolves to empty string at runtime.
This is missing a value assignment. Compare to cert-manager/manager.yaml, which sets this to 1.18.1. If the operator consumes this variable, an empty string will likely cause issues (blank version metadata or operand misbehavior).
The suggested template field {{ .ReleaseImage.service_ca_operator_version }} does not exist in the codebase. Either:
- Drop the env entry entirely if unused, or
- Provide the correct value source (hardcoded, templated, or from release payload)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/components/service-ca/deployment.yaml` around lines 40 - 42, The env
entry OPERATOR_IMAGE_VERSION is declared without a value and will be empty at
runtime; either remove this env var if it's unused or set a concrete source—add
a value: "1.18.1" (or the correct operator version) or a valueFrom that
references the release payload/Helm value used by your charts (e.g., replace the
nonexistent {{ .ReleaseImage.service_ca_operator_version }} with the correct
template key like a .Values or other release variable); update the Deployment
spec where OPERATOR_IMAGE_VERSION is set so the operator receives a non-empty
value.
pacevedom
force-pushed
the
rebase-bbad4d6f8-5.0.0-0.nightly-2026-04-16-172425_amd64-2026-04-16_arm64-2026-04-16
branch
from
d5e31af to
c781b76
Compare
openshift-ci
bot
added
lgtm
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/bin/pyutils/generate_common_versions.py (1)
358-392: LGTM — VERSION_MAP serialization is clean.Generates
[4]=22 ...string consumable bydeclare -A. Empty-map case (declare -A X=()) also remains valid. One tiny nit if you ever revisit: you could sort items for deterministic output across Python dict reorderings, but not a concern at current size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bin/pyutils/generate_common_versions.py` around lines 358 - 392, The generated last_minor_for_major_bash string should be deterministic across dict orderings: update the comprehension that builds last_minor_for_major_bash to iterate over a sorted VERSION_MAP (e.g., sorted(VERSION_MAP.items(), key=lambda kv: int(kv[0]) or kv[0])) so entries like "[{major}]={info['last_minor']}" are produced in a stable order; locate the last_minor_for_major_bash construction in generate_common_versions.py and replace the unsorted VERSION_MAP.items() iteration with a sorted version.test/image-blueprints/layer2-presubmit/group1/rhel98-source-fake-next-minor.toml (1)
14-14: Nit: parent ref still hardcodes4..The
# parent = "rhel-9.6-microshift-4.{{ .Env.PREVIOUS_MINOR_VERSION }}"line keeps a literal4.prefix. Fine for this rebase (previous minor is still 4.x), but it'll need the same major-aware treatment the next timePREVIOUS_MINOR_VERSIONcrosses a major boundary. Worth a follow-up to introduce aPREVIOUS_MAJOR_VERSIONenv for symmetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/image-blueprints/layer2-presubmit/group1/rhel98-source-fake-next-minor.toml` at line 14, The parent line hardcodes the literal "4." prefix in the string "rhel-9.6-microshift-4.{{ .Env.PREVIOUS_MINOR_VERSION }}", which will break when PREVIOUS_MINOR_VERSION crosses a major boundary; update the template to derive the full previous version using both major and minor env vars by adding a PREVIOUS_MAJOR_VERSION environment variable and replacing the hardcoded "4." with "{{ .Env.PREVIOUS_MAJOR_VERSION }}.{{ .Env.PREVIOUS_MINOR_VERSION }}" (or equivalent concatenation) so the parent reference becomes major-aware and symmetric with PREVIOUS_MINOR_VERSION.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 358-392: The generated last_minor_for_major_bash string should be
deterministic across dict orderings: update the comprehension that builds
last_minor_for_major_bash to iterate over a sorted VERSION_MAP (e.g.,
sorted(VERSION_MAP.items(), key=lambda kv: int(kv[0]) or kv[0])) so entries like
"[{major}]={info['last_minor']}" are produced in a stable order; locate the
last_minor_for_major_bash construction in generate_common_versions.py and
replace the unsorted VERSION_MAP.items() iteration with a sorted version.
In
`@test/image-blueprints/layer2-presubmit/group1/rhel98-source-fake-next-minor.toml`:
- Line 14: The parent line hardcodes the literal "4." prefix in the string
"rhel-9.6-microshift-4.{{ .Env.PREVIOUS_MINOR_VERSION }}", which will break when
PREVIOUS_MINOR_VERSION crosses a major boundary; update the template to derive
the full previous version using both major and minor env vars by adding a
PREVIOUS_MAJOR_VERSION environment variable and replacing the hardcoded "4."
with "{{ .Env.PREVIOUS_MAJOR_VERSION }}.{{ .Env.PREVIOUS_MINOR_VERSION }}" (or
equivalent concatenation) so the parent reference becomes major-aware and
symmetric with PREVIOUS_MINOR_VERSION.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eebcb37-90f9-4b1c-bd40-b2e7343c6317
📒 Files selected for processing (9)
test/assets/common_versions.sh.templatetest/bin/build_images.shtest/bin/common.shtest/bin/common_versions.shtest/bin/pyutils/build_bootc_images.pytest/bin/pyutils/generate_common_versions.pytest/image-blueprints-bootc/el10/layer2-presubmit/group2/rhel102-bootc-source-fake-next-minor.containerfiletest/image-blueprints-bootc/el9/layer2-presubmit/group2/rhel98-bootc-source-fake-next-minor.containerfiletest/image-blueprints/layer2-presubmit/group1/rhel98-source-fake-next-minor.toml
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
pacevedom
force-pushed
the
rebase-bbad4d6f8-5.0.0-0.nightly-2026-04-16-172425_amd64-2026-04-16_arm64-2026-04-16
branch
from
8cc368b to
c8a39a8
Compare
pacevedom
force-pushed
the
rebase-bbad4d6f8-5.0.0-0.nightly-2026-04-16-172425_amd64-2026-04-16_arm64-2026-04-16
branch
from
c8a39a8 to
5137c1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/bin/pyutils/generate_common_versions.py`:
- Around line 352-356: The fallback grep path currently uses subprocess.run(...,
check=True) which will raise CalledProcessError if grep finds no match and
converts a previously soft empty gitops_version into a hard crash; update the
block that runs when gitops_version is falsy (after get_gitops_version calls) to
run grep with check=False (or wrap subprocess.run in try/except
CalledProcessError), safely handle an empty stdout (keep gitops_version as empty
or preserve a prior value), and change the logging message in that branch to
neutral wording (e.g., "Falling back to local common_versions.sh, resulting
GITOPS_VERSION=<value_or_empty>") so it no longer implies an API failure; refer
to the gitops_version variable and the subprocess.run invocation in this section
to locate and fix the code.
🪄 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: Pro Plus
Run ID: dd45b87d-4dda-4df0-8ab0-6b7fa6ee4e8d
📒 Files selected for processing (8)
test/assets/common_versions.sh.templatetest/bin/build_images.shtest/bin/common_versions.shtest/bin/pyutils/build_bootc_images.pytest/bin/pyutils/generate_common_versions.pytest/image-blueprints-bootc/el10/layer2-presubmit/group2/rhel102-bootc-source-fake-next-minor.containerfiletest/image-blueprints-bootc/el9/layer2-presubmit/group2/rhel98-bootc-source-fake-next-minor.containerfiletest/image-blueprints/layer2-presubmit/group1/rhel98-source-fake-next-minor.toml
✅ Files skipped from review due to trivial changes (2)
- test/image-blueprints-bootc/el9/layer2-presubmit/group2/rhel98-bootc-source-fake-next-minor.containerfile
- test/image-blueprints/layer2-presubmit/group1/rhel98-source-fake-next-minor.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- test/image-blueprints-bootc/el10/layer2-presubmit/group2/rhel102-bootc-source-fake-next-minor.containerfile
- test/bin/build_images.sh
- test/bin/pyutils/build_bootc_images.py
- test/assets/common_versions.sh.template
| if not gitops_version: | ||
| target_file = pathlib.Path(__file__).resolve().parent / '../common_versions.sh' | ||
| args = ['grep', '-oP', '(?<=GITOPS_VERSION=).*', str(target_file)] | ||
| gitops_version = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, check=True).stdout.strip() | ||
| logging.info(f"API fetch failed, preserving existing GITOPS_VERSION={gitops_version}") |
There was a problem hiding this comment.
Fallback grep can crash the run; log message is also slightly misleading.
Switching the guard to if not gitops_version: now routes empty-string returns from get_gitops_version (lines 279, 293 — e.g. no OCP-compatible GitOps version found after 4 Y-1 hops) through the grep path. Two small gotchas here:
subprocess.run(..., check=True)raisesCalledProcessErrorwhengrepfinds no match (exit 1), turning a previously soft "" outcome into a hard crash of the whole generator. A missing/renamedGITOPS_VERSION=line incommon_versions.shwould take the whole PR workflow down with it.- The log says
"API fetch failed, preserving existing GITOPS_VERSION=...", but we reach this branch even when the API succeeded and simply returned no compatible version — worth wording more neutrally.
🛡️ Proposed hardening
if not gitops_version:
target_file = pathlib.Path(__file__).resolve().parent / '../common_versions.sh'
args = ['grep', '-oP', '(?<=GITOPS_VERSION=).*', str(target_file)]
- gitops_version = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, check=True).stdout.strip()
- logging.info(f"API fetch failed, preserving existing GITOPS_VERSION={gitops_version}")
+ result = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, check=False)
+ gitops_version = result.stdout.strip() if result.returncode == 0 else ""
+ logging.info(f"Could not resolve GITOPS_VERSION from API; preserving existing GITOPS_VERSION={gitops_version!r}")🧰 Tools
🪛 Ruff (0.15.10)
[error] 355-355: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/bin/pyutils/generate_common_versions.py` around lines 352 - 356, The
fallback grep path currently uses subprocess.run(..., check=True) which will
raise CalledProcessError if grep finds no match and converts a previously soft
empty gitops_version into a hard crash; update the block that runs when
gitops_version is falsy (after get_gitops_version calls) to run grep with
check=False (or wrap subprocess.run in try/except CalledProcessError), safely
handle an empty stdout (keep gitops_version as empty or preserve a prior value),
and change the logging message in that branch to neutral wording (e.g., "Falling
back to local common_versions.sh, resulting GITOPS_VERSION=<value_or_empty>") so
it no longer implies an API failure; refer to the gitops_version variable and
the subprocess.run invocation in this section to locate and fix the code.
|
/retest |
|
/test ? |
|
/test e2e-aws-tests-cache |
2 similar comments
|
/test e2e-aws-tests-cache |
|
/test e2e-aws-tests-cache |
|
/test e2e-aws-tests-bootc-periodic-el9 |
|
/test e2e-aws-tests-bootc-arm-el10 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom, pmtk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@pacevedom: This PR has been marked as verified by 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. |
|
@pacevedom: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
pacevedom
deleted the
rebase-bbad4d6f8-5.0.0-0.nightly-2026-04-16-172425_amd64-2026-04-16_arm64-2026-04-16
branch
3 participants
Summary by CodeRabbit