Merge pull request #130 from njhale/fix-ignored-cachedir

Bug 1775527: Fix ignored cachedir in appregistry image builder

Author: openshift-merge-robot

Makefile

@@ -43,7 +43,6 @@ container-codegen:
 	docker rm temp-codegen
 
 generate-fakes:
-	go install -mod=vendor ./vendor/github.com/maxbrunsfeld/counterfeiter/v6
 	go generate ./...
 
 clean:

go.sum

@@ -243,6 +243,7 @@ github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANyt
 github.com/jackc/fake v0.0.0-20150926172116-812a484cc733/go.mod h1:WrMFNQdiFJ80sQsxDoMokWK1W5TQtxBFNpzWTD84ibQ=
 github.com/jackc/pgx v3.2.0+incompatible/go.mod h1:0ZGrqGqkRlliWnWB4zKnWtjbSWbGkVEFm4TeybAXq+I=
 github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
+github.com/joefitzgerald/rainbow-reporter v0.1.0 h1:AuMG652zjdzI0YCCnXAqATtRBpGXMcAnrajcaTrSeuo=
 github.com/joefitzgerald/rainbow-reporter v0.1.0/go.mod h1:481CNgqmVHQZzdIbN52CupLJyoVwB10FQ/IQlF1pdL8=
 github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo=
 github.com/json-iterator/go v0.0.0-20180612202835-f2b4162afba3/go.mod h1:+SdeFBvtyEkXs7REEP0seUULqWtbJapLOCVDaaPEHmU=
@@ -351,6 +352,7 @@ github.com/rcrowley/go-metrics v0.0.0-20181016184325-3113b8401b8a/go.mod h1:bCqn
 github.com/remyoudompheng/bigfft v0.0.0-20170806203942-52369c62f446/go.mod h1:uYEyJGbgTkfkS4+E/PavXkNJcbFIpEtjt2B0KDQ5+9M=
 github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR/rfWxYHBV53g=
 github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0=
+github.com/sclevine/spec v1.2.0 h1:1Jwdf9jSfDl9NVmt8ndHqbTZ7XCCPbh1jI3hkDBHVYA=
 github.com/sclevine/spec v1.2.0/go.mod h1:W4J29eT/Kzv7/b9IWLB055Z+qvVC9vt0Arko24q7p+U=
 github.com/shopspring/decimal v0.0.0-20180709203117-cd690d0c9e24/go.mod h1:M+9NzErvs504Cn4c5DxATwIqPbtswREoFCre64PpcG4=
 github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=

pkg/apprclient/apprclientfakes/fake_client.go

