✨ OPRUN-4113: Compute target namespace defaults #2178

perdasilva · 2025-09-02T08:39:51Z

Description

Updates registry+v1 renderer to compute target namespace defaults based on the bundle's install mode configuration:

bundle supports AllNamespaces: default is [""] (install in AllNamespaces mode)
bundle supports OwnNamespace: default is []
bundle supports neither AllNamespaces or OwnNamespace mode: undefined (user must specify TargetNamespaces)

We also update the targetNamespace validation to ensure the own namespace cannot be used unless OwnNamespace install mode is supported

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-09-02T08:40:06Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`01b28a8`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/68b9ba22387ea40008a416be
😎 Deploy Preview	https://deploy-preview-2178--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

codecov · 2025-09-02T08:50:45Z

Codecov Report

❌ Patch coverage is 93.61702% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.77%. Comparing base (5d6e431) to head (01b28a8).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ternal/operator-controller/rukpak/render/render.go	93.61%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2178      +/-   ##
==========================================
+ Coverage   72.73%   72.77%   +0.04%     
==========================================
  Files          79       79              
  Lines        7391     7421      +30     
==========================================
+ Hits         5376     5401      +25     
- Misses       1667     1671       +4     
- Partials      348      349       +1

Flag	Coverage Δ
e2e	`44.19% <42.55%> (-0.04%)`	⬇️
experimental-e2e	`56.13% <44.68%> (-0.05%)`	⬇️
unit	`58.34% <93.61%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

anik120

/lgtm

internal/operator-controller/rukpak/render/render.go

joelanford · 2025-09-02T15:43:39Z

internal/operator-controller/rukpak/render/render.go

+		if supportedInstallModes.Has(v1alpha1.InstallModeTypeSingleNamespace) {
 			return nil
 		}


I'm just noticing, but this logic (both before and after) means that an operator could support only SingleNamespace, and we'd allow that namespace to be the install namespace.

I don't think that's correct. We should only allow watchNamespace == installNamespace if the OwnNamespace install mode is supported.

I was trying to look through the v0 codebase to ascertain this anb neither in the enhancement nor in code do I find this a reference to this. But, it's certainly possible (that it is and that I've missed it). Do you have a ref?

I don't have a ref, but I do remember conversations about the distinction related to RBAC where some teams wanted to keep operator namespace separate from operand namespace.

I'll try to find a definitive ref. This is non-blocking since this PR doesn't change this logic at all.

Just let me know and I'll create another issue to address this. Should be straightforward enough. And a good one to fix before we go GA with Single/OwnNamespace

Found it: https://github.com/operator-framework/api/blob/0433e5e6641217b79b29a90f4f2732ffab72ad74/pkg/operators/v1alpha1/clusterserviceversion.go#L179-L182

thank you! I kept looking for references to the specific type. Lemme fix that up.

internal/operator-controller/rukpak/render/render_test.go

oceanc80

/lgtm

anik120 · 2025-09-03T12:54:43Z

/assign @thetechnick

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

…on on single namespace mode Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

joelanford · 2025-09-03T16:48:51Z

internal/operator-controller/rukpak/render/render.go

-	case set.Len() == 0 || (set.Len() == 1 && set.Has("")):
-		if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeAllNamespaces)) {
+	case set.Len() == 0:
+		if supportedInstallModes.Len() == 1 && supportedInstallModes.Has(v1alpha1.InstallModeTypeSingleNamespace) {


Trying to make sense of this. Am I understanding correctly that set.Len() will only be 0 if we were unable to set a default target namespace, and that only happens when neither AllNamespaces nor OwnNamespace are supported.

And therefore, by the time we get to this code, the possibilities are:

only SingleNamespace

only MultiNamespace

both SingleNamespace and MultiNamespace

And of those three cases, the only one that requires exactly one target namespace is "only SingleNamespace. Hence the if conditional you have here.

If that's correct. Can you add a comment that explains this a little bit? I don't think it is obvious why we aren't checking other supported install mode combinations.

Also, what if someone explicitly uses option WithTargetNamespaces(nil) or WithTargetNamespaces([]string{}])?

The result of that would be:

The defaulting logic runs (setting a default in the case of All or Own namespace)

The functional option runs (unsetting the default)

So we could have set.Len() == 0 for any combination of supported install modes unless we somehow require at least one namespace to be defined with that option. What if we did this?

func WithTargetNamespaces(namespace string, extraNamespaces ...string) Option { return func(o *Options) { o.TargetNamespaces = append([]string{namespace}, extraNamespaces...) } }

That way, using WithTargetNamespaces only accepts 1..N namespaces, not 0..N.

While I think this change in option does guarantee that at least one namespace is specified, it's annoying to use for the caller because it will force me to break up my slice of string into components. I won't be able to do targetNamespaces..., I'd need to do something like targetNamespace[0], targetNamespaces[1:]... but that will involve length checks, etc.

I think its ok for you to specify nil or empty...the validator will pick it up.

alternatively, we could just wrap an if around it and only update the value if len(namespaces) > 0? wdyt?

I went with my proposed solution above. I've also added more checks for MultiNamespace (no including install namespace if no OwnNamespace support)

Good points! Your solution seems like a good compromise.

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

joelanford · 2025-09-04T17:08:26Z

/approve
/lgtm

openshift-ci · 2025-09-04T17:08:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, joelanford, oceanc80

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:

~~internal/operator-controller/OWNERS~~ [joelanford]

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

perdasilva requested a review from a team as a code owner September 2, 2025 08:39

openshift-ci bot requested review from anik120 and rashmigottipati September 2, 2025 08:40

perdasilva changed the title ~~✨ Compute target namespace defaults~~ OPRUN-4113: ✨ Compute target namespace defaults Sep 2, 2025

perdasilva changed the title ~~OPRUN-4113: ✨ Compute target namespace defaults~~ ✨ OPRUN-4113: Compute target namespace defaults Sep 2, 2025

perdasilva mentioned this pull request Sep 2, 2025

✨ OPRUN-4090: (rukpak) Add config support for reg+v1 bundle to chart converter #2166

Merged

4 tasks

anik120 approved these changes Sep 2, 2025

View reviewed changes

internal/operator-controller/rukpak/render/render.go Outdated Show resolved Hide resolved

openshift-ci bot assigned anik120 Sep 2, 2025

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

perdasilva force-pushed the renderer-defaults branch from 3610edf to 14e1789 Compare September 2, 2025 15:37

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2025

anik120 reviewed Sep 2, 2025

View reviewed changes

internal/operator-controller/rukpak/render/render.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 2, 2025

View reviewed changes

internal/operator-controller/rukpak/render/render_test.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 2, 2025

View reviewed changes

internal/operator-controller/rukpak/render/render_test.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 2, 2025

View reviewed changes

internal/operator-controller/rukpak/render/render_test.go Outdated Show resolved Hide resolved

perdasilva force-pushed the renderer-defaults branch 2 times, most recently from 97ffd8e to 0a263ed Compare September 3, 2025 06:58

oceanc80 approved these changes Sep 3, 2025

View reviewed changes

openshift-ci bot assigned oceanc80 Sep 3, 2025

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

anik120 approved these changes Sep 3, 2025

View reviewed changes

openshift-ci bot assigned thetechnick Sep 3, 2025

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2025

Per Goncalves da Silva added 2 commits September 3, 2025 16:24

Compute target namespace defaults

dc73f58

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

Update target namespace validation to reject own namespace installati…

4fc183a

…on on single namespace mode Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

perdasilva force-pushed the renderer-defaults branch from 2fcbefa to 4fc183a Compare September 3, 2025 14:25

joelanford reviewed Sep 3, 2025

View reviewed changes

Update WithTargetNamespace behavior and MultiNamespace validation

01b28a8

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

openshift-ci bot assigned joelanford Sep 4, 2025

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

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

openshift-merge-bot bot merged commit b4aeb92 into operator-framework:main Sep 4, 2025
25 checks passed

✨ OPRUN-4113: Compute target namespace defaults #2178

✨ OPRUN-4113: Compute target namespace defaults #2178

Uh oh!

Conversation

perdasilva commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlify bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecov bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anik120 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

oceanc80 left a comment

Choose a reason for hiding this comment

Uh oh!

anik120 commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford commented Sep 4, 2025

Uh oh!

openshift-ci bot commented Sep 4, 2025

Uh oh!

Uh oh!

Uh oh!

perdasilva commented Sep 2, 2025 •

edited

Loading

netlify bot commented Sep 2, 2025 •

edited

Loading

codecov bot commented Sep 2, 2025 •

edited

Loading

joelanford Sep 3, 2025 •

edited

Loading