Addressed reviewer comments

Signed-off-by: Per G. da Silva <[email protected]>

Author: Per G. da Silva

api/v1/clusterextension_types.go

@@ -535,23 +535,6 @@ type ClusterExtension struct {
 	Status ClusterExtensionStatus `json:"status,omitempty"`
 }
 
-// ExtensionConfigBytes returns the ClusterExtension configuration input by the user
-// through .spec.config as a byte slice
-func (e *ClusterExtension) ExtensionConfigBytes() []byte {
-	if e.Spec.Config == nil {
-		return nil
-	}
-	switch e.Spec.Config.ConfigType {
-	case ClusterExtensionConfigTypeInline:
-		if e.Spec.Config.Inline != nil {
-			return e.Spec.Config.Inline.Raw
-		}
-		return nil
-	default:
-		return nil
-	}
-}
-
 // +kubebuilder:object:root=true
 
 // ClusterExtensionList contains a list of ClusterExtension

api/v1/clusterextension_types_test.go

@@ -9,11 +9,8 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/stretchr/testify/require"
 	"golang.org/x/exp/slices" // replace with "slices" in go 1.21
-	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
-	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets"
 )
 
@@ -103,50 +100,3 @@ func parseConstants(prefix string) ([]string, error) {
 	}
 	return constValues, nil
 }
-
-func Test_ExtensionConfigBytes(t *testing.T) {
-	for _, tc := range []struct {
-		name                string
-		config              *ocv1.ClusterExtensionConfig
-		expectedConfigBytes []byte
-	}{
-		{
-			name:                "nil for no config",
-			config:              nil,
-			expectedConfigBytes: nil,
-		},
-		{
-			name: "nil for unknown config type",
-			config: &ocv1.ClusterExtensionConfig{
-				ConfigType: "not-a-real-config-type",
-			},
-			expectedConfigBytes: nil,
-		},
-		{
-			name: ".spec.config.inline.raw for Inline config",
-			config: &ocv1.ClusterExtensionConfig{
-				ConfigType: ocv1.ClusterExtensionConfigTypeInline,
-				Inline: &apiextensionsv1.JSON{
-					Raw: []byte(`{"key": "value"}`),
-				},
-			},
-			expectedConfigBytes: []byte(`{"key": "value"}`),
-		},
-		{
-			name: "nil when .spec.config.inline is nil",
-			config: &ocv1.ClusterExtensionConfig{
-				ConfigType: ocv1.ClusterExtensionConfigTypeInline,
-			},
-			expectedConfigBytes: nil,
-		},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			ext := &ocv1.ClusterExtension{
-				Spec: ocv1.ClusterExtensionSpec{
-					Config: tc.config,
-				},
-			}
-			require.Equal(t, tc.expectedConfigBytes, ext.ExtensionConfigBytes())
-		})
-	}
-}

docs/draft/howto/single-ownnamespace-install.md

@@ -64,7 +64,7 @@ install mode support, if the bundle:
  - bundle only supports *SingleNamespace* and/or *OwnNamespace* => `watchNamespace` is required
 
 The `watchNamespace` configuration can only be the install namespace if the bundle supports the *OwnNamespace* install mode, and
-it can only be any other namespace is the bundle supports the *SingleNamespace* install mode.
+it can only be any other namespace if the bundle supports the *SingleNamespace* install mode.
 
 Examples:
 

internal/operator-controller/applier/provider.go

@@ -69,7 +69,13 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens
 	}
 
 	if r.IsSingleOwnNamespaceEnabled {
-		bundleConfig, err := bundle.UnmarshallConfig(ext.ExtensionConfigBytes(), rv1, ext.Spec.Namespace)
+		bundleConfigBytes := extensionConfigBytes(ext)
+		// treat no config as empty to properly validate the configuration
+		// e.g. ensure that validation catches missing required fields
+		if bundleConfigBytes == nil {
+			bundleConfigBytes = []byte(`{}`)
+		}
+		bundleConfig, err := bundle.UnmarshalConfig(bundleConfigBytes, rv1, ext.Spec.Namespace)
 		if err != nil {
 			return nil, fmt.Errorf("invalid bundle configuration: %w", err)
 		}
@@ -128,3 +134,17 @@ func (r *RegistryV1HelmChartProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExten
 
 	return chrt, nil
 }
+
+// ExtensionConfigBytes returns the ClusterExtension configuration input by the user
+// through .spec.config as a byte slice.
+func extensionConfigBytes(ext *ocv1.ClusterExtension) []byte {
+	if ext.Spec.Config != nil {
+		switch ext.Spec.Config.ConfigType {
+		case ocv1.ClusterExtensionConfigTypeInline:
+			if ext.Spec.Config.Inline != nil {
+				return ext.Spec.Config.Inline.Raw
+			}
+		}
+	}
+	return nil
+}

internal/operator-controller/applier/provider_test.go

@@ -244,15 +244,6 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
 	t.Run("rejects bundles without AllNamespaces install mode and with SingleNamespace support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) {
 		expectedWatchNamespace := "some-namespace"
 		provider := applier.RegistryV1ManifestProvider{
-			BundleRenderer: render.BundleRenderer{
-				ResourceGenerators: []render.ResourceGenerator{
-					func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
-						t.Log("ensure watch namespace is appropriately configured")
-						require.Equal(t, []string{expectedWatchNamespace}, opts.TargetNamespaces)
-						return nil, nil
-					},
-				},
-			},
 			IsSingleOwnNamespaceEnabled: false,
 		}
 
@@ -322,17 +313,7 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
 	})
 
 	t.Run("rejects bundles with {SingleNamespace} install modes when no configuration is given", func(t *testing.T) {
-		expectedWatchNamespace := "some-namespace"
 		provider := applier.RegistryV1ManifestProvider{
-			BundleRenderer: render.BundleRenderer{
-				ResourceGenerators: []render.ResourceGenerator{
-					func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
-						t.Log("ensure watch namespace is appropriately configured")
-						require.Equal(t, []string{expectedWatchNamespace}, opts.TargetNamespaces)
-						return nil, nil
-					},
-				},
-			},
 			IsSingleOwnNamespaceEnabled: true,
 		}
 
@@ -349,18 +330,28 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
 	})
 
 	t.Run("accepts bundles with {OwnNamespace} install modes when the appropriate configuration is given", func(t *testing.T) {
+		installNamespace := "some-namespace"
 		provider := applier.RegistryV1ManifestProvider{
+			BundleRenderer: render.BundleRenderer{
+				ResourceGenerators: []render.ResourceGenerator{
+					func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
+						t.Log("ensure watch namespace is appropriately configured")
+						require.Equal(t, []string{installNamespace}, opts.TargetNamespaces)
+						return nil, nil
+					},
+				},
+			},
 			IsSingleOwnNamespaceEnabled: true,
 		}
 		bundleFS := bundlefs.Builder().WithPackageName("test").
 			WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace).Build()).Build()
 		_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
 			Spec: ocv1.ClusterExtensionSpec{
-				Namespace: "install-namespace",
+				Namespace: installNamespace,
 				Config: &ocv1.ClusterExtensionConfig{
 					ConfigType: ocv1.ClusterExtensionConfigTypeInline,
 					Inline: &apiextensionsv1.JSON{
-						Raw: []byte(`{"watchNamespace": "install-namespace"}`),
+						Raw: []byte(`{"watchNamespace": "` + installNamespace + `"}`),
 					},
 				},
 			},

internal/operator-controller/rukpak/bundle/config.go

@@ -17,20 +17,20 @@ type Config struct {
 	WatchNamespace *string `json:"watchNamespace"`
 }
 
-// UnmarshallConfig returns a deserialized and validated *bundle.Config based on bytes and validated
+// UnmarshalConfig returns a deserialized *bundle.Config based on bytes and validated
 // against rv1 and the desired install namespaces. It will error if:
 // - rv is nil
 // - bytes is not a valid YAML/JSON object
 // - bytes is a valid YAML/JSON object but does not follow the registry+v1 schema
-// if bytes is nil it will be treated as an empty json object ({})
-func UnmarshallConfig(bytes []byte, rv1 RegistryV1, installNamespace string) (*Config, error) {
+// - if bytes is nil, a nil *bundle.Config is returned with no error
+func UnmarshalConfig(bytes []byte, rv1 RegistryV1, installNamespace string) (*Config, error) {
 	if bytes == nil {
-		bytes = []byte("{}")
+		return nil, nil
 	}
 
 	bundleConfig := &Config{}
 	if err := yaml.UnmarshalStrict(bytes, bundleConfig); err != nil {
-		return nil, fmt.Errorf("error unmarshalling registry+v1 configuration: %w", formatUnmarshallError(err))
+		return nil, fmt.Errorf("error unmarshalling registry+v1 configuration: %w", formatUnmarshalError(err))
 	}
 
 	// collect bundle install modes
@@ -98,8 +98,8 @@ func isWatchNamespaceConfigRequired(bundleInstallModeSet sets.Set[v1alpha1.Insta
 		!bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true})
 }
 
-// formatUnmarshallError format JSON unmarshal errors to be more readable
-func formatUnmarshallError(err error) error {
+// formatUnmarshalError format JSON unmarshal errors to be more readable
+func formatUnmarshalError(err error) error {
 	var unmarshalErr *json.UnmarshalTypeError
 	if errors.As(err, &unmarshalErr) {
 		if unmarshalErr.Field == "" {

internal/operator-controller/rukpak/bundle/config_test.go

@@ -12,7 +12,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing/clusterserviceversion"
 )
 
-func Test_UnmarshallConfig(t *testing.T) {
+func Test_UnmarshalConfig(t *testing.T) {
 	for _, tc := range []struct {
 		name                  string
 		rawConfig             []byte
@@ -22,11 +22,9 @@ func Test_UnmarshallConfig(t *testing.T) {
 		expectedConfig        *bundle.Config
 	}{
 		{
-			name:                  "treats nil config as {}",
-			rawConfig:             nil,
-			supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace},
-			expectedConfig:        nil,
-			expectedErrMessage:    "required field \"watchNamespace\" is missing",
+			name:           "returns nil for nil config",
+			rawConfig:      nil,
+			expectedConfig: nil,
 		},
 		{
 			name:                  "accepts json config",
@@ -299,7 +297,7 @@ func Test_UnmarshallConfig(t *testing.T) {
 				}
 			}
 
-			config, err := bundle.UnmarshallConfig(tc.rawConfig, rv1, tc.installNamespace)
+			config, err := bundle.UnmarshalConfig(tc.rawConfig, rv1, tc.installNamespace)
 			require.Equal(t, tc.expectedConfig, config)
 			if tc.expectedErrMessage != "" {
 				require.Error(t, err)

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

@@ -162,9 +162,8 @@ func validateTargetNamespaces(rv1 *bundle.RegistryV1, installNamespace string, t
 		// If only OwnNamespace is supported, the default will be [install-namespace] -> only watch the install/own namespace
 		if supportedInstallModes.Has(v1alpha1.InstallModeTypeMultiNamespace) {
 			return errors.New("at least one target namespace must be specified")
-		} else {
-			return errors.New("exactly one target namespace must be specified")
 		}
+		return errors.New("exactly one target namespace must be specified")
 	case set.Len() == 1 && set.Has(""):
 		if supportedInstallModes.Has(v1alpha1.InstallModeTypeAllNamespaces) {
 			return nil

testdata/images/bundles/own-namespace-operator/v1.0.0/manifests/ownnamespaceoperator.clusterserviceversion.yaml

@@ -27,11 +27,11 @@ spec:
   apiservicedefinitions: {}
   customresourcedefinitions:
   owned:
-    - description: A dummy resource for an operator that only supports own namespace install mode
-      displayName: OwnNamespace
-      kind: OwnNamespace
-      name: ownnamespaces.olm.operatorframework.io
-      version: v1
+  - description: A dummy resource for an operator that only supports own namespace install mode
+    displayName: OwnNamespace
+    kind: OwnNamespace
+    name: ownnamespaces.olm.operatorframework.io
+    version: v1
   description: OLM OwnNamespace Testing Operator
   displayName: test-operator
   icon: