code restructuring #1106

blublinsky · 2025-10-31T10:22:44Z

Description

This is a massive PR restructuring operator code in the individual components. Not so much code changes, but a lot of moving code around.
Additionally:

increased code coverage
add comprehensive documentation
improved local testing

Type of change

Related Tickets & Documents

Related Issue #
OLS-2251
Closes #
OLS-2251

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.

xrajesh · 2025-11-06T02:28:19Z

docs/olm-bundle-management.md

+| `OwnNamespace` | Operator watches its own namespace | Development/testing |
+| `SingleNamespace` | Operator watches one namespace | Namespace isolation |
+| `MultiNamespace` | Operator watches specific namespaces | Multi-tenant with selection |
+| `AllNamespaces` | Operator watches cluster-wide | Cluster-scoped resources (like Lightspeed) |


Openshift lightspeed supports only OwnNamespace - to watch resources on the namespace its installed on . @blublinsky .
Also are these auto generated files in the docs folder ?

Yes they are, but I went through and verified them. They are quite useful, at least for me

``AllNamespaces| Operator watches cluster-wide | Cluster-scoped resources (like Lightspeed) - that is not correct - OLS uses OwnNamespace --@blublinsky

add explanation

raptorsun · 2025-11-09T22:52:06Z

internal/controller/utils/testing.go

@@ -0,0 +1,102 @@
+package utils


this file will be compiled into the normal operator build.
These are test codes.
Let's rename it to something ending with _test.go postfix to make it only compiled into test build.

If renamed to testing_test.go:
Would only be compiled during testing (which is fine)
BUT: The _test suffix in Go typically means the file is in the package utils_test (external test package), not package utils
Current testing.go is in package utils, making helpers available to other packages' tests

Best practice: Keep it as testing.go. This is the standard Go convention for test helper/utility files that:
Live in the same package as the code being tested
Provide shared test fixtures, builders, and utilities
Are used by multiple *_test.go files
Examples from the Go standard library:
testing/testing.go - the testing framework itself
Many packages have example_test.go, export_test.go, or utility files without _test.go suffix
So no advantage - keep it as testing.go!

raptorsun · 2025-11-09T22:52:30Z

internal/controller/utils/test_fixtures.go

@@ -0,0 +1,452 @@
+package utils


this file will be compiled into the normal operator build.
These are test codes.
Let's rename it to something ending with _test.go postfix to make it only compiled into test build.

raptorsun · 2025-11-09T22:55:14Z

internal/controller/utils/types.go

+	Namespace                      string
+	ReconcileInterval              time.Duration
+}
+
 /*** controller internal ***/
 type ReconcileFunc func(context.Context, *olsv1alpha1.OLSConfig) error


According to how the reconile functions are called, it may be more useful to declare it as type ReconcileFunc func(reconciler.Reconciler, context.Context, *olsv1alpha1.OLSConfig) error

so that we can avoid the wrapper function like

tasks := []utils.ReconcileTask{ { Name: "reconcile Postgres ConfigMap", Task: func(ctx context.Context, cr *olsv1alpha1.OLSConfig) error { return reconcilePostgresConfigMap(r, ctx, cr) }, },

raptorsun · 2025-11-09T23:03:49Z

the code looks good.
2 suggestions:

rename the files containing test codes only to *_test.go to avoid compile it into the released operator build.
declare ReconcileFunc with 3 arguments, in order to avoid uneccessary wrapper functions in reconciliator functions.

xrajesh · 2025-11-10T04:11:32Z

internal/controller/olsconfig_controller.go

+//  1. Reconcile operator-level resources (service monitor, network policy)
+//  2. Delegate LLM provider secret reconciliation to appserver.ReconcileLLMSecrets()
+//  3. Reconcile PostgreSQL database (if conversation cache enabled)
+//  4. Reconcile Console UI plugin (if enabled)


@reconcile Console UI plugin (if enabled) -- this will be always enabled @blublinsky

xrajesh · 2025-11-10T05:15:55Z

/approve
@blublinsky Please address the comments - then its ready to merge

openshift-ci · 2025-11-10T05:16:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xrajesh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xrajesh]

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

openshift-ci · 2025-11-11T13:54:36Z

@blublinsky: all tests passed!

Full PR test history. Your PR dashboard.

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.

JoaoFula · 2025-11-11T16:18:23Z

/lgtm

openshift-ci bot requested review from joshuawilson and xrajesh October 31, 2025 10:23

blublinsky force-pushed the restructuring branch 5 times, most recently from f02cee4 to 86e5748 Compare November 3, 2025 16:00

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

blublinsky force-pushed the restructuring branch 2 times, most recently from 8efa3d2 to 9f42e39 Compare November 3, 2025 19:19

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

blublinsky force-pushed the restructuring branch 2 times, most recently from 21edca0 to b6cc7b4 Compare November 5, 2025 16:30

xrajesh reviewed Nov 6, 2025

View reviewed changes

blublinsky mentioned this pull request Nov 7, 2025

add LCore management support #1120

Open

7 tasks

raptorsun reviewed Nov 9, 2025

View reviewed changes

xrajesh reviewed Nov 10, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2025

blublinsky force-pushed the restructuring branch from b6cc7b4 to b21ab29 Compare November 11, 2025 08:07

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

blublinsky force-pushed the restructuring branch from b21ab29 to 65fc6f4 Compare November 11, 2025 09:32

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

code restructuring

18e34be

blublinsky force-pushed the restructuring branch from 65fc6f4 to 18e34be Compare November 11, 2025 12:03

blublinsky requested a review from xrajesh November 11, 2025 13:12

blublinsky requested a review from raptorsun November 11, 2025 13:12

openshift-ci bot assigned JoaoFula Nov 11, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2025

openshift-merge-bot bot merged commit 006d07a into openshift:main Nov 11, 2025
11 checks passed

code restructuring #1106

code restructuring #1106

Conversation

blublinsky commented Oct 31, 2025 • edited 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!

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!

Choose a reason for hiding this comment

Uh oh!

raptorsun Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raptorsun commented Nov 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xrajesh commented Nov 10, 2025

Uh oh!

openshift-ci bot commented Nov 10, 2025

Uh oh!

openshift-ci bot commented Nov 11, 2025

Uh oh!

JoaoFula commented Nov 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

blublinsky commented Oct 31, 2025 •

edited

Loading

raptorsun Nov 9, 2025 •

edited

Loading