rhwa/sbr: update RELATED_IMAGE_SBR_AGENT only if missing#79603
Open
clobrano wants to merge 1 commit into
Open
Conversation
Modify the install-last-operator-version test step to check if RELATED_IMAGE_SBR_AGENT already exists in the CSV environment variables before adding it. If it exists, update the value; if not, add it as a new variable. This prevents duplicate environment variable entries and ensures the CSV always has the correct CI-built agent image without accumulating stale copies. Co-Authored-By: Claude
Contributor
WalkthroughThe pull request updates a CI operator configuration file to make environment variable injection idempotent. The ChangesCI operator CSV patching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clobrano 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 |
openshift-ci
Bot
added
the
approved
Contributor
Author
|
/pj-rehearse auto-ack |
Contributor
|
@clobrano: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/config/medik8s/storage-based-remediation/medik8s-storage-based-remediation-main__4.20.yaml`:
- Around line 74-82: ENV_INDEX may contain multiple lines if more than one
RELATED_IMAGE_SBR_AGENT exists, which breaks the JSON patch path; instead of
treating ENV_INDEX as a single value, iterate over each index returned by the jq
expression and apply the oc patch csv for each index separately (using the same
CSV_NAME, OPERATOR_NAMESPACE and SBR_AGENT_IMAGE variables) so each matching env
entry is updated individually; implement the loop where ENV_INDEX is produced
(replace the single oc patch call) and ensure you quote/trim each index before
using it in the patch path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 77417310-0f6f-490c-9d3c-cd669686eae9
📒 Files selected for processing (1)
ci-operator/config/medik8s/storage-based-remediation/medik8s-storage-based-remediation-main__4.20.yaml
Comment on lines
+74
to
+82
| ENV_INDEX=$(oc get csv "$CSV_NAME" -n "$OPERATOR_NAMESPACE" -o json | \ | ||
| jq -r '.spec.install.spec.deployments[0].spec.template.spec.containers[0].env | to_entries | .[] | select(.value.name=="RELATED_IMAGE_SBR_AGENT") | .key') | ||
|
|
||
| if [[ -n "$ENV_INDEX" ]]; then | ||
| # Update existing env variable | ||
| echo "Updating existing RELATED_IMAGE_SBR_AGENT at index ${ENV_INDEX}" | ||
| oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[ | ||
| {"op":"replace","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/'"${ENV_INDEX}"'/value","value":"'"${SBR_AGENT_IMAGE}"'"} | ||
| ]' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
json='{"spec":{"install":{"spec":{"deployments":[{"spec":{"template":{"spec":{"containers":[{"env":[{"name":"RELATED_IMAGE_SBR_AGENT","value":"old-a"},{"name":"OTHER","value":"x"},{"name":"RELATED_IMAGE_SBR_AGENT","value":"old-b"}]}]}}}}]}}}}'
ENV_INDEX=$(jq -r '.spec.install.spec.deployments[0].spec.template.spec.containers[0].env
| to_entries | .[] | select(.value.name=="RELATED_IMAGE_SBR_AGENT") | .key' <<<"$json")
printf 'ENV_INDEX contains:\n%s\n' "$ENV_INDEX"
printf 'Resulting path would be:\n/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/%s/value\n' "$ENV_INDEX"Repository: openshift/release
Length of output: 189
Handle multiple matching env indices to avoid invalid JSON patch paths
If more than one RELATED_IMAGE_SBR_AGENT entry exists, the jq expression returns multiple indices (e.g., 0 and 2), so ENV_INDEX becomes multi-line (0\n2). That makes the JSON patch path invalid (e.g., /.../env/0\n2/value) and the patch can fail instead of converging.
Suggested fix
- ENV_INDEX=$(oc get csv "$CSV_NAME" -n "$OPERATOR_NAMESPACE" -o json | \
- jq -r '.spec.install.spec.deployments[0].spec.template.spec.containers[0].env | to_entries | .[] | select(.value.name=="RELATED_IMAGE_SBR_AGENT") | .key')
+ ENV_INDEXES=$(oc get csv "$CSV_NAME" -n "$OPERATOR_NAMESPACE" -o json | \
+ jq -r '(.spec.install.spec.deployments[0].spec.template.spec.containers[0].env // [])
+ | to_entries[]
+ | select(.value.name=="RELATED_IMAGE_SBR_AGENT")
+ | .key')
- if [[ -n "$ENV_INDEX" ]]; then
+ if [[ -n "$ENV_INDEXES" ]]; then
# Update existing env variable
- echo "Updating existing RELATED_IMAGE_SBR_AGENT at index ${ENV_INDEX}"
- oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[
- {"op":"replace","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/'"${ENV_INDEX}"'/value","value":"'"${SBR_AGENT_IMAGE}"'"}
- ]'
+ while IFS= read -r ENV_INDEX; do
+ [[ -z "$ENV_INDEX" ]] && continue
+ echo "Updating existing RELATED_IMAGE_SBR_AGENT at index ${ENV_INDEX}"
+ oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[
+ {"op":"replace","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/'"${ENV_INDEX}"'/value","value":"'"${SBR_AGENT_IMAGE}"'"}
+ ]'
+ done <<< "$ENV_INDEXES"📝 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
| ENV_INDEX=$(oc get csv "$CSV_NAME" -n "$OPERATOR_NAMESPACE" -o json | \ | |
| jq -r '.spec.install.spec.deployments[0].spec.template.spec.containers[0].env | to_entries | .[] | select(.value.name=="RELATED_IMAGE_SBR_AGENT") | .key') | |
| if [[ -n "$ENV_INDEX" ]]; then | |
| # Update existing env variable | |
| echo "Updating existing RELATED_IMAGE_SBR_AGENT at index ${ENV_INDEX}" | |
| oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[ | |
| {"op":"replace","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/'"${ENV_INDEX}"'/value","value":"'"${SBR_AGENT_IMAGE}"'"} | |
| ]' | |
| ENV_INDEXES=$(oc get csv "$CSV_NAME" -n "$OPERATOR_NAMESPACE" -o json | \ | |
| jq -r '(.spec.install.spec.deployments[0].spec.template.spec.containers[0].env // []) | |
| | to_entries[] | |
| | select(.value.name=="RELATED_IMAGE_SBR_AGENT") | |
| | .key') | |
| if [[ -n "$ENV_INDEXES" ]]; then | |
| # Update existing env variable | |
| while IFS= read -r ENV_INDEX; do | |
| [[ -z "$ENV_INDEX" ]] && continue | |
| echo "Updating existing RELATED_IMAGE_SBR_AGENT at index ${ENV_INDEX}" | |
| oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[ | |
| {"op":"replace","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/'"${ENV_INDEX}"'/value","value":"'"${SBR_AGENT_IMAGE}"'"} | |
| ]' | |
| done <<< "$ENV_INDEXES" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/config/medik8s/storage-based-remediation/medik8s-storage-based-remediation-main__4.20.yaml`
around lines 74 - 82, ENV_INDEX may contain multiple lines if more than one
RELATED_IMAGE_SBR_AGENT exists, which breaks the JSON patch path; instead of
treating ENV_INDEX as a single value, iterate over each index returned by the jq
expression and apply the oc patch csv for each index separately (using the same
CSV_NAME, OPERATOR_NAMESPACE and SBR_AGENT_IMAGE variables) so each matching env
entry is updated individually; implement the loop where ENV_INDEX is produced
(replace the single oc patch call) and ensure you quote/trim each index before
using it in the patch path.
Contributor
Author
There was a problem hiding this comment.
If more than one entry exists the CSV is invalid. This change is made exactly to avoid adding duplicates.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@clobrano, that's a fair point — a CSV with duplicate environment variable entries is already malformed, and handling that corrupted state is outside the scope of this PR. The idempotent logic here is designed precisely to prevent that from ever happening, so the single-index assumption is correct for any valid CSV. I'll dismiss this concern.
🏁 Script executed:
#!/bin/bash
# Resolve the CodeRabbit review comment on this PR
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOAk3rVc6F6GXo"}) {
thread {
isResolved
}
}
}'
Length of output: 302
🏁 Script executed:
#!/bin/bash
# List review threads on PR `#79603` to find the correct thread ID
gh api graphql -f query='
{
repository(owner: "openshift", name: "release") {
pullRequest(number: 79603) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}'
Length of output: 5449
🏁 Script executed:
#!/bin/bash
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOBHvUUM6Dzhw6"}) {
thread {
isResolved
}
}
}'
Length of output: 266
@clobrano, that's a valid point — a CSV with duplicate RELATED_IMAGE_SBR_AGENT entries would already be malformed, and this PR is specifically designed to prevent that situation. The single-index assumption is correct for any well-formed CSV. I'll dismiss my concern.
(I wasn't able to resolve the review thread programmatically due to permission restrictions, but you can resolve it manually on the PR.)
🧠 Learnings used
Learnt from: rdiscala
Repo: openshift/release PR: 79357
File: ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-sno-day2-worker-4.18.yaml:23-23
Timestamp: 2026-05-20T11:54:20.424Z
Learning: In the openshift/release repository CI operator configuration YAMLs, do not treat an intentionally impossible cron schedule (e.g., `0 0 31 2 *` for “Feb 31”) as a misconfiguration. This pattern is an intentional mechanism used to disable/park a periodic CI job temporarily (e.g., while an upstream bug is being fixed). Only flag cron syntax errors when the schedule is not an intentional “impossible date” disable pattern used for job parking.
Learnt from: dhaiducek
Repo: openshift/release PR: 79563
File: ci-operator/config/stolostron/cluster-proxy/stolostron-cluster-proxy-main.yaml:41-43
Timestamp: 2026-05-20T19:31:57.286Z
Learning: In the openshift/release repo, for ci-operator container test step config YAML files under ci-operator/config/, remember that the `commands` field is read by ci-operator at runtime from the config YAML and is not embedded into the generated ProwJob YAML under `ci-operator/jobs/`. Therefore, if a PR changes only `commands` (and not any ProwJob-spec affecting structural fields such as test/job names, job types/spec, or metadata that changes the resulting ProwJob spec), you generally do NOT need to run `make update` and regenerating artifacts should be a no-op diff. Run `make update` only when the changes affect the generated ProwJob spec (e.g., new/renamed tests, new job types, or other spec/metadata changes that alter `ci-operator/jobs/` output).
Learnt from: tthvo
Repo: openshift/release PR: 79564
File: ci-operator/config/openshift/multiarch/openshift-multiarch-main__nightly-4.20-upgrade-from-nightly-4.19.yaml:94-94
Timestamp: 2026-05-20T20:01:14.481Z
Learning: In the openshift/release repo, when editing ci-operator config YAMLs under ci-operator/config/, changes that only modify the *values* of environment variables inside test step `env` blocks (e.g., COMPUTE_NODE_TYPE, ADDITIONAL_WORKER_VM_TYPE, MIGRATION_INFRA_MACHINE_TYPE) do not change the generated ProwJob specs under ci-operator/jobs/. For these value-only env tweaks, reviewers should not require running `make update` because it should produce no diff. Require `make update` only for structural/semantic changes that can affect the generated ProwJob spec (e.g., adding/renaming tests, changing job types, or altering metadata/fields that feed into the ProwJob spec).
Learnt from: karampok
Repo: openshift/release PR: 79537
File: ci-operator/config/openshift/bond-cni/openshift-bond-cni-main.yaml:32-33
Timestamp: 2026-05-21T09:04:21.025Z
Learning: In openshift/release, when modifying a ci-operator config YAML under ci-operator/config/ by adding an `env` block (key/value environment variables such as `TEST_TYPE`, `TEST_SUITE`, `TEST_ARGS`) to an existing test step, this change does not affect the generated ProwJob specs under ci-operator/jobs/. In this case, `make update` is a no-op—do not request or flag that `make update` was (or needs to be) run for these env-only edits.
Contributor
|
@clobrano: 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. |
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
1 participant
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.
Modify the install-last-operator-version test step to check if RELATED_IMAGE_SBR_AGENT already exists in the CSV environment variables before adding it. If it exists, update the value; if not, add it as a new variable.
This prevents duplicate environment variable entries and ensures the CSV always has the correct CI-built agent image without accumulating stale copies.
Co-Authored-By: Claude
Summary by CodeRabbit
This PR modifies the CI configuration for the medik8s/storage-based-remediation operator's 4.20 test pipeline to make the
RELATED_IMAGE_SBR_AGENTenvironment variable injection idempotent.What's changing:
The
install-last-operator-versiontest step in theopenshift-e2ejob now intelligently handles theRELATED_IMAGE_SBR_AGENTCSV environment variable. Instead of unconditionally appending a new entry each time the test runs, the logic now:Why this matters:
Previously, repeated test runs could accumulate stale or duplicate
RELATED_IMAGE_SBR_AGENTentries in the Operator CSV. This change ensures the test is reentrant—running it multiple times produces the same result without data duplication, while always ensuring the CSV contains the correct CI-built agent image for OLM to propagate to deployments.Impact:
Only affects the medik8s/storage-based-remediation CI configuration for OpenShift 4.20. The changes are purely in test automation shell logic with no impact to the operator code itself.