🐛 fix channel naming validation (#231)

* fix: check for warning when the channel naming does not follow the convention

* applying nit suggestions

* fix-channel-check

* adressing nit review

pkg/manifests/bundleloader.go

@@ -19,9 +19,10 @@ import (
 
 // bundleLoader loads a bundle directory from disk
 type bundleLoader struct {
-	dir      string
-	bundle   *Bundle
-	foundCSV bool
+	dir             string
+	bundle          *Bundle
+	foundCSV        bool
+	annotationsFile AnnotationsFile
 }
 
 func NewBundleLoader(dir string) bundleLoader {
@@ -37,6 +38,7 @@ func (b *bundleLoader) LoadBundle() error {
 	}
 
 	errs = append(errs, b.calculateCompressedBundleSize())
+	b.addChannelsFromAnnotationsFile()
 
 	if !b.foundCSV {
 		errs = append(errs, fmt.Errorf("unable to find a csv in bundle directory %s", b.dir))
@@ -47,6 +49,21 @@ func (b *bundleLoader) LoadBundle() error {
 	return utilerrors.NewAggregate(errs)
 }
 
+// Add values from the annotations when the values are not loaded
+func (b *bundleLoader) addChannelsFromAnnotationsFile() {
+	// Note that they will not get load for Bundle Format directories
+	// and PackageManifest should not have the annotationsFile. However,
+	// the following check to ensure that channels and default channels
+	// are empty before set the annotations is just an extra precaution
+	channels := strings.Split(b.annotationsFile.Annotations.Channels, ",")
+	if len(channels) > 0 && len(b.bundle.Channels) == 0 {
+		b.bundle.Channels = channels
+	}
+	if len(b.annotationsFile.Annotations.DefaultChannelName) > 0 && len(b.bundle.DefaultChannel) == 0 {
+		b.bundle.DefaultChannel = b.annotationsFile.Annotations.DefaultChannelName
+	}
+}
+
 // Compress the bundle to check its size
 func (b *bundleLoader) calculateCompressedBundleSize() error {
 	if b.bundle == nil {
@@ -108,6 +125,19 @@ func (b *bundleLoader) LoadBundleWalkFunc(path string, f os.FileInfo, err error)
 		return nil
 	}
 
+	annotationsFile := AnnotationsFile{}
+	if strings.HasPrefix(f.Name(), "annotations") {
+		annFile, err := os.ReadFile(path)
+		if err != nil {
+			return err
+		}
+		if err := yaml.Unmarshal(annFile, &annotationsFile); err == nil {
+			b.annotationsFile = annotationsFile
+		} else {
+			return fmt.Errorf("unable to load the annotations file %s: %s", path, err)
+		}
+	}
+
 	fileReader, err := os.Open(path)
 	if err != nil {
 		return fmt.Errorf("unable to load file %s: %s", path, err)

pkg/validation/internal/good_practices.go

@@ -3,6 +3,7 @@ package internal
 import (
 	goerrors "errors"
 	"fmt"
+	"strings"
 
 	"github.com/operator-framework/api/pkg/manifests"
 	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -17,6 +18,9 @@ import (
 // This validator will raise an WARNING when:
 //
 // - The resources request for CPU and/or Memory are not defined for any of the containers found in the CSV
+//
+// - The channel names seems are not following the convention https://olm.operatorframework.io/docs/best-practices/channel-naming/
+//
 var GoodPracticesValidator interfaces.Validator = interfaces.ValidatorFunc(goodPracticesValidator)
 
 func goodPracticesValidator(objs ...interface{}) (results []errors.ManifestResult) {
@@ -51,6 +55,10 @@ func validateGoodPracticesFrom(bundle *manifests.Bundle) errors.ManifestResult {
 		result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName()))
 	}
 
+	channels := append(bundle.Channels, bundle.DefaultChannel)
+	if warn := validateHubChannels(channels); warn != nil {
+		result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName()))
+	}
 	return result
 }
 
@@ -75,3 +83,46 @@ func validateResourceRequests(csv *operatorsv1alpha1.ClusterServiceVersion) (err
 	}
 	return errs, warns
 }
+
+// validateHubChannels will check the channels. The motivation for the following check is to ensure that operators
+// authors knows if their operator bundles are or not respecting the Naming Convention Rules.
+// However, the operator authors still able to choose the names as please them.
+func validateHubChannels(channels []string) error {
+	const candidate = "candidate"
+	const stable = "stable"
+	const fast = "fast"
+
+	channels = getUniqueValues(channels)
+	var channelsNotFollowingConventional []string
+	for _, channel := range channels {
+		if !strings.HasPrefix(channel, candidate) &&
+			!strings.HasPrefix(channel, stable) &&
+			!strings.HasPrefix(channel, fast) &&
+			channel != "" {
+			channelsNotFollowingConventional = append(channelsNotFollowingConventional, channel)
+		}
+
+	}
+
+	if len(channelsNotFollowingConventional) > 0 {
+		return fmt.Errorf("channel(s) %+q are not following the recommended naming convention: "+
+			"https://olm.operatorframework.io/docs/best-practices/channel-naming",
+			channelsNotFollowingConventional)
+	}
+
+	return nil
+}
+
+// getUniqueValues return the values without duplicates
+func getUniqueValues(array []string) []string {
+	var result []string
+	uniqueValues := make(map[string]string)
+	for _, n := range array {
+		uniqueValues[strings.TrimSpace(n)] = ""
+	}
+
+	for k, _ := range uniqueValues {
+		result = append(result, k)
+	}
+	return result
+}

pkg/validation/internal/good_practices_test.go

@@ -62,6 +62,14 @@ func Test_ValidateGoodPractices(t *testing.T) {
 			},
 			errStrings: []string{"Error: Value etcdoperator.v0.9.4: unable to find a deployment to install in the CSV"},
 		},
+		{
+			name:        "should raise an warn when the channel does not follows the convention",
+			wantWarning: true,
+			args: args{
+				bundleDir: "./testdata/bundle_with_metadata",
+			},
+			warnStrings: []string{"Warning: Value memcached-operator.v0.0.1: channel(s) [\"alpha\"] are not following the recommended naming convention: https://olm.operatorframework.io/docs/best-practices/channel-naming"},
+		},
 	}
 
 	for _, tt := range tests {
@@ -92,3 +100,50 @@ func Test_ValidateGoodPractices(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateHubChannels(t *testing.T) {
+	type args struct {
+		channels []string
+	}
+	tests := []struct {
+		name        string
+		args        args
+		wantWarn    bool
+		warnStrings []string
+	}{
+		{
+			name: "should not return warning when the channel names following the convention",
+			args: args{
+				channels: []string{"fast", "candidate"},
+			},
+			wantWarn: false,
+		},
+		{
+			name: "should return warning when the channel names are NOT following the convention",
+			args: args{
+				channels: []string{"mychannel-4.5"},
+			},
+			wantWarn:    true,
+			warnStrings: []string{"channel(s) [\"mychannel-4.5\"] are not following the recommended naming convention: https://olm.operatorframework.io/docs/best-practices/channel-naming"},
+		},
+		{
+			name: "should return warning when has 1 channel NOT following the convention along the others which follows up",
+			args: args{
+				channels: []string{"alpha", "fast-v2.1", "candidate-v2.2"},
+			},
+			wantWarn:    true,
+			warnStrings: []string{"channel(s) [\"alpha\"] are not following the recommended naming convention: https://olm.operatorframework.io/docs/best-practices/channel-naming"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateHubChannels(tt.args.channels)
+			if (err != nil) != tt.wantWarn {
+				t.Errorf("validateHubChannels() error = %v, wantWarn %v", err, tt.wantWarn)
+			}
+			if len(tt.warnStrings) > 0 {
+				require.Contains(t, tt.warnStrings, err.Error())
+			}
+		})
+	}
+}

pkg/validation/internal/operatorhub.go

@@ -215,40 +215,9 @@ func validateBundleOperatorHub(bundle *manifests.Bundle, k8sVersion string) erro
 		result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName()))
 	}
 
