OLS-1829: Detect change in specified BYOK images by syedriko · Pull Request #1171 · openshift/lightspeed-operator

syedriko · 2025-12-04T05:13:10Z

Description

Implemented change detection for BYOK images.

Type of change

Related Tickets & Documents

Related Issue #
Closes # https://issues.redhat.com/browse/OLS-1829

Checklist before requesting a review

I have performed a self-review of my code.
PR has passed all pre-merge test jobs.
If it is a core feature, I have added thorough tests.

Testing

Please provide detailed steps to perform tests related to this code change.
How were the fix/results from this change verified? Please provide relevant screenshots or results.

openshift-ci-robot · 2025-12-04T05:13:14Z

openshift-ci · 2025-12-04T05:13:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign raptorsun for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

syedriko · 2025-12-06T17:34:56Z

/retest

syedriko · 2025-12-07T02:28:39Z

/retest

syedriko · 2025-12-07T04:32:19Z

/retest

syedriko · 2025-12-09T02:36:50Z

/test bundle-e2e-4-20

syedriko · 2025-12-17T20:29:35Z

/retest

syedriko · 2025-12-18T01:32:45Z

/retest

syedriko · 2025-12-18T13:57:44Z

/retest

syedriko · 2025-12-18T21:51:37Z

/test bundle-e2e-4-20

syedriko · 2025-12-19T00:52:31Z

/retest

openshift-ci · 2026-02-16T21:18:39Z

@syedriko: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	`62151b2`	link	true	`/test lint`

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

blublinsky · 2026-02-18T12:52:28Z

internal/controller/appserver/rag.go

+		isName := utils.ImageStreamNameFor(rag.Image)
+		initContainerName := fmt.Sprintf("rag-%d", idx)
+		triggers = append(triggers, fmt.Sprintf(`{"from":{"kind":"ImageStreamTag","name":"%s:latest"},"fieldPath":"spec.template.spec.initContainers[?(@.name==\"%s\")].image"}`, isName, initContainerName))
+	}


Problem: Line 56 builds JSON as a string using fmt.Sprintf(). This is fragile and error-prone.

The better option is
// Add struct definitions at top of file
type ImageTrigger struct {
From ImageTriggerFrom json:"from"
FieldPath string json:"fieldPath"
}

type ImageTriggerFrom struct {
Kind string json:"kind"
Name string json:"name"
}

// Updated function
func generateImageStreamTriggers(cr *olsv1alpha1.OLSConfig) (string, error) {
var triggers []ImageTrigger
for idx, rag := range cr.Spec.OLSConfig.RAG {
isName := utils.ImageStreamNameFor(rag.Image)
initContainerName := fmt.Sprintf("rag-%d", idx)
triggers = append(triggers, ImageTrigger{
From: ImageTriggerFrom{
Kind: "ImageStreamTag",
Name: fmt.Sprintf("%s:latest", isName),
},
FieldPath: fmt.Sprintf(spec.template.spec.initContainers[?(@.name=="%s")].image, initContainerName),
})
}
data, err := json.Marshal(triggers)
if err != nil {
return "", fmt.Errorf("marshal image triggers: %w", err)
}
return string(data), nil
}

blublinsky · 2026-02-18T12:55:31Z

internal/controller/appserver/deployment.go

 			deployment.Spec.Template.Spec.ImagePullSecrets = cr.Spec.OLSConfig.ImagePullSecrets
 		}
+		deployment.Annotations[utils.OLSAppServerImageStreamTriggerAnnotation] = generateImageStreamTriggers(cr)
 	}


this requires error handling

triggers, err := generateImageStreamTriggers(cr)
if err != nil {
return nil, fmt.Errorf("generate image stream triggers: %w", err)
}
deployment.Annotations[utils.OLSAppServerImageStreamTriggerAnnotation] = triggers

blublinsky · 2026-02-18T13:45:27Z

internal/controller/utils/utils.go


-func DeploymentSpecEqual(a, b *appsv1.DeploymentSpec) bool {
+func DeploymentSpecEqual(a, b *appsv1.DeploymentSpec, compareInitContainers bool) bool {
 	if !apiequality.Semantic.DeepEqual(a.Template.Spec.NodeSelector, b.Template.Spec.NodeSelector) || // check node selector


This looks like a breaking change in the API
Can you make sure it does not break anything else

blublinsky · 2026-02-18T13:47:55Z

internal/controller/appserver/reconciler.go

+	for _, rag := range cr.Spec.OLSConfig.RAG {
+		ragImages = append(ragImages, rag.Image)
+	}
+


Do we need images validation here?

blublinsky · 2026-02-18T13:52:05Z

internal/controller/utils/utils.go

+	sum := sha1.Sum([]byte(image)) //nolint:gosec
+	sfx := hex.EncodeToString(sum[:])[:6]
+	return fmt.Sprintf("%s-%s", slug, sfx)
+}


Function performs multiple string replacements and regex operations without explaining or documenting the Kubernetes name constraints it's trying to satisfy.

At least we need comments what we are doing here and why
We also need unit tests for this function

blublinsky · 2026-02-18T13:56:01Z

internal/controller/appserver/reconciler.go

+				if err := r.Update(ctx, updated); err != nil {
+					return fmt.Errorf("update ImageStream %q: %w", name, err)
+				}
+			}


