Improve OLM and Tekton trackers errors handling (#1290)

Signed-off-by: Sergio Castaño Arteaga <tegioz@icloud.com>

.golangci.yml

@@ -15,11 +15,9 @@ linters:
     - gosimple
     - govet
     - ineffassign
-    - interfacer
     - misspell
     - nakedret
     - prealloc
-    - scopelint
     - staticcheck
     - structcheck
     - stylecheck

internal/tracker/source/olm/olm.go

@@ -115,7 +115,11 @@ func (s *TrackerSource) GetPackagesAvailable() (map[string]*hub.Package, error)
 			}
 
 			// Prepare and store package version
-			p := s.preparePackage(s.i.Repository, manifest, csv, csvData)
+			p, err := s.preparePackage(s.i.Repository, manifest, csv, csvData)
+			if err != nil {
+				s.warn(fmt.Errorf("error preparing package %s version %s: %w", pkgName, version, err))
+				continue
+			}
 			packagesAvailable[pkg.BuildKey(p)] = p
 		}
 
@@ -134,25 +138,31 @@ func (s *TrackerSource) preparePackage(
 	manifest *manifests.PackageManifest,
 	csv *operatorsv1alpha1.ClusterServiceVersion,
 	csvData []byte,
-) *hub.Package {
+) (*hub.Package, error) {
 	// Prepare package from manifest and csv
 	p := &hub.Package{
-		Name:             manifest.PackageName,
+		Name:           manifest.PackageName,
-		DisplayName:      csv.Spec.DisplayName,
+		DisplayName:    csv.Spec.DisplayName,
-		Description:      csv.Annotations["description"],
+		Description:    csv.Annotations["description"],
-		Keywords:         csv.Spec.Keywords,
+		Keywords:       csv.Spec.Keywords,
-		Readme:           csv.Spec.Description,
+		Readme:         csv.Spec.Description,
-		Version:          csv.Spec.Version.String(),
+		Version:        csv.Spec.Version.String(),
-		IsOperator:       true,
+		IsOperator:     true,
-		Capabilities:     csv.Annotations["capabilities"],
+		Capabilities:   csv.Annotations["capabilities"],
-		DefaultChannel:   manifest.DefaultChannelName,
+		DefaultChannel: manifest.DefaultChannelName,
-		License:          csv.Annotations[licenseAnnotation],
+		License:        csv.Annotations[licenseAnnotation],
-		Provider:         csv.Spec.Provider.Name,
+		Provider:       csv.Spec.Provider.Name,
-		ContainersImages: getContainersImages(csv, csvData),
+		Install:        csv.Annotations[installAnnotation],
-		Install:          csv.Annotations[installAnnotation],
+		Repository:     r,
-		Repository:       r,
 	}
 
+	// Containers images
+	containersImages, err := getContainersImages(csv, csvData)
+	if err != nil {
+		return nil, err
+	}
+	p.ContainersImages = containersImages
+
 	// TS
 	ts, err := time.Parse(time.RFC3339, csv.Annotations["createdAt"])
 	if err == nil {
@@ -241,38 +251,54 @@ func (s *TrackerSource) preparePackage(
 		p.CRDsExamples = crdsExamples
 	}
 
+	// Changes
+	if v, ok := csv.Annotations[changesAnnotation]; ok {
+		changes, err := source.ParseChangesAnnotation(v)
+		if err != nil {
+			return nil, err
+		}
+		p.Changes = changes
+	}
+
+	// Prerelease
+	if v, ok := csv.Annotations[prereleaseAnnotation]; ok {
+		prerelease, err := strconv.ParseBool(v)
+		if err != nil {
+			return nil, fmt.Errorf("invalid prerelease value: %s", v)
+		}
+		p.Prerelease = prerelease
+	}
+
 	// Recommendations
 	if v, ok := csv.Annotations[recommendationsAnnotation]; ok {
 		var recommendations []*hub.Recommendation
-		if err := yaml.Unmarshal([]byte(v), &recommendations); err == nil {
+		if err := yaml.Unmarshal([]byte(v), &recommendations); err != nil {
-			p.Recommendations = recommendations
+			return nil, fmt.Errorf("invalid recommendations value: %s", v)
 		}
+		p.Recommendations = recommendations
 	}
 
-	// Misc
+	// Security updates
+	if v, ok := csv.Annotations[securityUpdatesAnnotation]; ok {
+		containsSecurityUpdates, err := strconv.ParseBool(v)
+		if err != nil {
+			return nil, fmt.Errorf("invalid containsSecurityUpdates value: %s", v)
+		}
+		p.ContainsSecurityUpdates = containsSecurityUpdates
+	}
+
+	// Prepare data specific to the package kind
 	var isGlobalOperator bool
 	for _, e := range csv.Spec.InstallModes {
 		if e.Type == operatorsv1alpha1.InstallModeTypeAllNamespaces && e.Supported {
 			isGlobalOperator = true
 		}
 	}
-	changes, err := source.ParseChangesAnnotation(csv.Annotations[changesAnnotation])
-	if err == nil {
-		p.Changes = changes
-	}
-	containsSecurityUpdates, err := strconv.ParseBool(csv.Annotations[securityUpdatesAnnotation])
-	if err == nil {
-		p.ContainsSecurityUpdates = containsSecurityUpdates
-	}
-	prerelease, err := strconv.ParseBool(csv.Annotations[prereleaseAnnotation])
-	if err == nil {
-		p.Prerelease = prerelease
-	}
 	p.Data = map[string]interface{}{
 		"isGlobalOperator": isGlobalOperator,
 	}
 
-	return p
+	return p, nil
 }
 
 // warn is a helper that sends the error provided to the errors collector and
@@ -335,7 +361,10 @@ func getCSV(path string) (*operatorsv1alpha1.ClusterServiceVersion, []byte, erro
 
 // getContainersImages returns all containers images declared in the csv data
 // provided.
-func getContainersImages(csv *operatorsv1alpha1.ClusterServiceVersion, csvData []byte) []*hub.ContainerImage {
+func getContainersImages(
+	csv *operatorsv1alpha1.ClusterServiceVersion,
+	csvData []byte,
+) ([]*hub.ContainerImage, error) {
 	var images []*hub.ContainerImage
 
 	// Container image annotation
@@ -354,8 +383,13 @@ func getContainersImages(csv *operatorsv1alpha1.ClusterServiceVersion, csvData [
 	if err := yaml.Unmarshal(csvData, &csvRI); err == nil {
 		images = append(images, csvRI.Spec.RelatedImages...)
 	}
-	var imagesWhitelist []string
+
-	if err := yaml.Unmarshal([]byte(csv.Annotations[imagesWhitelistAnnotation]), &imagesWhitelist); err == nil {
+	// Images whitelisting
+	if v, ok := csv.Annotations[imagesWhitelistAnnotation]; ok {
+		var imagesWhitelist []string
+		if err := yaml.Unmarshal([]byte(v), &imagesWhitelist); err != nil {
+			return nil, fmt.Errorf("invalid imagesWhitelist value: %s", v)
+		}
 		for _, image := range images {
 			if contains(imagesWhitelist, image.Image) {
 				image.Whitelisted = true
@@ -363,7 +397,7 @@ func getContainersImages(csv *operatorsv1alpha1.ClusterServiceVersion, csvData [
 		}
 	}
 
-	return images
+	return images, nil
 }
 
 // contains is a helper to check if a list contains the string provided.

internal/tracker/source/olm/olm_test.go

@@ -136,7 +136,7 @@ func TestTrackerSource(t *testing.T) {
 		sw.AssertExpectations(t)
 	})
 
-	t.Run("error saving logo image, package returned anyway", func(t *testing.T) {
+	t.Run("invalid changes annotation", func(t *testing.T) {
 		t.Parallel()
 
 		// Setup services and expectations
@@ -146,6 +146,26 @@ func TestTrackerSource(t *testing.T) {
 			BasePath:   "testdata/path3",
 			Svc:        sw.Svc,
 		}
+		expectedErr := "error preparing package test-operator version 0.1.0: invalid changes annotation: single string"
+		sw.Ec.On("Append", i.Repository.RepositoryID, expectedErr).Return()
+
+		// Run test and check expectations
+		packages, err := NewTrackerSource(i).GetPackagesAvailable()
+		assert.Equal(t, map[string]*hub.Package{}, packages)
+		assert.NoError(t, err)
+		sw.AssertExpectations(t)
+	})
+
+	t.Run("error saving logo image, package returned anyway", func(t *testing.T) {
+		t.Parallel()
+
+		// Setup services and expectations
+		sw := source.NewTestsServicesWrapper()
+		i := &hub.TrackerSourceInput{
+			Repository: &hub.Repository{},
+			BasePath:   "testdata/path4",
+			Svc:        sw.Svc,
+		}
 		sw.Is.On("SaveImage", sw.Svc.Ctx, imageData).Return("", tests.ErrFake)
 		expectedErr := "error saving package test-operator image: fake error for tests"
 		sw.Ec.On("Append", i.Repository.RepositoryID, expectedErr).Return()
@@ -168,7 +188,7 @@ func TestTrackerSource(t *testing.T) {
 		sw := source.NewTestsServicesWrapper()
 		i := &hub.TrackerSourceInput{
 			Repository: &hub.Repository{},
-			BasePath:   "testdata/path3",
+			BasePath:   "testdata/path4",
 			Svc:        sw.Svc,
 		}
 		sw.Is.On("SaveImage", sw.Svc.Ctx, imageData).Return("logoImageID", nil)

internal/tracker/source/olm/testdata/path3/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml

@@ -2,25 +2,7 @@ apiVersion: operators.coreos.com/v1alpha1
 kind: ClusterServiceVersion
 metadata:
   annotations:
-    artifacthub.io/changes: |
+    artifacthub.io/changes: single string
-      - feature 1
-      - fix 1
-    artifacthub.io/containsSecurityUpdates: "true"
-    artifacthub.io/imagesWhitelist: |
-      - registry.io/image2:1.0.0
-    artifacthub.io/install: |
-      Install instructions (markdown)
-    artifacthub.io/license: Apache-2.0
-    artifacthub.io/prerelease: "true"
-    artifacthub.io/recommendations: |
-      - url: https://artifacthub.io/packages/helm/artifact-hub/artifact-hub
-    capabilities: Basic Install
-    categories: Application Runtime
-    containerImage: repo.url:latest
-    createdAt: "2019-06-28T15:23:00Z"
-    description: This is just a test
-    repository: https://github.com/test/test-operator
-    alm-examples: '[{"apiVersion": "crds.com/v1", "kind": "Test"}]'
   name: test-operator.v0.1.0
   namespace: placeholder
 spec:
@@ -33,8 +15,6 @@ spec:
         displayName: Test
   description: Test Operator README
   displayName: Test Operator
-  icon:
-    - base64data: iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==
   installModes:
     - supported: true
       type: OwnNamespace

internal/tracker/source/olm/testdata/path4/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml

@@ -0,0 +1,64 @@
+apiVersion: operators.coreos.com/v1alpha1
+kind: ClusterServiceVersion
+metadata:
+  annotations:
+    artifacthub.io/changes: |
+      - feature 1
+      - fix 1
+    artifacthub.io/containsSecurityUpdates: "true"
+    artifacthub.io/imagesWhitelist: |
+      - registry.io/image2:1.0.0
+    artifacthub.io/install: |
+      Install instructions (markdown)
+    artifacthub.io/license: Apache-2.0
+    artifacthub.io/prerelease: "true"
+    artifacthub.io/recommendations: |
+      - url: https://artifacthub.io/packages/helm/artifact-hub/artifact-hub
+    capabilities: Basic Install
+    categories: Application Runtime
+    containerImage: repo.url:latest
+    createdAt: "2019-06-28T15:23:00Z"
+    description: This is just a test
+    repository: https://github.com/test/test-operator
+    alm-examples: '[{"apiVersion": "crds.com/v1", "kind": "Test"}]'
+  name: test-operator.v0.1.0
+  namespace: placeholder
+spec:
+  customresourcedefinitions:
+    owned:
+      - description: Test CRD
+        kind: Test
+        name: test.crds.com
+        version: v1
+        displayName: Test
+  description: Test Operator README
+  displayName: Test Operator
+  icon:
+    - base64data: iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==
+  installModes:
+    - supported: true
+      type: OwnNamespace
+    - supported: true
+      type: SingleNamespace
+    - supported: false
+      type: MultiNamespace
+    - supported: true
+      type: AllNamespaces
+  keywords:
+    - Test
+  links:
+    - name: Sample link
+      url: https://sample.link
+  maintainers:
+    - email: test@email.com
+      name: Test
+  provider:
+    name: Test
+  version: 0.1.0
+  install:
+    strategy: deployment
+  relatedImages:
+    - name: image1
+      image: registry.io/image1:1.0.0
+    - name: image2
+      image: registry.io/image2:1.0.0

internal/tracker/source/olm/testdata/path4/test-operator/test-operator.package.yaml

@@ -0,0 +1,5 @@
+channels:
+  - currentCSV: test-operator.v0.1.0
+    name: alpha
+defaultChannel: alpha
+packageName: test-operator

internal/tracker/source/tekton/tekton.go

@@ -214,9 +214,10 @@ func enrichPackageFromAnnotations(p *hub.Package, annotations map[string]string)
 	// Changes
 	if v, ok := annotations[changesAnnotation]; ok {
 		changes, err := source.ParseChangesAnnotation(v)
-		if err == nil {
+		if err != nil {
-			p.Changes = changes
+			return err
 		}
+		p.Changes = changes
 	}
 
 	// License