-	if warn := validateHubChannels(bundle.Channels); warn != nil {
-		result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName()))
-	}
-
 	return result
 }
 
-// validateHubChannels will check the channels. The motivation for the following check is to ensure that operators
-// authors knows if their operator bundles are or not respecting the Naming Convention Rules.
-// However, the operator authors still able to choose the names as please them.
-func validateHubChannels(channels []string) error {
-	const candidate = "candidate"
-	const stable = "stable"
-	const fast = "fast"
-
-	var channelsNotFollowingConventional []string
-	for _, channel := range channels {
-		if !strings.HasPrefix(channel, candidate) &&
-			!strings.HasPrefix(channel, stable) &&
-			!strings.HasPrefix(channel, fast) {
-			channelsNotFollowingConventional = append(channelsNotFollowingConventional, channel)
-		}
-
-	}
-
-	if len(channelsNotFollowingConventional) > 0 {
-		return fmt.Errorf("channel(s) %+q are not following the recommended naming convention: "+
-			"https://olm.operatorframework.io/docs/best-practices/channel-naming",
-			channelsNotFollowingConventional)
-	}
-
-	return nil
-}
-
 // validateHubCSVSpec will check the CSV against the criteria to publish an
 // operator bundle in the OperatorHub.io
 func validateHubCSVSpec(csv v1alpha1.ClusterServiceVersion) CSVChecks {

pkg/validation/internal/operatorhub_test.go

@@ -342,36 +342,3 @@ func TestCheckSpecMinKubeVersion(t *testing.T) {
 		})
 	}
 }
