internal/util/operator-registry: fix default channel check (#2116)

* internal/util/operator-registry: error if package manifest has no default channel and more than one channel

* CHANGELOG.md: add default channel bug fix

* internal/util/operator-registry: add test cases for default channels

Author: Eric Stroczynski

CHANGELOG.md

@@ -22,8 +22,9 @@
 ### Bug Fixes
 
 - OLM internal manager is not returning errors in the initialization. ([#1976](https://github.com/operator-framework/operator-sdk/pull/1976))
-- Added missing default role permission for `deployments`, which is required to create the metrics service for the operator. ([#2090](https://github.com/operator-framework/operator-sdk/pull/2090)) 
+- Added missing default role permission for `deployments`, which is required to create the metrics service for the operator. ([#2090](https://github.com/operator-framework/operator-sdk/pull/2090))
 - Handle invalid maxArtifacts annotation on CRs for Ansible based operators. ([2093](https://github.com/operator-framework/operator-sdk/pull/2093))
+- When validating package manifests, only return an error if default channel is not set and more than one channel is available. ([#2116](https://github.com/operator-framework/operator-sdk/pull/2116))
 
 ## v0.11.0
 
@@ -78,7 +79,7 @@
       ```
 - [`pkg/test.FrameworkClient`](https://github.com/operator-framework/operator-sdk/blob/master/pkg/test/client.go#L33) methods `List()` and `Delete()` have new signatures corresponding to the homonymous methods of `sigs.k8s.io/controller-runtime/pkg/client.Client`. ([#1876](https://github.com/operator-framework/operator-sdk/pull/1876))
 - CRD file names were previously of the form `<group>_<version>_<kind>_crd.yaml`. Now that CRD manifest `spec.version` is deprecated in favor of `spec.versions`, i.e. multiple versions can be specified in one CRD, CRD file names have the form `<full group>_<resource>_crd.yaml`. `<full group>` is the full group name of your CRD while `<group>` is the last subdomain of `<full group>`, ex. `foo.bar.com` vs `foo`. `<resource>` is the plural lower-case CRD Kind found at `spec.names.plural`. ([#1876](https://github.com/operator-framework/operator-sdk/pull/1876))
-- Upgrade Python version from `2.7` to `3.6`, Ansible version from `2.8.0` to `~=2.8` and ansible-runner from `1.2` to `1.3.4` in the Ansible based images. ([#1947](https://github.com/operator-framework/operator-sdk/pull/1947)) 
+- Upgrade Python version from `2.7` to `3.6`, Ansible version from `2.8.0` to `~=2.8` and ansible-runner from `1.2` to `1.3.4` in the Ansible based images. ([#1947](https://github.com/operator-framework/operator-sdk/pull/1947))
 - Made the default scorecard version `v1alpha2` which is new for this release and could break users that were parsing the older scorecard output (`v1alpha1`).  Users can still specify version `v1alpha1` on the scorecard configuration to use the older style for some period of time until `v1alpha1` is removed.
 - Replaced `pkg/kube-metrics.NewCollectors()` with `pkg/kube-metrics.NewMetricsStores()` and changed exported function signature for `pkg/kube-metrics.ServeMetrics()` due to a [breaking change in kube-state-metrics](https://github.com/kubernetes/kube-state-metrics/pull/786). ([#1943](https://github.com/operator-framework/operator-sdk/pull/1943))
 

internal/util/operator-registry/package_manifest.go

@@ -21,19 +21,20 @@ import (
 	"github.com/pkg/errors"
 )
 
-func ValidatePackageManifest(pm *registry.PackageManifest) error {
-	if pm.PackageName == "" {
+func ValidatePackageManifest(pkg *registry.PackageManifest) error {
+	if pkg.PackageName == "" {
 		return errors.New("package name cannot be empty")
 	}
-	if len(pm.Channels) == 0 {
+	numChannels := len(pkg.Channels)
+	if numChannels == 0 {
 		return errors.New("channels cannot be empty")
 	}
-	if pm.DefaultChannelName == "" {
+	if pkg.DefaultChannelName == "" && numChannels > 1 {
 		return errors.New("default channel cannot be empty")
 	}
 
 	seen := map[string]struct{}{}
-	for i, c := range pm.Channels {
+	for i, c := range pkg.Channels {
 		if c.Name == "" {
 			return fmt.Errorf("channel %d name cannot be empty", i)
 		}
@@ -45,8 +46,8 @@ func ValidatePackageManifest(pm *registry.PackageManifest) error {
 		}
 		seen[c.Name] = struct{}{}
 	}
-	if _, ok := seen[pm.DefaultChannelName]; !ok {
-		return fmt.Errorf("default channel %s does not exist in channels", pm.DefaultChannelName)
+	if _, found := seen[pkg.DefaultChannelName]; pkg.DefaultChannelName != "" && !found {
+		return fmt.Errorf("default channel %s does not exist in channels", pkg.DefaultChannelName)
 	}
 
 	return nil

internal/util/operator-registry/package_manifest_test.go

@@ -21,62 +21,89 @@ import (
 )
 
 func TestValidatePackageManifest(t *testing.T) {
-	channels := []registry.PackageChannel{
-		{Name: "foo", CurrentCSVName: "bar"},
-	}
-	pm := &registry.PackageManifest{
-		Channels:           channels,
-		DefaultChannelName: "baz",
-		PackageName:        "test-package",
-	}
-
 	cases := []struct {
 		description string
 		wantErr     bool
 		errMsg      string
-		operation   func(*registry.PackageManifest)
+		pkg         *registry.PackageManifest
 	}{
 		{
-			"default channel does not exist",
-			true, "default channel baz does not exist in channels", nil,
+			"successful validation",
+			false, "",
+			&registry.PackageManifest{
+				Channels: []registry.PackageChannel{
+					{Name: "foo", CurrentCSVName: "bar"},
+				},
+				DefaultChannelName: "foo",
+				PackageName:        "test-package",
+			},
 		},
 		{
-			"successful validation",
+			"successful validation no default channel with only one channel",
 			false, "",
-			func(pm *registry.PackageManifest) {
-				pm.DefaultChannelName = pm.Channels[0].Name
+			&registry.PackageManifest{
+				Channels: []registry.PackageChannel{
+					{Name: "foo", CurrentCSVName: "bar"},
+				},
+				PackageName: "test-package",
+			},
+		},
+		{
+			"no default channel and more than one channel",
+			true, "default channel cannot be empty",
+			&registry.PackageManifest{
+				Channels: []registry.PackageChannel{
+					{Name: "foo", CurrentCSVName: "bar"},
+					{Name: "foo2", CurrentCSVName: "baz"},
+				},
+				PackageName: "test-package",
+			},
+		},
+		{
+			"default channel does not exist",
+			true, "default channel baz does not exist in channels",
+			&registry.PackageManifest{
+				Channels: []registry.PackageChannel{
+					{Name: "foo", CurrentCSVName: "bar"},
+				},
+				DefaultChannelName: "baz",
+				PackageName:        "test-package",
 			},
 		},
 		{
 			"channels are empty",
 			true, "channels cannot be empty",
-			func(pm *registry.PackageManifest) {
-				pm.Channels = nil
+			&registry.PackageManifest{
+				Channels:           nil,
+				DefaultChannelName: "baz",
+				PackageName:        "test-package",
 			},
 		},
 		{
 			"one channel's CSVName is empty",
 			true, "channel foo currentCSV cannot be empty",
-			func(pm *registry.PackageManifest) {
-				pm.Channels = make([]registry.PackageChannel, 1)
-				copy(pm.Channels, channels)
-				pm.Channels[0].CurrentCSVName = ""
+			&registry.PackageManifest{
+				Channels:           []registry.PackageChannel{{Name: "foo"}},
+				DefaultChannelName: "baz",
+				PackageName:        "test-package",
 			},
 		},
 		{
 			"duplicate channel name",
 			true, "duplicate package manifest channel name foo; channel names must be unique",
-			func(pm *registry.PackageManifest) {
-				pm.Channels = append(channels, channels...)
+			&registry.PackageManifest{
+				Channels: []registry.PackageChannel{
+					{Name: "foo", CurrentCSVName: "bar"},
+					{Name: "foo", CurrentCSVName: "baz"},
+				},
+				DefaultChannelName: "baz",
+				PackageName:        "test-package",
 			},
 		},
 	}
 
 	for _, c := range cases {
-		if c.operation != nil {
-			c.operation(pm)
-		}
-		err := ValidatePackageManifest(pm)
+		err := ValidatePackageManifest(c.pkg)
 		if c.wantErr {
 			if err == nil {
 				t.Errorf(`%s: expected error "%s", got none`, c.description, c.errMsg)