already exisitng

No, it wasn't already existing. The diff indicates it was previously hyphen-separated

Is this unrelated to the scope of the PR?

Was this a code gap that you found and fixed in this PR, or just additional test coverage?

removed it, it was extra coverage unrelated to this PR's scope, part of the refactor churn i guess

Nit: I'd suggest renaming this to something more semantic related to the feature it gates in the manifest provider rather than the feature gate we decided to put it behind.

Maybe IsNamespaceManagementEnabled, but still driven in main.go from the BoxcutterRuntime feature gate.

renamed

What happens if neither are provided? What happens if both are provided?

In this code, "neither" and "both" can't happen. It's a simple either/or on one thing: is spec.namespace set or not?

I'm asking from the perspective of the library, not the perspective of the caller. We should try to make the library itself cohesive and ergonomic (which would imply that options are in fact optional).

My suggestion here would be to remove render.RenderInstallNamespace() entirely. That would become the default. And then if WithInstallNamespace is passed as an option, then we get the existing self-managed behavior.

And with that change, I'd suggest WithSelfManagedInstallNamespace as the name to help illustrate that the default behavior is not a self-managed namespace.

reworked it so the default (no option) is the OLM-managed namespace that emits its own Namespace object, and added WithSelfManagedInstallNamespace(ns) for the bring-your-own-namespace case. Dropped WithInstallNamespace/RenderInstallNamespace. The provider's if/else collapses to a single option, and the zero-option call now does the sensible thing.

I wonder if experimental should just let go of this immutability and allow changes: set to unset, unset to set, and set to something else.

We imagined that we'd eventually deprecate spec.namespace and move the field to be sourced from spec.config. Now that we don't have an SA field, I'm thinking the need for immutable namespace field is now much lower, maybe non-existent.

Agreed the SA-driven rationale is gone, but immutability still guards against stranding already-installed resources when the namespace changes, relaxing it safely needs GC/re-apply-on-change handling. I'd prefer to keep it immutable here and revisit when we move the field to spec.config.

I don't think we'd strand anything though, would we? We'd regenerate the entire set of manifests and anything in the old install namespace would be re-created in the new install namespace.

Unmanaged objects in the old namespace, potentially related to the ClusterExtension, wouldn't move. But I think that is a general problem that we can't really solve anyway, especially when OLM will have support for helm charts that can essentially do anything using their own config schema.

Ultimately, I guess the question is "if this field had started out as a field in spec.config and was not required due to SA existence, would we have made it immutable then?"

I think the answer is "probably not", but maybe we can dig up some of those discussions to help inform us.

As far as I can tell, this is not a PR-blocking discussion. It seems like this PR's immutability rules still only allow foo -> foo and "" -> "", and disallow setting/unsetting/changing. Which is the same semantic we have today, just with the relaxing of allowing unset on creation.

Any reason we need both InstallNamespace and InstallNamespaceTemplate. Could we just have InstallNamespace *corev1.Namespace, and if all we have is a name (and no metadata), we'd just populate/read from InstallNamespace.Metadata.Name?

Otherwise, it seems like there's the awkward possibility of InstallNamespace and InstallNamespaceTemplate disagreeing on the name.

Rather than collapse to *corev1.Namespace (which forces every namespace-name read to .Name), I dropped the typed template and kept only the labels/annotations it actually contributes, the name now has a single source of truth in InstallNamespace and there's no second .Name to disagree with.

From a compiler/library standpoint though, InstallNamespaceTemplate.Metadata.Name is a valid field (that will always be unset, it sounds like). So there's still some ambiguity in the actual types/fields even if we (right now) are being careful to ignore it. Mainly I'm worried about this being something that could be a point of confusion to our future selves.

Can we think of a way to eliminate the ambiguity at the struct level?

Agreed, that ambiguity is exactly why I dropped the typed *corev1.Namespace template. render.Options now carries InstallNamespace string plus InstallNamespaceLabels/InstallNamespaceAnnotations map[string]string, so there's no second .Metadata.Name field to be silently ignored. The struct expresses only what it actually uses.

The API checker is complaining about this:

api/v1/clusterextension_types.go:52:2:kubeapilinter:commentstart: godoc for field ClusterExtensionSpec.Namespace should start with 'namespace ...'

But I think it's "ok" since the field has been moved back to experimental.

When an extension is installed without spec.namespace, this resolves the namespace from each newly selected bundle. If a later bundle changes its suggested namespace or adds a template, the upgrade moves all namespaced resources and archives the old revision, which can delete the previous managed namespace and its contents.

Do we want to persist or otherwise retain the initially resolved namespace for the ClusterExtension lifecycle? Is it acceptable to change the namespace?

Yes, it is not acceptable to change the namespace. It's new to the managed-namespace path (before this PR spec.namespace was required and immutable).

No persistence needed though: since we should reject the change rather than follow it, we only need to detect it, and the namespace is already discoverable via the owner-kind/owner-name labels OLM puts on every rendered object. So ValidateInstallNamespace now fills its empty spec.namespace == "" branch: find the owned namespace, resolve from the incoming bundle, terminal error if they differ.

fixed in the last commit

I'm not sure I agree with this. If the install namespace is system managed, what's wrong with the bundle author having an opinion about moving it to a different namespace?

Another perspective: the spec is the spec is the spec. What happened in the past (e.g. an old bundle was installed in namespace foo) has no bearing on what the spec directly or indirectly (via a newly resolved bundle) says should happen in the future.

We should not use current state as input to future state.