🌱 Return *DeploymentConfig directly from GetDeploymentConfig()

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/config/config.go

@@ -107,43 +107,33 @@ 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 {
+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
+		return nil, 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)
 	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

@@ -596,7 +596,7 @@ func Test_GetDeploymentConfig(t *testing.T) {
 	tests := []struct {
 		name                        string
 		rawConfig                   []byte
-		expectedDeploymentConfig    map[string]any
+		expectedDeploymentConfig    *config.DeploymentConfig
 		expectedDeploymentConfigNil bool
 	}{
 		{
@@ -620,23 +620,13 @@ func Test_GetDeploymentConfig(t *testing.T) {
 				"deploymentConfig": {
 					"nodeSelector": {
 						"kubernetes.io/os": "linux"
-					},
-					"resources": {
-						"requests": {
-							"memory": "128Mi"
-						}
 					}
 				}
 			}`),
-			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",
-					},
-				},
 			},
 			expectedDeploymentConfigNil: false,
 		},
@@ -650,7 +640,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,52 +654,8 @@ 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()
-		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) {
-		rawConfig := []byte(`{
-			"deploymentConfig": {
-				"nodeSelector": {
-					"kubernetes.io/os": "linux"
-				}
-			}
-		}`)
-
-		schema, err := bundle.GetConfigSchema()
+		result, err := cfg.GetDeploymentConfig()
 		require.NoError(t, err)
-
-		cfg, err := config.UnmarshalConfig(rawConfig, schema, "")
-		require.NoError(t, err)
-
-		// Get the deploymentConfig
-		result1 := cfg.GetDeploymentConfig()
-		require.NotNil(t, result1)
-
-		// Mutate the returned map
-		result1["nodeSelector"] = map[string]any{
-			"mutated": "value",
-		}
-		result1["newField"] = "added"
-
-		// Get deploymentConfig again - should be unaffected by mutations
-		result2 := cfg.GetDeploymentConfig()
-		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)
-
-		// result1 should have the mutations
-		require.Equal(t, "added", result1["newField"])
+		require.Nil(t, result)
 	})
 }