Repository: redhat-openshift-ecosystem/openshift-preflight
Pull request #630 (2022-05-12 13:09:20)

Refactor hasUniqueTagCheck to get rid of CraneEngine
The extra "CraneEngine" that should have never been name that was
problematic. It was introduced merely to test crane calls. A better
way to test crane is available, so let's get rid of some unnecessary
abstractions.

Signed-off-by: Brad P. Crochet <brad@redhat.com>

openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/630#" title="Author self-approved">bcrochet</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-openshift-ecosystem%2Fopenshift-preflight).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/OWNERS)~~ [bcrochet]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/certification/engine/engine.go b/certification/engine/engine.go
index e19b07b..95c5234 100644
--- a/certification/engine/engine.go
+++ b/certification/engine/engine.go
@@ -89,7 +89,7 @@ var (
 // Container checks
 var (
 	hasLicenseCheck        certification.Check = &containerpol.HasLicenseCheck{}
-	hasUniqueTagCheck      certification.Check = containerpol.NewHasUniqueTagCheck(internal.NewCraneEngine())
+	hasUniqueTagCheck      certification.Check = containerpol.NewHasUniqueTagCheck()
 	maxLayersCheck         certification.Check = &containerpol.MaxLayersCheck{}
 	hasNoProhibitedCheck   certification.Check = &containerpol.HasNoProhibitedPackagesCheck{}
 	hasRequiredLabelsCheck certification.Check = &containerpol.HasRequiredLabelsCheck{}
diff --git a/certification/internal/engine/crane.go b/certification/internal/engine/crane.go
deleted file mode 100644
index b0943cf..0000000
--- a/certification/internal/engine/crane.go
+++ /dev/null
@@ -1,23 +0,0 @@
-package engine
-
-import (
-	"context"
-
-	"github.com/google/go-containerregistry/pkg/crane"
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/authn"
-)
-
-type craneEngine struct{}
-
-func NewCraneEngine() *craneEngine {
-	return &craneEngine{}
-}
-
-func (c *craneEngine) ListTags(ctx context.Context, imageURI string) ([]string, error) {
-	options := []crane.Option{
-		crane.WithContext(ctx),
-		crane.WithAuthFromKeychain(authn.PreflightKeychain),
-	}
-
-	return crane.ListTags(imageURI, options...)
-}
diff --git a/certification/internal/policy/container/has_unique_tag.go b/certification/internal/policy/container/has_unique_tag.go
index 9650974..699a55c 100644
--- a/certification/internal/policy/container/has_unique_tag.go
+++ b/certification/internal/policy/container/has_unique_tag.go
@@ -5,22 +5,21 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/google/go-containerregistry/pkg/crane"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
-	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/service"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/internal/authn"
 )
 
-func NewHasUniqueTagCheck(tagLister service.TagLister) *HasUniqueTagCheck {
-	return &HasUniqueTagCheck{TagLister: tagLister}
+func NewHasUniqueTagCheck() *hasUniqueTagCheck {
+	return &hasUniqueTagCheck{}
 }
 
 // HasUniqueTagCheck evaluates the image to ensure that it has a tag other than
 // the latest tag, which is considered to be a "floating" tag and may not accurately
 // represent the same image over time.
-type HasUniqueTagCheck struct {
-	TagLister service.TagLister
-}
+type hasUniqueTagCheck struct{}
 
-func (p *HasUniqueTagCheck) Validate(ctx context.Context, imgRef certification.ImageReference) (bool, error) {
+func (p *hasUniqueTagCheck) Validate(ctx context.Context, imgRef certification.ImageReference) (bool, error) {
 	tags, err := p.getDataToValidate(ctx, fmt.Sprintf("%s/%s", imgRef.ImageRegistry, imgRef.ImageRepository))
 	if err != nil {
 		return false, err
@@ -28,11 +27,16 @@ func (p *HasUniqueTagCheck) Validate(ctx context.Context, imgRef certification.I
 	return p.validate(tags)
 }
 
-func (p *HasUniqueTagCheck) getDataToValidate(ctx context.Context, image string) ([]string, error) {
-	return p.TagLister.ListTags(ctx, image)
+func (p *hasUniqueTagCheck) getDataToValidate(ctx context.Context, image string) ([]string, error) {
+	options := []crane.Option{
+		crane.WithContext(ctx),
+		crane.WithAuthFromKeychain(authn.PreflightKeychain),
+	}
+
+	return crane.ListTags(image, options...)
 }
 
-func (p *HasUniqueTagCheck) validate(tags []string) (bool, error) {
+func (p *hasUniqueTagCheck) validate(tags []string) (bool, error) {
 	// An image passes the check if:
 	// 1) it has more than one tag (`latest` is acceptable)
 	// OR
@@ -40,11 +44,11 @@ func (p *HasUniqueTagCheck) validate(tags []string) (bool, error) {
 	return len(tags) > 1 || len(tags) == 1 && strings.ToLower(tags[0]) != "latest", nil
 }
 
-func (p *HasUniqueTagCheck) Name() string {
+func (p *hasUniqueTagCheck) Name() string {
 	return "HasUniqueTag"
 }
 
-func (p *HasUniqueTagCheck) Metadata() certification.Metadata {
+func (p *hasUniqueTagCheck) Metadata() certification.Metadata {
 	return certification.Metadata{
 		Description:      "Checking if container has a tag other than 'latest', so that the image can be uniquely identified.",
 		Level:            "best",
@@ -53,7 +57,7 @@ func (p *HasUniqueTagCheck) Metadata() certification.Metadata {
 	}
 }
 
-func (p *HasUniqueTagCheck) Help() certification.HelpText {
+func (p *hasUniqueTagCheck) Help() certification.HelpText {
 	return certification.HelpText{
 		Message:    "Check HasUniqueTag encountered an error. Please review the preflight.log file for more information.",
 		Suggestion: "Add a tag to your image. Consider using Semantic Versioning. https://semver.org/",
diff --git a/certification/internal/service/service.go b/certification/internal/service/service.go
deleted file mode 100644
index 3cad6ba..0000000
--- a/certification/internal/service/service.go
+++ /dev/null
@@ -1,7 +0,0 @@
-package service
-
-import "context"
-
-type TagLister interface {
-	ListTags(ctx context.Context, imageURI string) ([]string, error)
-}
diff --git a/go.sum b/go.sum
index ecf8f9e..cedec99 100644
--- a/go.sum
+++ b/go.sum
@@ -1520,6 +1520,7 @@ golang.org/x/mod v0.4.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.4.1/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
 golang.org/x/mod v0.5.0/go.mod h1:5OXOZSfqPIIbmVBIIKWRFfZjPR0E5r58TLhUjH0a2Ro=
+golang.org/x/mod v0.5.1 h1:OJxoQ/rynoF0dcCdI7cLPktw/hR2cueqYfjm43oqK38=
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
@@ -1896,6 +1897,7 @@ golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
 golang.org/x/tools v0.1.6-0.20210820212750-d4cc65f0b2ff/go.mod h1:YD9qOF0M9xpSpdWTBbzEl5e/RnCefISl8E5Noe10jFM=
 golang.org/x/tools v0.1.6/go.mod h1:LGqMHiF4EqQNHR1JncWGqT5BVaXmza+X+BDGol+dOxo=
 golang.org/x/tools v0.1.7/go.mod h1:LGqMHiF4EqQNHR1JncWGqT5BVaXmza+X+BDGol+dOxo=
+golang.org/x/tools v0.1.9 h1:j9KsMiaP1c3B0OTQGth0/k+miLGTgLsAFUCrF2vLcF8=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=


Test patch:
diff --git a/certification/internal/policy/container/has_unique_tag_test.go b/certification/internal/policy/container/has_unique_tag_test.go
index 0de2f60..1a71049 100644
--- a/certification/internal/policy/container/has_unique_tag_test.go
+++ b/certification/internal/policy/container/has_unique_tag_test.go
@@ -2,45 +2,57 @@ package container
 
 import (
 	"context"
+	"fmt"
+	"net/http/httptest"
+	"net/url"
 
+	"github.com/google/go-containerregistry/pkg/crane"
+	"github.com/google/go-containerregistry/pkg/registry"
+	"github.com/google/go-containerregistry/pkg/v1/random"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
 )
 
-// podmanEngine is a package-level variable. In some tests, we
-// override it with a "happy path" engine, that returns good data.
-// In the unhappy path, we override it with an engine that returns
-// nothing but errors.
+var _ = Describe("UniqueTag", func() {
+	var hasUniqueTagCheck hasUniqueTagCheck = *NewHasUniqueTagCheck()
+	var src, dst, host string
 
-type fakeTagLister struct {
-	Tags []string
-}
+	BeforeEach(func() {
+		// Set up a fake registry.
+		s := httptest.NewServer(registry.New())
+		DeferCleanup(func() {
+			s.Close()
+		})
+		u, err := url.Parse(s.URL)
+		Expect(err).ToNot(HaveOccurred())
+		src = fmt.Sprintf("%s/test/preflight", u.Host)
+		dst = fmt.Sprintf("%s/test/tags", u.Host)
+		host = u.Host
 
-func (ftl *fakeTagLister) ListTags(ctx context.Context, imageUri string) ([]string, error) {
-	return ftl.Tags, nil
-}
+		img, err := random.Image(1024, 5)
+		Expect(err).ToNot(HaveOccurred())
 
-var _ = Describe("UniqueTag", func() {
-	var hasUniqueTagCheck HasUniqueTagCheck
+		err = crane.Push(img, src)
+		Expect(err).ToNot(HaveOccurred())
 
+		err = crane.Copy(src, dst)
+		Expect(err).ToNot(HaveOccurred())
+
+		err = crane.Tag(dst, "unique-tag")
+		Expect(err).ToNot(HaveOccurred())
+	})
 	Describe("Checking for unique tags", func() {
 		Context("When it has tags other than latest", func() {
-			BeforeEach(func() {
-				hasUniqueTagCheck = *NewHasUniqueTagCheck(&fakeTagLister{Tags: validImageTags()})
-			})
 			It("should pass Validate", func() {
-				ok, err := hasUniqueTagCheck.Validate(context.TODO(), certification.ImageReference{ImageRegistry: "index.docker.io", ImageRepository: "dummy/image"})
+				ok, err := hasUniqueTagCheck.Validate(context.TODO(), certification.ImageReference{ImageRegistry: host, ImageRepository: "test/tags"})
 				Expect(err).ToNot(HaveOccurred())
 				Expect(ok).To(BeTrue())
 			})
 		})
 		Context("When it has only latest tag", func() {
-			BeforeEach(func() {
-				hasUniqueTagCheck = *NewHasUniqueTagCheck(&fakeTagLister{Tags: invalidImageTags()})
-			})
 			It("should not pass Validate", func() {
-				ok, err := hasUniqueTagCheck.Validate(context.TODO(), certification.ImageReference{ImageRegistry: "index.docker.io", ImageRepository: "dummy/other-image"})
+				ok, err := hasUniqueTagCheck.Validate(context.TODO(), certification.ImageReference{ImageRegistry: host, ImageRepository: "test/preflight"})
 				Expect(err).ToNot(HaveOccurred())
 				Expect(ok).To(BeFalse())
 			})