ROX-35662: Support konflux deployments on non-OCP#232
Conversation
…E_* env vars This removes the OpenShift-only ImageContentSourcePolicy mechanism and instead populates RELATED_IMAGE_* environment variables on the operator deployment, enabling --konflux on any cluster type (GKE, Kind, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds Konflux operator image support, populates ChangesKonflux operator image support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant deployOperatorNonOLM
participant createDeploymentFromCSV
participant managerContainerFromPodSpec
participant rewriteKonfluxOperatorImage
participant injectEnvVarsIntoManagerContainer
deployOperatorNonOLM->>createDeploymentFromCSV: build Deployment from CSV
createDeploymentFromCSV->>managerContainerFromPodSpec: locate manager container
createDeploymentFromCSV->>rewriteKonfluxOperatorImage: replace image when Konflux is enabled
createDeploymentFromCSV->>injectEnvVarsIntoManagerContainer: merge operator env vars
createDeploymentFromCSV-->>deployOperatorNonOLM: Deployment ready for apply
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/deployer/operator.go (1)
490-507: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting a shared
findManagerContainerhelper.
rewriteKonfluxOperatorImageandinjectEnvVarsIntoManagerContainer(lines 455-487) share identical container-finding logic: iterate containers, type-assert tomap[string]interface{}, comparecontainer["name"]tomanagerContainerName, and return the same "not found" error. Extracting a helper would eliminate this duplication.♻️ Optional refactor: shared container-finding helper
+// findManagerContainer locates the manager container in the provided list. +func findManagerContainer(containers []interface{}) (map[string]interface{}, error) { + for _, c := range containers { + container, ok := c.(map[string]interface{}) + if !ok { + continue + } + if container["name"] == managerContainerName { + return container, nil + } + } + return nil, fmt.Errorf("container %q not found in deployment", managerContainerName) +} + // rewriteKonfluxOperatorImage replaces the manager container's image with the // Konflux-built operator image. func (d *Deployer) rewriteKonfluxOperatorImage(containers []interface{}) error { - for _, c := range containers { - container, ok := c.(map[string]interface{}) - if !ok { - continue - } - if container["name"] != managerContainerName { - continue - } + container, err := findManagerContainer(containers) + if err != nil { + return err + } newImage := KonfluxOperatorImage(&d.config) d.logger.Infof("Rewriting operator image to %s", newImage) container["image"] = newImage return nil - } - return fmt.Errorf("container %q not found in deployment", managerContainerName) }Similarly,
injectEnvVarsIntoManagerContainercould use the same helper.🤖 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 `@internal/deployer/operator.go` around lines 490 - 507, The container lookup logic in rewriteKonfluxOperatorImage is duplicated with injectEnvVarsIntoManagerContainer, so extract a shared findManagerContainer helper that iterates the containers, type-asserts to map[string]interface{}, matches managerContainerName, and returns the container plus the standard not-found error. Update rewriteKonfluxOperatorImage to use this helper (and optionally injectEnvVarsIntoManagerContainer as well) so the lookup and error handling live in one place.
🤖 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.
Nitpick comments:
In `@internal/deployer/operator.go`:
- Around line 490-507: The container lookup logic in rewriteKonfluxOperatorImage
is duplicated with injectEnvVarsIntoManagerContainer, so extract a shared
findManagerContainer helper that iterates the containers, type-asserts to
map[string]interface{}, matches managerContainerName, and returns the container
plus the standard not-found error. Update rewriteKonfluxOperatorImage to use
this helper (and optionally injectEnvVarsIntoManagerContainer as well) so the
lookup and error handling live in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: f79a95b1-c04e-4fed-9904-663702b3a163
📒 Files selected for processing (4)
cmd/deploy.gointernal/deployer/konflux.gointernal/deployer/konflux_test.gointernal/deployer/operator.go
|
@coderabbitai, I have addressed your nit comment, please check again. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 232 --repo stackrox/roxie --json comments,reviews -q '.reviews[] | select(.author.login=="coderabbitai") | .body' | head -100
echo "-----review comments-----"
gh api repos/stackrox/roxie/pulls/232/comments --jq '.[] | select(.user.login=="coderabbitai-app[bot]" or .user.login=="coderabbitai") | {id, body, path, line, diff_hunk}'Length of output: 3865 🏁 Script executed: #!/bin/bash
ast-grep outline internal/deployer/operator.go --items all --match 'findManagerContainer|rewriteKonfluxOperatorImage|injectEnvVarsIntoManagerContainer'
echo "-----"
rg -n 'findManagerContainer|rewriteKonfluxOperatorImage|injectEnvVarsIntoManagerContainer' internal/deployer/operator.goLength of output: 1081 🏁 Script executed: #!/bin/bash
sed -n '380,510p' internal/deployer/operator.goLength of output: 4405 Confirmed — the nit has been addressed. The container-lookup logic is now consolidated into the single
|
mclasmeier
changed the title
porridge
left a comment
porridge
left a comment
There was a problem hiding this comment.
It would also be nice to be a bit more generic than "konflux/non-konflux", i.e. think of stackrox-io or custom image repos, but maybe it's material for another PR.
2 participants
Replace ICSP-based Konflux image rewriting with operator RELATED_IMAGE_* env vars
This removes the OpenShift-only ImageContentSourcePolicy mechanism and instead populates RELATED_IMAGE_* environment variables on the operator deployment, enabling --konflux on any cluster type (GKE, Kind, etc.).
@porridge This feature was discussed in the release sync meetings. GKE is just so much quicker. Builds on top of your feature for setting env vars in operator.