🌱 Return *DeploymentConfig directly from GetDeploymentConfig() (#2598)

Change GetDeploymentConfig() to return (*DeploymentConfig, error) instead of
map[string]any, eliminating the intermediate convertToDeploymentConfig() function
in provider.go. The caller was immediately converting the map to a DeploymentConfig
anyway, so this simplifies the API and removes unnecessary indirection.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: joelanford

internal/operator-controller/applier/provider.go

@@ -112,13 +112,13 @@ func (r *RegistryV1ManifestProvider) extractBundleConfigOptions(rv1 *bundle.Regi
 		opts = append(opts, render.WithTargetNamespaces(*watchNS))
 	}
 
-	// Extract and convert deploymentConfig if present and the feature gate is enabled.
+	// Extract deploymentConfig if present and the feature gate is enabled.
 	if r.IsDeploymentConfigEnabled {
-		if deploymentConfigMap := bundleConfig.GetDeploymentConfig(); deploymentConfigMap != nil {
-			deploymentConfig, err := convertToDeploymentConfig(deploymentConfigMap)
-			if err != nil {
-				return nil, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid deploymentConfig: %w", err))
-			}
+		deploymentConfig, err := bundleConfig.GetDeploymentConfig()
+		if err != nil {
+			return nil, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid deploymentConfig: %w", err))
+		}
+		if deploymentConfig != nil {
 			opts = append(opts, render.WithDeploymentConfig(deploymentConfig))
 		}
 	}
@@ -187,29 +187,6 @@ func extensionConfigBytes(ext *ocv1.ClusterExtension) []byte {
 	return nil
 }
 
-// convertToDeploymentConfig converts a map[string]any (from validated bundle config)
-// to a *config.DeploymentConfig struct that can be passed to the renderer.
-// Returns nil if the map is empty.
-func convertToDeploymentConfig(deploymentConfigMap map[string]any) (*config.DeploymentConfig, error) {
-	if len(deploymentConfigMap) == 0 {
-		return nil, nil
-	}
-
-	// Marshal the map to JSON
-	data, err := json.Marshal(deploymentConfigMap)
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal deploymentConfig: %w", err)
-	}
-
-	// Unmarshal into the DeploymentConfig struct
-	var deploymentConfig config.DeploymentConfig
-	if err := json.Unmarshal(data, &deploymentConfig); err != nil {
-		return nil, fmt.Errorf("failed to unmarshal deploymentConfig: %w", err)
-	}
-
-	return &deploymentConfig, nil
-}
-
 func getBundleAnnotations(bundleFS fs.FS) (map[string]string, error) {
 	// The need to get the underlying bundle in order to extract its annotations
 	// will go away once we have a bundle interface that can surface the annotations independently of the

internal/operator-controller/applier/provider_test.go

@@ -17,6 +17,7 @@ import (
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/config"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1"
@@ -599,8 +600,8 @@ func Test_RegistryV1ManifestProvider_DeploymentConfig(t *testing.T) {
 			BundleRenderer: render.BundleRenderer{
 				ResourceGenerators: []render.ResourceGenerator{
 					func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
-						t.Log("ensure deploymentConfig is nil for empty config object")
-						require.Nil(t, opts.DeploymentConfig)
+						t.Log("ensure deploymentConfig is empty for empty config object")
+						require.Equal(t, &config.DeploymentConfig{}, opts.DeploymentConfig)
 						return nil, nil
 					},
 				},

internal/operator-controller/config/config.go

@@ -106,44 +106,29 @@ func (c *Config) GetWatchNamespace() *string {
 }
 
 // GetDeploymentConfig returns the deploymentConfig value if present in the configuration.
-// Returns nil if deploymentConfig is not set or is explicitly set to null.
-// The returned value is a generic map[string]any that can be marshaled to JSON
-// for validation or conversion to specific types (like v1alpha1.SubscriptionConfig).
-//
-// Returns a defensive deep copy so callers can't mutate the internal Config state.
-func (c *Config) GetDeploymentConfig() map[string]any {
+// Returns (nil, nil) if deploymentConfig is not set or is explicitly set to null.
+// Returns a non-nil error if the value cannot be marshaled or unmarshaled into a DeploymentConfig.
+func (c *Config) GetDeploymentConfig() (*DeploymentConfig, error) {
 	if c == nil || *c == nil {
-		return nil
+		return nil, nil
 	}
 	val, exists := (*c)["deploymentConfig"]
 	if !exists {
-		return nil
+		return nil, nil
 	}
 	// User set deploymentConfig: null - treat as "not configured"
 	if val == nil {
-		return nil
-	}
-	// Schema validation ensures this is an object (map)
-	dcMap, ok := val.(map[string]any)
-	if !ok {
-		return nil
+		return nil, nil
 	}
-
-	// Return a defensive deep copy so callers can't mutate the internal Config state.
-	// We use JSON marshal/unmarshal because the data is already JSON-compatible and
-	// this handles nested structures correctly.
-	data, err := json.Marshal(dcMap)
+	data, err := json.Marshal(val)
 	if err != nil {
-		// This should never happen since the map came from validated JSON/YAML,
-		// but return nil as a safe fallback
-		return nil
+		return nil, fmt.Errorf("failed to marshal deploymentConfig: %w", err)
 	}
-	var copied map[string]any
-	if err := json.Unmarshal(data, &copied); err != nil {
-		// This should never happen for valid JSON
-		return nil
+	var dc DeploymentConfig
+	if err := json.Unmarshal(data, &dc); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal deploymentConfig: %w", err)
 	}
-	return copied
+	return &dc, nil
 }
 
 // UnmarshalConfig takes user configuration, validates it, and creates a Config object.

internal/operator-controller/config/config_test.go

@@ -5,6 +5,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/utils/ptr"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -596,7 +598,7 @@ func Test_GetDeploymentConfig(t *testing.T) {
 	tests := []struct {
 		name                        string
 		rawConfig                   []byte
-		expectedDeploymentConfig    map[string]any
+		expectedDeploymentConfig    *config.DeploymentConfig
 		expectedDeploymentConfigNil bool
 	}{
 		{
@@ -628,13 +630,13 @@ func Test_GetDeploymentConfig(t *testing.T) {
 					}
 				}
 			}`),
-			expectedDeploymentConfig: map[string]any{
-				"nodeSelector": map[string]any{
+			expectedDeploymentConfig: &config.DeploymentConfig{
+				NodeSelector: map[string]string{
 					"kubernetes.io/os": "linux",
 				},
-				"resources": map[string]any{
-					"requests": map[string]any{
-						"memory": "128Mi",
+				Resources: &corev1.ResourceRequirements{
+					Requests: corev1.ResourceList{
+						corev1.ResourceMemory: resource.MustParse("128Mi"),
 					},
 				},
 			},
@@ -650,7 +652,8 @@ func Test_GetDeploymentConfig(t *testing.T) {
 			cfg, err := config.UnmarshalConfig(tt.rawConfig, schema, "")
 			require.NoError(t, err)
 
-			result := cfg.GetDeploymentConfig()
+			result, err := cfg.GetDeploymentConfig()
+			require.NoError(t, err)
 			if tt.expectedDeploymentConfigNil {
 				require.Nil(t, result)
 			} else {
@@ -663,12 +666,13 @@ func Test_GetDeploymentConfig(t *testing.T) {
 	// Test nil config separately
 	t.Run("nil config returns nil", func(t *testing.T) {
 		var cfg *config.Config
-		result := cfg.GetDeploymentConfig()
+		result, err := cfg.GetDeploymentConfig()
+		require.NoError(t, err)
 		require.Nil(t, result)
 	})
 
-	// Test that returned map is a defensive copy (mutations don't affect original)
-	t.Run("returned map is defensive copy - mutations don't affect original", func(t *testing.T) {
+	// Test that returned struct is a separate instance (mutations don't affect original)
+	t.Run("returned struct is independent copy - mutations don't affect original", func(t *testing.T) {
 		rawConfig := []byte(`{
 			"deploymentConfig": {
 				"nodeSelector": {
@@ -684,31 +688,29 @@ func Test_GetDeploymentConfig(t *testing.T) {
 		require.NoError(t, err)
 
 		// Get the deploymentConfig
-		result1 := cfg.GetDeploymentConfig()
+		result1, err := cfg.GetDeploymentConfig()
+		require.NoError(t, err)
 		require.NotNil(t, result1)
 
-		// Mutate the returned map
-		result1["nodeSelector"] = map[string]any{
-			"mutated": "value",
-		}
-		result1["newField"] = "added"
+		// Mutate the returned struct
+		result1.NodeSelector["mutated"] = "value"
 
 		// Get deploymentConfig again - should be unaffected by mutations
-		result2 := cfg.GetDeploymentConfig()
+		result2, err := cfg.GetDeploymentConfig()
+		require.NoError(t, err)
 		require.NotNil(t, result2)
 
 		// Original values should be intact
-		require.Equal(t, map[string]any{
-			"nodeSelector": map[string]any{
-				"kubernetes.io/os": "linux",
-			},
-		}, result2)
-
-		// New field should not exist
-		_, exists := result2["newField"]
-		require.False(t, exists)
+		require.Equal(t, map[string]string{
+			"kubernetes.io/os": "linux",
+		}, result2.NodeSelector)
+	})
 
-		// result1 should have the mutations
-		require.Equal(t, "added", result1["newField"])
+	// Test that invalid deploymentConfig type returns an error
+	t.Run("invalid deploymentConfig type returns error", func(t *testing.T) {
+		cfg := config.Config{"deploymentConfig": "not-an-object"}
+		result, err := cfg.GetDeploymentConfig()
+		require.Error(t, err)
+		require.Nil(t, result)
 	})
 }