Addressed reviewer comments

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

Author: Per G. da Silva

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 := ext.ExtensionConfigBytes()
+		// treat no config as empty to properly validate the configuration
+		// e.g. ensure that all required fields are set and set appropriately
+		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)
 		}

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: