Refactor converter to use validator

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

Author: Per Goncalves da Silva

internal/operator-controller/rukpak/convert/registryv1.go

@@ -47,7 +47,7 @@ func RegistryV1ToHelmChart(rv1 fs.FS, installNamespace string, watchNamespace st
 		return nil, err
 	}
 
-	plain, err := Convert(reg, installNamespace, []string{watchNamespace})
+	plain, err := PlainConverter.Convert(reg, installNamespace, []string{watchNamespace})
 	if err != nil {
 		return nil, err
 	}
@@ -230,15 +230,23 @@ func saNameOrDefault(saName string) string {
 	return saName
 }
 
-func Convert(in RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
+type Converter struct {
+	BundleValidator BundleValidator
+}
+
+func (c Converter) Convert(rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
+	if err := c.BundleValidator.Validate(&rv1); err != nil {
+		return nil, err
+	}
+
 	if installNamespace == "" {
-		installNamespace = in.CSV.Annotations["operatorframework.io/suggested-namespace"]
+		installNamespace = rv1.CSV.Annotations["operatorframework.io/suggested-namespace"]
 	}
 	if installNamespace == "" {
-		installNamespace = fmt.Sprintf("%s-system", in.PackageName)
+		installNamespace = fmt.Sprintf("%s-system", rv1.PackageName)
 	}
 	supportedInstallModes := sets.New[string]()
-	for _, im := range in.CSV.Spec.InstallModes {
+	for _, im := range rv1.CSV.Spec.InstallModes {
 		if im.Supported {
 			supportedInstallModes.Insert(string(im.Type))
 		}
@@ -255,18 +263,18 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 		return nil, err
 	}
 
-	if len(in.CSV.Spec.APIServiceDefinitions.Owned) > 0 {
+	if len(rv1.CSV.Spec.APIServiceDefinitions.Owned) > 0 {
 		return nil, fmt.Errorf("apiServiceDefintions are not supported")
 	}
 
-	if len(in.CSV.Spec.WebhookDefinitions) > 0 {
+	if len(rv1.CSV.Spec.WebhookDefinitions) > 0 {
 		return nil, fmt.Errorf("webhookDefinitions are not supported")
 	}
 
 	deployments := []appsv1.Deployment{}
 	serviceAccounts := map[string]corev1.ServiceAccount{}
-	for _, depSpec := range in.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs {
-		annotations := util.MergeMaps(in.CSV.Annotations, depSpec.Spec.Template.Annotations)
+	for _, depSpec := range rv1.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs {
+		annotations := util.MergeMaps(rv1.CSV.Annotations, depSpec.Spec.Template.Annotations)
 		annotations["olm.targetNamespaces"] = strings.Join(targetNamespaces, ",")
 		depSpec.Spec.Template.Annotations = annotations
 		deployments = append(deployments, appsv1.Deployment{
@@ -300,8 +308,8 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 	clusterRoles := []rbacv1.ClusterRole{}
 	clusterRoleBindings := []rbacv1.ClusterRoleBinding{}
 
-	permissions := in.CSV.Spec.InstallStrategy.StrategySpec.Permissions
-	clusterPermissions := in.CSV.Spec.InstallStrategy.StrategySpec.ClusterPermissions
+	permissions := rv1.CSV.Spec.InstallStrategy.StrategySpec.Permissions
+	clusterPermissions := rv1.CSV.Spec.InstallStrategy.StrategySpec.ClusterPermissions
 	allPermissions := append(permissions, clusterPermissions...)
 
 	// Create all the service accounts
@@ -328,7 +336,7 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 	for _, ns := range targetNamespaces {
 		for _, permission := range permissions {
 			saName := saNameOrDefault(permission.ServiceAccountName)
-			name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
+			name, err := generateName(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
 			if err != nil {
 				return nil, err
 			}
@@ -339,7 +347,7 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 
 	for _, permission := range clusterPermissions {
 		saName := saNameOrDefault(permission.ServiceAccountName)
-		name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
+		name, err := generateName(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
 		if err != nil {
 			return nil, err
 		}
@@ -370,10 +378,10 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 		obj := obj
 		objs = append(objs, &obj)
 	}
-	for _, obj := range in.CRDs {
+	for _, obj := range rv1.CRDs {
 		objs = append(objs, &obj)
 	}
-	for _, obj := range in.Others {
+	for _, obj := range rv1.Others {
 		obj := obj
 		supported, namespaced := registrybundle.IsSupported(obj.GetKind())
 		if !supported {
@@ -391,6 +399,10 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
 	return &Plain{Objects: objs}, nil
 }
 
+var PlainConverter = Converter{
+	BundleValidator: RegistryV1BundleValidator,
+}
+
 const maxNameLength = 63
 
 func generateName(base string, o interface{}) (string, error) {

internal/operator-controller/rukpak/convert/registryv1_test.go

@@ -1,6 +1,7 @@
 package convert_test
 
 import (
+	"errors"
 	"fmt"
 	"io/fs"
 	"os"
@@ -52,6 +53,24 @@ func getCsvAndService() (v1alpha1.ClusterServiceVersion, corev1.Service) {
 	return csv, svc
 }
 
+func TestConverterValidatesBundle(t *testing.T) {
+	converter := convert.Converter{
+		BundleValidator: []func(rv1 *convert.RegistryV1) []error{
+			func(rv1 *convert.RegistryV1) []error {
+				return []error{errors.New("test error")}
+			},
+		},
+	}
+
+	_, err := converter.Convert(convert.RegistryV1{}, "installNamespace", []string{"watchNamespace"})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "test error")
+}
+
+func TestPlainConverterUsedRegV1Validator(t *testing.T) {
+	require.Equal(t, convert.RegistryV1BundleValidator, convert.PlainConverter.BundleValidator)
+}
+
 func TestRegistryV1SuiteNamespaceNotAvailable(t *testing.T) {
 	var targetNamespaces []string
 
@@ -70,7 +89,7 @@ func TestRegistryV1SuiteNamespaceNotAvailable(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
 	require.NoError(t, err)
 
 	t.Log("By verifying if plain bundle has required objects")
@@ -104,7 +123,7 @@ func TestRegistryV1SuiteNamespaceAvailable(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
 	require.NoError(t, err)
 
 	t.Log("By verifying if plain bundle has required objects")
@@ -145,7 +164,7 @@ func TestRegistryV1SuiteNamespaceUnsupportedKind(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
 	require.Error(t, err)
 	require.ErrorContains(t, err, "bundle contains unsupported resource")
 	require.Nil(t, plainBundle)
@@ -179,7 +198,7 @@ func TestRegistryV1SuiteNamespaceClusterScoped(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
 	require.NoError(t, err)
 
 	t.Log("By verifying if plain bundle has required objects")
@@ -266,7 +285,7 @@ func TestRegistryV1SuiteGenerateAllNamespace(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
 	require.NoError(t, err)
 
 	t.Log("By verifying if plain bundle has required objects")
@@ -299,7 +318,7 @@ func TestRegistryV1SuiteGenerateMultiNamespace(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
 	require.NoError(t, err)
 
 	t.Log("By verifying if plain bundle has required objects")
@@ -332,7 +351,7 @@ func TestRegistryV1SuiteGenerateSingleNamespace(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
 	require.NoError(t, err)
 
 	t.Log("By verifying if plain bundle has required objects")
@@ -365,7 +384,7 @@ func TestRegistryV1SuiteGenerateOwnNamespace(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
 	require.NoError(t, err)
 
 	t.Log("By verifying if plain bundle has required objects")
@@ -470,7 +489,7 @@ func TestConvertInstallModeValidation(t *testing.T) {
 			}
 
 			t.Log("By converting to plain")
-			plainBundle, err := convert.Convert(registryv1Bundle, tc.installNamespace, tc.watchNamespaces)
+			plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, tc.installNamespace, tc.watchNamespaces)
 			require.Error(t, err)
 			require.Nil(t, plainBundle)
 		})
@@ -559,7 +578,7 @@ func TestRegistryV1SuiteGenerateNoWebhooks(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
 	require.Error(t, err)
 	require.ErrorContains(t, err, "webhookDefinitions are not supported")
 	require.Nil(t, plainBundle)
@@ -590,7 +609,7 @@ func TestRegistryV1SuiteGenerateNoAPISerciceDefinitions(t *testing.T) {
 	}
 
 	t.Log("By converting to plain")
-	plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
+	plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
 	require.Error(t, err)
 	require.ErrorContains(t, err, "apiServiceDefintions are not supported")
 	require.Nil(t, plainBundle)

internal/operator-controller/rukpak/convert/validator.go

@@ -1,6 +1,7 @@
 package convert
 
 import (
+	"errors"
 	"fmt"
 	"slices"
 
@@ -9,23 +10,21 @@ import (
 
 type BundleValidator []func(v1 *RegistryV1) []error
 
-func (v BundleValidator) Validate(rv1 *RegistryV1) []error {
+func (v BundleValidator) Validate(rv1 *RegistryV1) error {
 	var errs []error
 	for _, validator := range v {
 		errs = append(errs, validator(rv1)...)
 	}
-	return errs
+	return errors.Join(errs...)
 }
 
-func NewBundleValidator() BundleValidator {
+var RegistryV1BundleValidator = BundleValidator{
 	// NOTE: if you update this list, Test_BundleValidatorHasAllValidationFns will fail until
 	// you bring the same changes over to that test. This helps ensure all validation rules are executed
 	// while giving us the flexibility to test each validation function individually
-	return BundleValidator{
-		CheckDeploymentSpecUniqueness,
-		CheckCRDResourceUniqueness,
-		CheckOwnedCRDExistence,
-	}
+	CheckDeploymentSpecUniqueness,
+	CheckCRDResourceUniqueness,
+	CheckOwnedCRDExistence,
 }
 
 // CheckDeploymentSpecUniqueness checks that each strategy deployment spec in the csv has a unique name.

internal/operator-controller/rukpak/convert/validator_test.go

@@ -20,7 +20,7 @@ func Test_BundleValidatorHasAllValidationFns(t *testing.T) {
 		convert.CheckCRDResourceUniqueness,
 		convert.CheckOwnedCRDExistence,
 	}
-	actualValidationFns := convert.NewBundleValidator()
+	actualValidationFns := convert.RegistryV1BundleValidator
 
 	require.Equal(t, len(expectedValidationFns), len(actualValidationFns))
 	for i := range expectedValidationFns {
@@ -40,7 +40,7 @@ func Test_BundleValidatorCallsAllValidationFnsInOrder(t *testing.T) {
 			return nil
 		},
 	}
-	require.Empty(t, validator.Validate(nil))
+	require.NoError(t, validator.Validate(nil))
 	require.Equal(t, "hi", actual)
 }
 
@@ -53,8 +53,8 @@ func Test_CheckDeploymentSpecUniqueness(t *testing.T) {
 		{
 			name: "accepts bundles with unique deployment strategy spec names",
 			bundle: &convert.RegistryV1{
-				CSV: MakeCSV(
-					WithStrategyDeploymentSpecs(
+				CSV: makeCSV(
+					withStrategyDeploymentSpecs(
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"},
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"},
 					),
@@ -63,8 +63,8 @@ func Test_CheckDeploymentSpecUniqueness(t *testing.T) {
 		}, {
 			name: "rejects bundles with duplicate deployment strategy spec names",
 			bundle: &convert.RegistryV1{
-				CSV: MakeCSV(
-					WithStrategyDeploymentSpecs(
+				CSV: makeCSV(
+					withStrategyDeploymentSpecs(
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"},
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"},
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"},
@@ -77,8 +77,8 @@ func Test_CheckDeploymentSpecUniqueness(t *testing.T) {
 		}, {
 			name: "errors are ordered by deployment strategy spec name",
 			bundle: &convert.RegistryV1{
-				CSV: MakeCSV(
-					WithStrategyDeploymentSpecs(
+				CSV: makeCSV(
+					withStrategyDeploymentSpecs(
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-a"},
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-b"},
 						v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-c"},
@@ -157,8 +157,8 @@ func Test_CheckOwnedCRDExistence(t *testing.T) {
 					{ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}},
 					{ObjectMeta: metav1.ObjectMeta{Name: "b.crd.something"}},
 				},
-				CSV: MakeCSV(
-					WithOwnedCRDs(
+				CSV: makeCSV(
+					withOwnedCRDs(
 						v1alpha1.CRDDescription{Name: "a.crd.something"},
 						v1alpha1.CRDDescription{Name: "b.crd.something"},
 					),
@@ -168,8 +168,8 @@ func Test_CheckOwnedCRDExistence(t *testing.T) {
 			name: "rejects bundles with missing owned custom resource definition resources",
 			bundle: &convert.RegistryV1{
 				CRDs: []apiextensionsv1.CustomResourceDefinition{},
-				CSV: MakeCSV(
-					WithOwnedCRDs(v1alpha1.CRDDescription{Name: "a.crd.something"}),
+				CSV: makeCSV(
+					withOwnedCRDs(v1alpha1.CRDDescription{Name: "a.crd.something"}),
 				),
 			},
 			expectedErrs: []error{
@@ -179,8 +179,8 @@ func Test_CheckOwnedCRDExistence(t *testing.T) {
 			name: "errors are ordered by owned custom resource definition name",
 			bundle: &convert.RegistryV1{
 				CRDs: []apiextensionsv1.CustomResourceDefinition{},
-				CSV: MakeCSV(
-					WithOwnedCRDs(
+				CSV: makeCSV(
+					withOwnedCRDs(
 						v1alpha1.CRDDescription{Name: "a.crd.something"},
 						v1alpha1.CRDDescription{Name: "c.crd.something"},
 						v1alpha1.CRDDescription{Name: "b.crd.something"},
@@ -201,21 +201,21 @@ func Test_CheckOwnedCRDExistence(t *testing.T) {
 	}
 }
 
-type CSVOption func(version *v1alpha1.ClusterServiceVersion)
+type csvOption func(version *v1alpha1.ClusterServiceVersion)
 
-func WithStrategyDeploymentSpecs(strategyDeploymentSpecs ...v1alpha1.StrategyDeploymentSpec) CSVOption {
+func withStrategyDeploymentSpecs(strategyDeploymentSpecs ...v1alpha1.StrategyDeploymentSpec) csvOption {
 	return func(csv *v1alpha1.ClusterServiceVersion) {
 		csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs = strategyDeploymentSpecs
 	}
 }
 
-func WithOwnedCRDs(crdDesc ...v1alpha1.CRDDescription) CSVOption {
+func withOwnedCRDs(crdDesc ...v1alpha1.CRDDescription) csvOption {
 	return func(csv *v1alpha1.ClusterServiceVersion) {
 		csv.Spec.CustomResourceDefinitions.Owned = crdDesc
 	}
 }
 
-func MakeCSV(opts ...CSVOption) v1alpha1.ClusterServiceVersion {
+func makeCSV(opts ...csvOption) v1alpha1.ClusterServiceVersion {
 	csv := v1alpha1.ClusterServiceVersion{}
 	for _, opt := range opts {
 		opt(&csv)