Skip to content

rhwa/sbr: update RELATED_IMAGE_SBR_AGENT only if missing #79603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
clobrano wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −4
Open

rhwa/sbr: update RELATED_IMAGE_SBR_AGENT only if missing #79603

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions ...onfig/medik8s/storage-based-remediation/medik8s-storage-based-remediation-main__4.20.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,30 @@ tests:
done
[[ -n "$CSV_NAME" ]] || { echo "CSV not found in namespace ${OPERATOR_NAMESPACE}"; exit 1; }

# Ensure env array exists before appending
# Ensure env array exists before patching
if ! oc get csv "$CSV_NAME" -n "$OPERATOR_NAMESPACE" -o jsonpath='{.spec.install.spec.deployments[0].spec.template.spec.containers[0].env[0].name}' >/dev/null 2>&1; then
oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[
{"op":"add","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env","value":[]}
]'
fi
oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[
{"op":"add","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/-","value":{"name":"RELATED_IMAGE_SBR_AGENT","value":"'"${SBR_AGENT_IMAGE}"'"}}
]'

# Check if RELATED_IMAGE_SBR_AGENT already exists in the env array
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}"'"}
]'
Comment on lines +74 to +82
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.

Copy link
Copy Markdown
Contributor Author

@clobrano clobrano May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If more than one entry exists the CSV is invalid. This change is made exactly to avoid adding duplicates.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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.

else
# Add new env variable
echo "Adding new RELATED_IMAGE_SBR_AGENT"
oc -n "$OPERATOR_NAMESPACE" patch csv "$CSV_NAME" --type=json -p='[
{"op":"add","path":"/spec/install/spec/deployments/0/spec/template/spec/containers/0/env/-","value":{"name":"RELATED_IMAGE_SBR_AGENT","value":"'"${SBR_AGENT_IMAGE}"'"}}
]'
fi
echo "CSV patched, waiting for OLM to process and trigger rollout..."
sleep 30
oc rollout status deployment/sbr-operator-controller-manager \
Expand Down