OLS-3522: Generate operator deployment args from related_images.json#1806
OLS-3522: Generate operator deployment args from related_images.json#1806blublinsky wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-3522 which is a valid 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. |
📝 WalkthroughWalkthroughChangesRelated image wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant related_images.json
participant Makefile
participant image_args_lib.sh
participant deployment-patch.yaml
participant hack/update_bundle.sh
participant CSV
related_images.json->>Makefile: provide image and operator metadata
Makefile->>image_args_lib.sh: generate or substitute deployment patch
image_args_lib.sh->>deployment-patch.yaml: write resolved patch structure
hack/update_bundle.sh->>image_args_lib.sh: read patch entries
image_args_lib.sh->>CSV: supply image and argument substitutions
hack/update_bundle.sh->>CSV: update relatedImages entries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
added
the
needs-rebase
blublinsky
force-pushed
the
related-images-pipeline
branch
from
0d346a4 to
4487cec
Compare
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.ai/spec/what/bundle-composition.md:
- Line 40: Correct the documented GetDefaultImage location in the bundle
composition description: reference internal/relatedimages/loader.go as its
implementation and note that internal/controller/utils/constants.go invokes it
through imageDefaultOr, replacing the inaccurate constants.go reference.
In `@docs/olm-bundle-management.md`:
- Around line 127-129: Add a language identifier to the workflow code fence in
the documentation, changing the opening fence to ```text (or another suitable
language) while preserving the workflow content.
In `@hack/snapshot_to_image_list.sh`:
- Around line 3-4: Update the comment above the loop to document both
related-image wiring fields, operator_arg and operator_target, or explicitly
state that the script ignores all wiring metadata and only updates
image/revision.
In `@hack/update_bundle.sh`:
- Around line 141-150: The loop uses the lowercase placeholder variable, but
both yq sub() expressions reference the incorrect uppercase PLACEHOLDER
variable. Update the image and argument replacement commands inside the loop
reading image_args::list_patch_entries to interpolate ${placeholder}, ensuring
the corresponding CSV placeholders are replaced.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f8aa005-8d82-4959-baa2-64c4d653d9ca
⛔ Files ignored due to path filters (1)
related_images.jsonis excluded by!related_images.json
📒 Files selected for processing (12)
.ai/spec/what/bundle-composition.mdMakefileconfig/default/deployment-patch.yamlconfig/default/kustomization.yamldocs/olm-bundle-management.mdhack/generate_deployment_patch.shhack/image_args_lib.shhack/image_placeholders.jsonhack/install/install-operator-direct.shhack/snapshot_to_image_list.shhack/update_bundle.shinternal/controller/utils/constants.go
💤 Files with no reviewable changes (1)
- hack/image_placeholders.json
|
|
||
| 16. The lightspeed-operator reconciles the agentic console plugin as a fully managed operand: Phase 1/2 reconciliation, `AgenticConsolePluginReady` status condition, health monitoring, and finalizer cleanup via `RemoveAgenticConsole()`. The lightspeed-operator reconciles the agentic alerts adapter as a fully managed operand (OLS-3348, opt-in via `spec.ols.deployment.alertsAdapter.configMapRef`): Phase 1/2 reconciliation when enabled, `AlertsAdapterReady` status condition (`NotConfigured` when disabled), health monitoring, operand teardown on disable, ConfigMap watcher restarts, and finalizer cleanup via `RemoveAlertsAdapter()`. | ||
| 17. Agentic operand images default from `related_images.json` (via `GetDefaultImage` in `constants.go`) and are passed to the operator through CSV deployment args: `--agentic-console-image` (`lightspeed-agentic-console-plugin`) and `--alerts-adapter-image` (`lightspeed-agentic-alerts-adapter`). Images are substituted by `hack/update_bundle.sh` from `hack/image_placeholders.json` and `config/default/deployment-patch.yaml`. Until Konflux productizes both operands, images may use interim Konflux `:main` or other non-`registry.redhat.io` references. | ||
| 17. Operand images default from `related_images.json` (via `GetDefaultImage` in `constants.go`) and are passed to the operator through CSV deployment args defined by `operator_arg` on each `related_images.json` entry. `config/default/deployment-patch.yaml` is generated from that file (`make manifests`); `hack/update_bundle.sh` substitutes digests at bundle time. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documented GetDefaultImage location.
GetDefaultImage is implemented in internal/relatedimages/loader.go; internal/controller/utils/constants.go invokes it through imageDefaultOr. Update the sentence to avoid sending maintainers to the wrong file.
As per path instructions, Markdown architecture descriptions must be cross-checked against the current implementation.
🤖 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 @.ai/spec/what/bundle-composition.md at line 40, Correct the documented
GetDefaultImage location in the bundle composition description: reference
internal/relatedimages/loader.go as its implementation and note that
internal/controller/utils/constants.go invokes it through imageDefaultOr,
replacing the inaccurate constants.go reference.
Source: Path instructions
| **Workflow:** | ||
| ``` | ||
| related_images.json → hack/update_bundle.sh → CSV relatedImages → CSV deployment args → Controller → Operand deployments | ||
| related_images.json → make manifests (deployment-patch.yaml) → hack/update_bundle.sh → CSV relatedImages + deployment args → Controller → Operand deployments |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the workflow code fence.
Use ```text (or another appropriate language) so the documentation passes markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/olm-bundle-management.md` around lines 127 - 129, Add a language
identifier to the workflow code fence in the documentation, changing the opening
fence to ```text (or another suitable language) while preserving the workflow
content.
Source: Linters/SAST tools
| # Component wiring (snapshot name, Konflux quay prefix, product registry prefix, operator_arg) lives in | ||
| # related_images.json optional fields; this script only loops those entries and updates image/revision. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both related-image wiring fields.
The contract includes both operator_arg and operator_target, but this comment names only operator_arg. Include both fields, or explicitly state that this script ignores wiring metadata and only updates image/revision.
🤖 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 `@hack/snapshot_to_image_list.sh` around lines 3 - 4, Update the comment above
the loop to document both related-image wiring fields, operator_arg and
operator_target, or explicitly state that the script ignores all wiring metadata
and only updates image/revision.
| while IFS='|' read -r name placeholder target _; do | ||
| IMG=$(${JQ} -r --arg n "${name}" '.[] | select(.name==$n) | .image' <<<"${RELATED_IMAGES}") | ||
| [ -z "${IMG}" ] || [ "${IMG}" = "null" ] && continue | ||
| # Escape double quotes for use inside yq double-quoted string | ||
| IMG_SAFE=$(printf '%s' "${IMG}" | sed 's/"/\\"/g') | ||
| if [ "${TARGET}" = "image" ]; then | ||
| if [ "${target}" = "image" ]; then | ||
| ${YQ} "(.spec.install.spec.deployments[].spec.template.spec.containers[].image |= sub(\"${PLACEHOLDER}\", \"${IMG_SAFE}\"))" -i ${CSV_FILE} | ||
| else | ||
| ${YQ} "(.spec.install.spec.deployments[].spec.template.spec.containers[].args[] |= sub(\"${PLACEHOLDER}\", \"${IMG_SAFE}\"))" -i ${CSV_FILE} | ||
| fi | ||
| done | ||
| done < <(image_args::list_patch_entries "${RELATED_IMAGES_FILENAME}" "${JQ}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'PLACEHOLDER|placeholder' hack/update_bundle.shRepository: openshift/lightspeed-operator
Length of output: 520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- hack/update_bundle.sh (top 220 lines) ---'
cat -n hack/update_bundle.sh | sed -n '1,220p'
echo
echo '--- PLACEHOLDER assignments/usages in repository ---'
rg -n '(^|[^A-Za-z0-9_])PLACEHOLDER=|export +PLACEHOLDER|PLACEHOLDER[^A-Za-z0-9_]' hack/update_bundle.sh . -g '!**/vendor/**'Repository: openshift/lightspeed-operator
Length of output: 8177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- hack/image_args_lib.sh outline ---'
ast-grep outline hack/image_args_lib.sh --view expanded || true
echo
echo '--- relevant functions/lines ---'
rg -n 'list_patch_entries|PLACEHOLDER|placeholder' hack/image_args_lib.sh hack/update_bundle.shRepository: openshift/lightspeed-operator
Length of output: 2251
Use the loop's placeholder variable here. image_args::list_patch_entries feeds the placeholder into placeholder, but the yq sub() calls read ${PLACEHOLDER} instead. That leaves the pattern empty/unset, so the CSV placeholders are not replaced with the related image values.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 141-141: placeholder appears unused. Verify use (or export if used externally).
(SC2034)
[info] 142-142: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
[info] 146-146: Possible misspelling: PLACEHOLDER may not be assigned. Did you mean placeholder?
(SC2153)
🤖 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 `@hack/update_bundle.sh` around lines 141 - 150, The loop uses the lowercase
placeholder variable, but both yq sub() expressions reference the incorrect
uppercase PLACEHOLDER variable. Update the image and argument replacement
commands inside the loop reading image_args::list_patch_entries to interpolate
${placeholder}, ensuring the corresponding CSV placeholders are replaced.
Source: Linters/SAST tools
|
@blublinsky: 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. |
2 participants
Description
Summary
related_images.jsonthe single source of truth for operator operand image wiring via new optional fields:operator_arg(CLI flag) andoperator_target: "image"(operator container).config/default/deployment-patch.yamlfromrelated_images.jsonusinghack/generate_deployment_patch.sh, hooked intomake manifests/make bundle.hack/image_placeholders.jsonand consolidate substitution logic inhack/image_args_lib.shformake deploy,hack/update_bundle.sh, andhack/install/install-operator-direct.sh.operator_arg/operator_targetfrom CSVspec.relatedImagesso OLM metadata stays name+image only.Closes OLS-3522
Motivation
Operand images were wired in three places (
related_images.json,image_placeholders.json,deployment-patch.yaml). Adding or removing an image required coordinated manual edits; stale flags like--ocp-rag-imagecould linger after an operand was retired.Type of change
Related Tickets & Documents
https://redhat.atlassian.net/browse/OLS-3522
https://redhat.atlassian.net/browse/OLS-3522
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Bug Fixes