-
-func TestValidateHubChannels(t *testing.T) {
-	type args struct {
-		channels []string
-	}
-	tests := []struct {
-		name     string
-		args     args
-		wantWarn bool
-	}{
-		{
-			name: "should not return warning when the channel names following the convention",
-			args: args{
-				channels: []string{"fast", "candidate"},
-			},
-			wantWarn: false,
-		},
-		{
-			name: "should return warning when the channel names are NOT following the convention",
-			args: args{
-				channels: []string{"mychannel-4.5"},
-			},
-			wantWarn: true,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if err := validateHubChannels(tt.args.channels); (err != nil) != tt.wantWarn {
-				t.Errorf("validateHubChannels() error = %v, wantWarn %v", err, tt.wantWarn)
-			}
-		})
-	}
-}

pkg/validation/internal/testdata/bundle_with_metadata/manifests/cache.example.com_memcacheds.yaml

@@ -0,0 +1,66 @@
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  annotations:
+    controller-gen.kubebuilder.io/version: v0.8.0
+  creationTimestamp: null
+  name: memcacheds.cache.example.com
+spec:
+  group: cache.example.com
+  names:
+    kind: Memcached
+    listKind: MemcachedList
+    plural: memcacheds
+    singular: bundle_with_metadata
+  scope: Namespaced
+  versions:
+  - name: v1alpha1
+    schema:
+      openAPIV3Schema:
+        description: Memcached is the Schema for the memcacheds API
+        properties:
+          apiVersion:
+            description: 'APIVersion defines the versioned schema of this representation
+              of an object. Servers should convert recognized schemas to the latest
+              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+            type: string
+          kind:
+            description: 'Kind is a string value representing the REST resource this
+              object represents. Servers may infer this from the endpoint the client
+              submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+            type: string
+          metadata:
+            type: object
+          spec:
+            description: MemcachedSpec defines the desired state of Memcached
+            properties:
+              foo:
+                description: Foo is an example field of Memcached. Edit memcached_types.go
+                  to remove/update
+                type: string
+              size:
+                description: Size defines the number of Memcached instances
+                format: int32
+                type: integer
+            type: object
+          status:
+            description: MemcachedStatus defines the observed state of Memcached
+            properties:
+              nodes:
+                description: Nodes store the name of the pods which are running Memcached
+                  instances
+                items:
+                  type: string
+                type: array
+            type: object
+        type: object
+    served: true
+    storage: true
+    subresources:
+      status: {}
+status:
+  acceptedNames:
+    kind: ""
+    plural: ""
+  conditions: []
+  storedVersions: []