Merge pull request #451 from dinhxuanvu/default-chan

Bug 1878213: Update index add func to handle optional default channel

Author: openshift-merge-robot

docs/design/operator-bundle.md

@@ -45,7 +45,7 @@ We use the following labels to annotate the operator bundle image.
     * The value `metadata.v1` implies that this bundle has operator metadata.
 * The label `operators.operatorframework.io.bundle.package.v1` reflects the package name of the bundle.
 * The label `operators.operatorframework.io.bundle.channels.v1` reflects the list of channels the bundle is subscribing to when added into an operator registry
-* The label `operators.operatorframework.io.bundle.channel.default.v1` reflects the default channel an operator should be subscribed to when installed from a registry
+* The label `operators.operatorframework.io.bundle.channel.default.v1` reflects the default channel an operator should be subscribed to when installed from a registry. This label is optional if the default channel has been set by previous bundles and the default channel is unchanged for this bundle.
 
 The labels will also be put inside a YAML file, as shown below.
 
@@ -240,7 +240,7 @@ $ ./opm alpha bundle build --directory /test/0.1.0/ --tag quay.io/coreos/test-op
 --image-builder podman --package test-operator --channels stable,beta --default stable
 ```
 
-The `--package` or `-p` is the name of package for the operator such as `etcd` which maps `channels` to a particular application definition. `channels` allow package authors to write different upgrade paths for different users (e.g. `beta` vs. `stable`). The `channels` list is provided via `--channels` or `-c` flag. Multiple `channels` are separated by a comma (`,`). The default channel is provided optionally via `--default` or `-e` flag. If the default channel is not provided, the first channel in channel list is selected as default.
+The `--package` or `-p` is the name of package for the operator such as `etcd` which maps `channels` to a particular application definition. `channels` allow package authors to write different upgrade paths for different users (e.g. `beta` vs. `stable`). The `channels` list is provided via `--channels` or `-c` flag. Multiple `channels` are separated by a comma (`,`). The default channel is provided optionally via `--default` or `-e` flag.
 
 *Notes:*
 * If there is `Dockerfile` existing in the directory, it will be overwritten.

pkg/lib/bundle/generate.go

@@ -112,9 +112,6 @@ func GenerateFunc(directory, outputDir, packageName, channels, channelDefault st
 
 		if channelDefault == "" {
 			channelDefault = i.GetDefaultChannel()
-			if !containsString(strings.Split(channels, ","), channelDefault) {
-				channelDefault = ""
-			}
 			log.Infof("Inferred default channel: %s", channelDefault)
 		}
 	}
@@ -299,16 +296,18 @@ func ValidateAnnotations(existing, expected []byte) error {
 func GenerateAnnotations(mediaType, manifests, metadata, packageName, channels, channelDefault string) ([]byte, error) {
 	annotations := &AnnotationMetadata{
 		Annotations: map[string]string{
-			MediatypeLabel:      mediaType,
-			ManifestsLabel:      manifests,
-			MetadataLabel:       metadata,
-			PackageLabel:        packageName,
-			ChannelsLabel:       channels,
-			ChannelDefaultLabel: channelDefault,
+			MediatypeLabel: mediaType,
+			ManifestsLabel: manifests,
+			MetadataLabel:  metadata,
+			PackageLabel:   packageName,
+			ChannelsLabel:  channels,
 		},
 	}
 
-	annotations.Annotations[ChannelDefaultLabel] = channelDefault
+	// Only add defaultChannel annotation if present
+	if channelDefault != "" {
+		annotations.Annotations[ChannelDefaultLabel] = channelDefault
+	}
 
 	afile, err := yaml.Marshal(annotations)
 	if err != nil {
@@ -343,7 +342,11 @@ func GenerateDockerfile(mediaType, manifests, metadata, copyManifestDir, copyMet
 	fileContent += fmt.Sprintf("LABEL %s=%s\n", MetadataLabel, metadata)
 	fileContent += fmt.Sprintf("LABEL %s=%s\n", PackageLabel, packageName)
 	fileContent += fmt.Sprintf("LABEL %s=%s\n", ChannelsLabel, channels)
-	fileContent += fmt.Sprintf("LABEL %s=%s\n\n", ChannelDefaultLabel, channelDefault)
+
+	// Only add defaultChannel annotation if present
+	if channelDefault != "" {
+		fileContent += fmt.Sprintf("LABEL %s=%s\n\n", ChannelDefaultLabel, channelDefault)
+	}
 
 	// CONTENT
 	fileContent += fmt.Sprintf("COPY %s %s\n", relativeManifestDirectory, "/manifests/")
@@ -352,7 +355,7 @@ func GenerateDockerfile(mediaType, manifests, metadata, copyManifestDir, copyMet
 	return []byte(fileContent), nil
 }
 
-// Write `fileName` file with `content` into a `directory`
+// WriteFile writes `fileName` file with `content` into a `directory`
 // Note: Will overwrite the existing `fileName` file if it exists
 func WriteFile(fileName, directory string, content []byte) error {
 	if _, err := os.Stat(directory); os.IsNotExist(err) {

pkg/lib/bundle/generate_test.go

@@ -170,7 +170,6 @@ func TestGenerateDockerfileFunc(t *testing.T) {
 		"LABEL operators.operatorframework.io.bundle.metadata.v1=%s\n"+
 		"LABEL operators.operatorframework.io.bundle.package.v1=test4\n"+
 		"LABEL operators.operatorframework.io.bundle.channels.v1=test5\n"+
-		"LABEL operators.operatorframework.io.bundle.channel.default.v1=\n\n"+
 		"COPY test2 /manifests/\n"+
 		"COPY metadata /metadata/\n", MetadataDir)
 
@@ -255,7 +254,7 @@ func TestGenerateFunc(t *testing.T) {
 	os.Remove(filepath.Join("./", DockerFile))
 
 	output := fmt.Sprintf("annotations:\n" +
-		"  operators.operatorframework.io.bundle.channel.default.v1: \"\"\n" +
+		"  operators.operatorframework.io.bundle.channel.default.v1: alpha\n" +
 		"  operators.operatorframework.io.bundle.channels.v1: beta\n" +
 		"  operators.operatorframework.io.bundle.manifests.v1: manifests/\n" +
 		"  operators.operatorframework.io.bundle.mediatype.v1: registry+v1\n" +

pkg/lib/bundle/validate.go

@@ -180,7 +180,7 @@ func validateAnnotations(mediaType string, fileAnnotations *AnnotationMetadata)
 
 	for label, item := range annotations {
 		val, ok := fileAnnotations.Annotations[label]
-		if !ok {
+		if !ok && label != ChannelDefaultLabel {
 			aErr := fmt.Errorf("Missing annotation %q", label)
 			validationErrors = append(validationErrors, aErr)
 		}
@@ -205,11 +205,12 @@ func validateAnnotations(mediaType string, fileAnnotations *AnnotationMetadata)
 			if val == "" {
 				aErr := fmt.Errorf("Expecting annotation %q to have non-empty value", label)
 				validationErrors = append(validationErrors, aErr)
-			} else {
-				annotations[label] = val
 			}
 		case ChannelDefaultLabel:
-			annotations[label] = val
+			if val == "" {
+				aErr := fmt.Errorf("Expecting annotation %q to have non-empty value", label)
+				validationErrors = append(validationErrors, aErr)
+			}
 		}
 	}
 

pkg/registry/populator.go

@@ -223,22 +223,12 @@ func (i *DirectoryPopulator) loadManifests(imagesToAdd []*ImageInput, imagesToRe
 }
 
 func (i *DirectoryPopulator) loadManifestsReplaces(bundle *Bundle, annotationsFile *AnnotationsFile) error {
-	channels, err := i.querier.ListChannels(context.TODO(), annotationsFile.GetName())
-	existingPackageChannels := map[string]string{}
-	for _, c := range channels {
-		current, err := i.querier.GetCurrentCSVNameForChannel(context.TODO(), annotationsFile.GetName(), c)
-		if err != nil {
-			return err
-		}
-		existingPackageChannels[c] = current
-	}
-
 	bcsv, err := bundle.ClusterServiceVersion()
 	if err != nil {
 		return fmt.Errorf("error getting csv from bundle %s: %s", bundle.Name, err)
 	}
 
-	packageManifest, err := translateAnnotationsIntoPackage(annotationsFile, bcsv, existingPackageChannels)
+	packageManifest, err := i.translateAnnotationsIntoPackage(annotationsFile, bcsv)
 	if err != nil {
 		return fmt.Errorf("Could not translate annotations file into packageManifest %s", err)
 	}
@@ -430,15 +420,23 @@ func (i *DirectoryPopulator) loadOperatorBundle(manifest PackageManifest, bundle
 }
 
 // translateAnnotationsIntoPackage attempts to translate the channels.yaml file at the given path into a package.yaml
-func translateAnnotationsIntoPackage(annotations *AnnotationsFile, csv *ClusterServiceVersion, existingPackageChannels map[string]string) (PackageManifest, error) {
+func (i *DirectoryPopulator) translateAnnotationsIntoPackage(annotations *AnnotationsFile, csv *ClusterServiceVersion) (PackageManifest, error) {
 	manifest := PackageManifest{}
+	existingChannels := map[string]string{}
+
+	pkgm, err := i.querier.GetPackage(context.TODO(), annotations.GetName())
+	if err == nil {
+		for _, c := range pkgm.Channels {
+			existingChannels[c.Name] = c.CurrentCSVName
+		}
+	}
 
 	for _, ch := range annotations.GetChannels() {
-		existingPackageChannels[ch] = csv.GetName()
+		existingChannels[ch] = csv.GetName()
 	}
 
 	channels := []PackageChannel{}
-	for c, current := range existingPackageChannels {
+	for c, current := range existingChannels {
 		channels = append(channels,
 			PackageChannel{
 				Name:           c,
@@ -447,9 +445,29 @@ func translateAnnotationsIntoPackage(annotations *AnnotationsFile, csv *ClusterS
 	}
 
 	manifest = PackageManifest{
-		PackageName:        annotations.GetName(),
-		DefaultChannelName: annotations.GetDefaultChannelName(),
-		Channels:           channels,
+		PackageName: annotations.GetName(),
+		Channels:    channels,
+	}
+
+	defaultChan := annotations.GetDefaultChannelName()
+	if defaultChan != "" {
+		if _, found := existingChannels[defaultChan]; found {
+			manifest.DefaultChannelName = annotations.GetDefaultChannelName()
+		} else {
+			return manifest, fmt.Errorf("Channel %s is set as default in annotations but not found in existing package channels", defaultChan)
+		}
+	} else {
+		// No default channel is provided in annotations. Attempt to infer from package manifest
+		if pkgm != nil {
+			manifest.DefaultChannelName = pkgm.GetDefaultChannel()
+		} else {
+			// Infer default channel if only one channel is provided
+			if len(annotations.GetChannels()) == 1 {
+				manifest.DefaultChannelName = annotations.GetChannels()[0]
+			} else {
+				return manifest, fmt.Errorf("Default channel is missing and can't be inferred")
+			}
+		}
 	}
 
 	return manifest, nil

pkg/registry/types.go

@@ -342,12 +342,5 @@ func (a *AnnotationsFile) GetChannels() []string {
 
 // GetDefaultChannelName returns the name of the default channel
 func (a *AnnotationsFile) GetDefaultChannelName() string {
-	if a.Annotations.DefaultChannelName != "" {
-		return a.Annotations.DefaultChannelName
-	}
-	channels := a.GetChannels()
-	if len(channels) == 1 {
-		return channels[0]
-	}
-	return ""
+	return a.Annotations.DefaultChannelName
 }