This code seems suspicious

blublinsky · 2026-02-18T13:58:53Z

internal/controller/appserver/rag_test.go

+				err = testReconcilerInstance.Get(ctx, types.NamespacedName{Name: utils.ImageStreamNameFor(rag.Image), Namespace: testReconcilerInstance.GetNamespace()}, &is)
+				Expect(err).NotTo(HaveOccurred())
+			}
+		})


Missing Test Coverage:
No tests for generateImageStreamTriggers() - the JSON generation function
No tests for ImageStreamNameFor() - the name generation function
No tests for edge cases:
Multiple RAG entries with same image (deduplication)
Invalid image references
ImageStream update scenarios
Orphaned ImageStream cleanup
Empty RAG list

blublinsky · 2026-02-18T14:00:29Z

test/e2e/byok_test.go

+		Expect(appServerDeployment.Spec.Template.Spec.InitContainers[0].Image).To(
+			Equal(internalRegistyHostName + "/" + utils.OLSNamespaceDefault + "/" + imageName + "@" + digest),
+		)
+	})


Add documentation on this?

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 4, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2025

openshift-ci bot requested review from bparees and xrajesh December 4, 2025 05:13

syedriko force-pushed the syedriko-ols-1829 branch from 3db7729 to 1c15fa8 Compare December 4, 2025 19:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2025

syedriko force-pushed the syedriko-ols-1829 branch 11 times, most recently from 09a7fb1 to ba9750b Compare December 6, 2025 17:29

syedriko force-pushed the syedriko-ols-1829 branch 3 times, most recently from 1ced66f to dae62eb Compare December 7, 2025 02:23

syedriko force-pushed the syedriko-ols-1829 branch from dae62eb to 99dad6c Compare December 7, 2025 04:04

syedriko force-pushed the syedriko-ols-1829 branch 3 times, most recently from 6e38915 to 26719f6 Compare December 10, 2025 22:13

syedriko force-pushed the syedriko-ols-1829 branch from 49c08c2 to 3374dec Compare December 17, 2025 20:18

syedriko force-pushed the syedriko-ols-1829 branch 2 times, most recently from 6a6fbf0 to 68a1b49 Compare December 17, 2025 22:19

syedriko force-pushed the syedriko-ols-1829 branch 2 times, most recently from ca383ff to 653aaca Compare December 18, 2025 04:34

syedriko force-pushed the syedriko-ols-1829 branch from 653aaca to 8a4bf20 Compare December 18, 2025 18:28

syedriko force-pushed the syedriko-ols-1829 branch from 8a4bf20 to c65bb52 Compare December 18, 2025 22:08

syedriko force-pushed the syedriko-ols-1829 branch from c65bb52 to 1ae0320 Compare January 20, 2026 23:15

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2026

syedriko force-pushed the syedriko-ols-1829 branch from 1ae0320 to da2c048 Compare January 20, 2026 23:28

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 20, 2026

syedriko force-pushed the syedriko-ols-1829 branch from da2c048 to 075115a Compare February 4, 2026 19:19

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2026

syedriko force-pushed the syedriko-ols-1829 branch from 075115a to 55f171f Compare February 12, 2026 17:00

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2026

syedriko force-pushed the syedriko-ols-1829 branch from 55f171f to c32aa64 Compare February 12, 2026 17:26

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2026

syedriko force-pushed the syedriko-ols-1829 branch 2 times, most recently from 6281413 to c01d74e Compare February 16, 2026 17:25

OLS-1829: Detect change in specified BYOK images

62151b2

syedriko force-pushed the syedriko-ols-1829 branch from c01d74e to 62151b2 Compare February 16, 2026 19:54

xrajesh requested a review from blublinsky February 17, 2026 18:41

blublinsky reviewed Feb 18, 2026

View reviewed changes

Conversation

syedriko commented Dec 4, 2025

Description

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Uh oh!

openshift-ci-robot commented Dec 4, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Uh oh!

openshift-ci bot commented Dec 4, 2025

Uh oh!

syedriko commented Dec 6, 2025

Uh oh!

syedriko commented Dec 7, 2025

Uh oh!

syedriko commented Dec 7, 2025

Uh oh!

syedriko commented Dec 9, 2025

Uh oh!

syedriko commented Dec 17, 2025

Uh oh!

syedriko commented Dec 18, 2025

Uh oh!

syedriko commented Dec 18, 2025

Uh oh!

syedriko commented Dec 18, 2025

Uh oh!

syedriko commented Dec 19, 2025

Uh oh!

openshift-ci bot commented Feb 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

openshift-ci-robot commented Dec 4, 2025 •

edited by openshift-ci bot

Loading