@@ -1,3 +1,4 @@
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Client
 package apprclient
 
 import (

pkg/apprclient/client.go

@@ -0,0 +1,13 @@
+package appregistry
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestAppregistry(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Appregistry Suite")
+}

pkg/appregistry/appregistry_suite_test.go

@@ -1,3 +1,4 @@
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 builder.go ImageAppender
 package appregistry
 
 import (
@@ -46,6 +47,7 @@ type AppregistryImageBuilder struct {
 	CleanOutput bool
 	ManifestDir string
 	DatabaseDir string
+	client      apprclient.Client
 }
 
 func NewAppregistryImageBuilder(options ...AppregistryBuildOption) (*AppregistryImageBuilder, error) {
@@ -69,14 +71,11 @@ func NewAppregistryImageBuilder(options ...AppregistryBuildOption) (*Appregistry
 		CleanOutput:         config.CleanOutput,
 		ManifestDir:         config.ManifestDir,
 		DatabaseDir:         config.DatabaseDir,
+		client:              config.Client,
 	}, nil
 }
 
 func (b *AppregistryImageBuilder) Build() error {
-	opts := apprclient.Options{Source: b.AppRegistryEndpoint}
-	if b.AuthToken != "" {
-		opts.AuthToken = b.AuthToken
-	}
 
 	defer func() {
 		if !b.CleanOutput {
@@ -87,12 +86,7 @@ func (b *AppregistryImageBuilder) Build() error {
 		}
 	}()
 
-	client, err := apprclient.New(opts)
-	if err != nil {
-		return err
-	}
-
-	downloader := NewManifestDownloader(client)
+	downloader := NewManifestDownloader(b.client)
 	if err := downloader.DownloadManifests(b.ManifestDir, b.AppRegistryOrg); err != nil {
 		return err
 	}

pkg/appregistry/appregistryfakes/fake_image_appender.go

@@ -3,7 +3,10 @@ package appregistry
 import (
 	"fmt"
 	"io/ioutil"
+	"os"
 	"path"
+
+	"github.com/operator-framework/operator-registry/pkg/apprclient"
 )
 
 type AppregistryBuildOptions struct {
@@ -20,6 +23,8 @@ type AppregistryBuildOptions struct {
 	CleanOutput bool
 	ManifestDir string
 	DatabaseDir string
+
+	Client apprclient.Client
 }
 
 func (o *AppregistryBuildOptions) Validate() error {
@@ -46,18 +51,31 @@ func (o *AppregistryBuildOptions) Validate() error {
 	if o.DatabaseDir == "" {
 		return fmt.Errorf("local database directory required")
 	}
+	if o.Client == nil {
+		return fmt.Errorf("app-registry client must not be nil")
+	}
 
 	return nil
 }
 
 func (o *AppregistryBuildOptions) Complete() error {
-	// if a user has specified a specific cache dir, don't clean it after run
-	o.CleanOutput = o.CacheDir == ""
+	// if a user hasn't specified a specific cache directory, generate a temporary one
+	if o.CacheDir == "" {
+		tmp, err := ioutil.TempDir("", "cache-")
+		if err != nil {
+			return err
+		}
+		o.CacheDir = tmp
 
-	// build a separate path for manifests and the built database, so that
-	// building is idempotent
+		// clean up temporary directories
+		o.CleanOutput = true
+	} else if err := os.MkdirAll(o.CacheDir, os.ModePerm); err != nil && !os.IsExist(err) {
+		return err
+	}
+
+	// build a separate path for manifests and the built database, so that building is idempotent
 	if o.ManifestDir == "" {
-		manifestDir, err := ioutil.TempDir("", "manifests-")
+		manifestDir, err := ioutil.TempDir(o.CacheDir, "manifests-")
 		if err != nil {
 			return err
 		}
@@ -75,6 +93,22 @@ func (o *AppregistryBuildOptions) Complete() error {
 	if o.DatabasePath == "" {
 		o.DatabasePath = path.Join(o.DatabaseDir, "bundles.db")
 	}
+
+	// create the client
+	if o.Client == nil {
+		opts := apprclient.Options{Source: o.AppRegistryEndpoint}
+		if o.AuthToken != "" {
+			opts.AuthToken = o.AuthToken
+		}
+
+		client, err := apprclient.New(opts)
+		if err != nil {
+			return err
+		}
+
+		o.Client = client
+	}
+
 	return nil
 }
 
@@ -88,7 +122,7 @@ func (c *AppregistryBuildOptions) Apply(options []AppregistryBuildOption) {
 // ToOption converts an AppregistryBuildOptions object into a function that applies
 // its current configuration to another AppregistryBuildOptions instance
 func (c *AppregistryBuildOptions) ToOption() AppregistryBuildOption {
-	return  func(o *AppregistryBuildOptions) {
+	return func(o *AppregistryBuildOptions) {
 		if c.Appender != nil {
 			o.Appender = c.Appender
 		}
@@ -116,6 +150,9 @@ func (c *AppregistryBuildOptions) ToOption() AppregistryBuildOption {
 		if c.DatabaseDir != "" {
 			o.DatabaseDir = c.DatabaseDir
 		}
+		if c.Client != nil {
+			o.Client = c.Client
+		}
 	}
 }
 
@@ -174,3 +211,9 @@ func WithCacheDir(s string) AppregistryBuildOption {
 		o.CacheDir = s
 	}
 }
+
+func WithClient(c apprclient.Client) AppregistryBuildOption {
+	return func(o *AppregistryBuildOptions) {
+		o.Client = c
+	}
+}

pkg/appregistry/appregistryfakes/fake_manifest_downloader.go

@@ -0,0 +1,82 @@
+package appregistry
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/operator-framework/operator-registry/pkg/apprclient"
+	"github.com/operator-framework/operator-registry/pkg/apprclient/apprclientfakes"
+)
+
+var _ = Describe("Image building", func() {
+	var (
+		options  []AppregistryBuildOption
+		builder  *AppregistryImageBuilder
+		pkg      *apprclient.RegistryMetadata
+		operator *apprclient.OperatorMetadata
+	)
+
+	JustBeforeEach(func() {
+		var err error
+		builder, err = NewAppregistryImageBuilder(options...)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(builder).ToNot(BeNil())
+		Expect(builder.Build()).To(Succeed())
+	})
+
+	BeforeEach(func() {
+		var noopAppender ImageAppendFunc = func(from, to, layer string) error {
+			return nil
+		}
+
+		client := &apprclientfakes.FakeClient{}
+		pkg = &apprclient.RegistryMetadata{
+			Namespace: "marsupials",
+			Name:      "koala",
+		}
+		client.ListPackagesReturns([]*apprclient.RegistryMetadata{pkg}, nil)
+
+		pkgBlob, err := ioutil.ReadFile("testdata/golden/marsupials_pkg.tar")
+		Expect(err).ToNot(HaveOccurred())
+
+		operator = &apprclient.OperatorMetadata{
+			RegistryMetadata: *pkg,
+			Blob:             pkgBlob,
+		}
+		client.RetrieveOneReturns(operator, nil)
+
+		options = append(options,
+			WithAppender(noopAppender),
+			WithAppRegistryOrg("metatheria"),
+			WithClient(client),
+		)
+	})
+
+	Context("with a custom cache dir", func() {
+		var (
+			cacheDir      string
+			manifestsGlob string
+		)
+
+		BeforeEach(func() {
+			cacheDir = filepath.Join("testdata", "custom-cache")
+			options = append(options, WithCacheDir(cacheDir))
+			manifestsGlob = filepath.Join(cacheDir, "manifests-*/koala/marsupials")
+		})
+
+		AfterEach(func() {
+			Expect(os.RemoveAll(cacheDir)).To(Succeed())
+		})
+
+		It("should retain unpacked operator manifests", func() {
+			Expect(cacheDir).To(BeADirectory())
+			Expect(filepath.Glob(filepath.Join(manifestsGlob, "package.yaml"))).To(HaveLen(1))
+			Expect(filepath.Glob(filepath.Join(manifestsGlob, "v1.0.0", "koala.v1.0.0.clusterserviceversion.yaml"))).To(HaveLen(1))
+		})
+
+	})
+})

pkg/appregistry/builder.go

@@ -1,3 +1,4 @@
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . ManifestDownloader
 package appregistry
 
 import (

pkg/appregistry/builder_options.go

@@ -1,4 +1,4 @@
-//go:generate counterfeiter command.go CommandRunner
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . CommandRunner
 package containertools
 
 import (
@@ -25,8 +25,8 @@ type CommandRunner interface {
 
 // ContainerCommandRunner is configured to select a container cli tool and execute commands with that
 // tooling.
-type ContainerCommandRunner struct{
-	logger *logrus.Entry
+type ContainerCommandRunner struct {
+	logger        *logrus.Entry
 	containerTool string
 }
 
@@ -76,7 +76,7 @@ func (r *ContainerCommandRunner) Pull(image string) error {
 // Build takes a dockerfile and a tag and builds a container image
 func (r *ContainerCommandRunner) Build(dockerfile, tag string) error {
 	args := []string{"build", "-f", dockerfile}
-	
+
 	if tag != "" {
 		args = append(args, "-t", tag)
 	}

pkg/appregistry/builder_test.go

@@ -1,4 +1,4 @@
-//go:generate counterfeiter dockerfilegenerator.go DockerfileGenerator
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . DockerfileGenerator
 package containertools
 
 import (
@@ -8,10 +8,10 @@ import (
 )
 
 const (
-	baseImage = "scratch"
+	baseImage                = "scratch"
 	defaultBinarySourceImage = "quay.io/operator-framework/upstream-registry-builder"
-	DefaultDbLocation = "./index.db"
-	DbLocationLabel = "operators.operatorframework.io.index.database.v1"
+	DefaultDbLocation        = "./index.db"
+	DbLocationLabel          = "operators.operatorframework.io.index.database.v1"
 )
 
 // DockerfileGenerator defines functions to generate index dockerfiles

pkg/appregistry/manifest_downloader.go

@@ -1,4 +1,4 @@
-//go:generate counterfeiter imagereader.go ImageReader
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . ImageReader
 package containertools
 
 import (

pkg/appregistry/testdata/golden/marsupials_pkg.tar

@@ -1,9 +1,9 @@
-//go:generate counterfeiter labelreader.go LabelReader
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . LabelReader
 package containertools
 
 import (
-	"fmt"
 	"encoding/json"
+	"fmt"
 
 	"github.com/sirupsen/logrus"
 )
@@ -14,15 +14,15 @@ type LabelReader interface {
 
 type ImageLabelReader struct {
 	Logger *logrus.Entry
-	Cmd CommandRunner
+	Cmd    CommandRunner
 }
 
 func NewLabelReader(containerTool string, logger *logrus.Entry) LabelReader {
 	cmd := NewCommandRunner(containerTool, logger)
-	
+
 	return ImageLabelReader{
 		Logger: logger,
-		Cmd: cmd,
+		Cmd:    cmd,
 	}
 }
 
@@ -39,7 +39,7 @@ type PodmanImageData struct {
 }
 
 // GetLabelsFromImage takes a container image path as input, pulls that image
-// to the local environment and then inspects it for labels  
+// to the local environment and then inspects it for labels
 func (r ImageLabelReader) GetLabelsFromImage(image string) (map[string]string, error) {
 	err := r.Cmd.Pull(image)
 	if err != nil {

pkg/containertools/command.go

@@ -1,5 +1,4 @@
-//go:generate counterfeiter indexer.go IndexAdder
-//go:generate counterfeiter indexer.go IndexDeleter
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate
 package indexer
 
 import (
@@ -11,6 +10,7 @@ import (
 
 // IndexAdder allows the creation of index container images from scratch or
 // based on previous index images
+//counterfeiter:generate . IndexAdder
 type IndexAdder interface {
 	AddToIndex(AddToIndexRequest) error
 }
@@ -19,17 +19,18 @@ type IndexAdder interface {
 func NewIndexAdder(containerTool string, logger *logrus.Entry) IndexAdder {
 	return ImageIndexer{
 		DockerfileGenerator: containertools.NewDockerfileGenerator(containerTool, logger),
-		CommandRunner: containertools.NewCommandRunner(containerTool, logger),
-		LabelReader: containertools.NewLabelReader(containerTool, logger),
-		RegistryAdder: registry.NewRegistryAdder(logger),
-		ImageReader: containertools.NewImageReader(containerTool, logger),
-		ContainerTool: containerTool,
-		Logger: logger,
+		CommandRunner:       containertools.NewCommandRunner(containerTool, logger),
+		LabelReader:         containertools.NewLabelReader(containerTool, logger),
+		RegistryAdder:       registry.NewRegistryAdder(logger),
+		ImageReader:         containertools.NewImageReader(containerTool, logger),
+		ContainerTool:       containerTool,
+		Logger:              logger,
 	}
 }
 
 // IndexDeleter takes indexes and deletes all references to an operator
 // from them
+//counterfeiter:generate . IndexDeleter
 type IndexDeleter interface {
 	DeleteFromIndex(DeleteFromIndexRequest) error
 }
@@ -38,11 +39,11 @@ type IndexDeleter interface {
 func NewIndexDeleter(containerTool string, logger *logrus.Entry) IndexDeleter {
 	return ImageIndexer{
 		DockerfileGenerator: containertools.NewDockerfileGenerator(containerTool, logger),
-		CommandRunner: containertools.NewCommandRunner(containerTool, logger),
-		LabelReader: containertools.NewLabelReader(containerTool, logger),
-		RegistryDeleter: registry.NewRegistryDeleter(logger),
-		ImageReader: containertools.NewImageReader(containerTool, logger),
-		ContainerTool: containerTool,
-		Logger: logger,
+		CommandRunner:       containertools.NewCommandRunner(containerTool, logger),
+		LabelReader:         containertools.NewLabelReader(containerTool, logger),
+		RegistryDeleter:     registry.NewRegistryDeleter(logger),
+		ImageReader:         containertools.NewImageReader(containerTool, logger),
+		ContainerTool:       containerTool,
+		Logger:              logger,
 	}
 }

pkg/containertools/dockerfilegenerator.go

@@ -1,11 +1,11 @@
-//go:generate counterfeiter registry.go RegistryAdder
-//go:generate counterfeiter registry.go RegistryDeleter
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate
 package registry
 
 import (
 	"github.com/sirupsen/logrus"
 )
 
+//counterfeiter:generate . RegistryAdder
 type RegistryAdder interface {
 	AddToRegistry(AddToRegistryRequest) error
 }
@@ -16,6 +16,7 @@ func NewRegistryAdder(logger *logrus.Entry) RegistryAdder {
 	}
 }
 
+//counterfeiter:generate . RegistryDeleter
 type RegistryDeleter interface {
 	DeleteFromRegistry(DeleteFromRegistryRequest) error
 }