flags: Move cache flag constants to enums and update tests

- Gate acceptance tests to work only when pack supports `--cache`
- string constants for cache type and cache format has been moved to enums
- Fix test cases, update acceptance test helper methods
- Add new unit tests

Signed-off-by: Nitish Gupta <imnitish.ng@gmail.com>

acceptance/acceptance_test.go

@@ -1759,6 +1759,7 @@ func testAcceptance(
 					when("--cache with options for build cache as image", func() {
 						var cacheImageName, cacheFlags string
 						it.Before(func() {
+							h.SkipIf(t, !pack.SupportsFeature(invoke.Cache), "")
 							cacheImageName = fmt.Sprintf("%s-cache", repoName)
 							cacheFlags = fmt.Sprintf("type=build;format=image;name=%s", cacheImageName)
 						})

acceptance/invoke/pack.go

@@ -5,6 +5,7 @@ package invoke
 
 import (
 	"bytes"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -210,20 +211,27 @@ func (i *PackInvoker) Supports(command string) bool {
 		search = command
 	}
 
+	re := regexp.MustCompile(fmt.Sprint(`\b%s\b`, search))
 	output, err := i.baseCmd(cmdParts...).CombinedOutput()
 	i.assert.Nil(err)
 
-	return strings.Contains(string(output), search) && !strings.Contains(string(output), "Unknown help topic")
+	return re.MatchString(string(output)) && !strings.Contains(string(output), "Unknown help topic")
 }
 
 type Feature int
 
-const CreationTime = iota
+const (
+	CreationTime = iota
+	Cache
+)
 
 var featureTests = map[Feature]func(i *PackInvoker) bool{
 	CreationTime: func(i *PackInvoker) bool {
 		return i.Supports("build --creation-time")
 	},
+	Cache: func(i *PackInvoker) bool {
+		return i.Supports("build --cache")
+	},
 }
 
 func (i *PackInvoker) SupportsFeature(f Feature) bool {

internal/build/lifecycle_execution.go

@@ -124,7 +124,7 @@ func (l *LifecycleExecution) PrevImageName() string {
 func (l *LifecycleExecution) Run(ctx context.Context, phaseFactoryCreator PhaseFactoryCreator) error {
 	phaseFactory := phaseFactoryCreator(l)
 	var buildCache Cache
-	if l.opts.CacheImage != "" || (l.opts.Cache.CacheType == "build" && l.opts.Cache.Format == "image") {
+	if l.opts.CacheImage != "" || (l.opts.Cache.CacheType == cache.Build && l.opts.Cache.Format == cache.CacheImage) {
 		cacheImageName := l.opts.CacheImage
 		if cacheImageName == "" {
 			cacheImageName = l.opts.Cache.Source

internal/build/lifecycle_execution_test.go

@@ -352,7 +352,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 						return fakePhaseFactory
 					})
 
-					h.AssertError(t, err, fmt.Sprintf("invalid cache image name: %s", "could not parse reference: %%!(NOVERB)"))
+					h.AssertError(t, err, fmt.Sprintf("invalid cache image name: %s", "could not parse reference: %%"))
 				})
 
 				it("fails for cache flags", func() {
@@ -365,8 +365,8 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 						TrustBuilder: false,
 						UseCreator:   false,
 						Cache: cache.CacheOpts{
-							CacheType: "build",
-							Format:    "image",
+							CacheType: cache.Build,
+							Format:    cache.CacheImage,
 							Source:    "%%%",
 						},
 						Termui: fakeTermui,
@@ -379,7 +379,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 						return fakePhaseFactory
 					})
 
-					h.AssertError(t, err, fmt.Sprintf("invalid cache image name: %s", "could not parse reference: %%!(NOVERB)"))
+					h.AssertError(t, err, fmt.Sprintf("invalid cache image name: %s", "could not parse reference: %%"))
 				})
 			})
 		})

internal/cache/cache_opts.go

@@ -8,12 +8,43 @@ import (
 	"github.com/pkg/errors"
 )
 
+type Cache int
+type Format int
 type CacheOpts struct {
-	CacheType string
-	Format    string
+	CacheType Cache
+	Format    Format
 	Source    string
 }
 
+const (
+	Build Cache = iota
+	Launch
+)
+const (
+	CacheVolume Format = iota
+	CacheImage
+)
+
+func (c Cache) String() string {
+	switch c {
+	case Build:
+		return "build"
+	case Launch:
+		return "launch"
+	}
+	return ""
+}
+
+func (f Format) String() string {
+	switch f {
+	case CacheImage:
+		return "image"
+	case CacheVolume:
+		return "volume"
+	}
+	return ""
+}
+
 func (c *CacheOpts) Set(value string) error {
 	csvReader := csv.NewReader(strings.NewReader(value))
 	csvReader.Comma = ';'
@@ -25,28 +56,34 @@ func (c *CacheOpts) Set(value string) error {
 	for _, field := range fields {
 		parts := strings.SplitN(field, "=", 2)
 
+		if len(parts) != 2 {
+			return errors.Errorf("invalid field '%s' must be a key=value pair", field)
+		}
+
 		if len(parts) == 2 {
 			key := strings.ToLower(parts[0])
 			value := strings.ToLower(parts[1])
 			switch key {
 			case "type":
-				if value != "build" && value != "launch" {
+				switch value {
+				case "build":
+					c.CacheType = Build
+				case "launch":
+					c.CacheType = Launch
+				default:
 					return errors.Errorf("invalid cache type '%s'", value)
 				}
-				c.CacheType = value
 			case "format":
-				if value != "image" {
+				switch value {
+				case "image":
+					c.Format = CacheImage
+				default:
 					return errors.Errorf("invalid cache format '%s'", value)
 				}
-				c.Format = value
 			case "name":
 				c.Source = value
 			}
 		}
-
-		if len(parts) != 2 {
-			return errors.Errorf("invalid field '%s' must be a key=value pair", field)
-		}
 	}
 
 	err = populateMissing(c)
@@ -58,10 +95,10 @@ func (c *CacheOpts) Set(value string) error {
 
 func (c *CacheOpts) String() string {
 	var cacheFlag string
-	if c.CacheType != "" {
+	if c.CacheType.String() != "" {
 		cacheFlag += fmt.Sprintf("type=%s;", c.CacheType)
 	}
-	if c.Format != "" {
+	if c.Format.String() != "" {
 		cacheFlag += fmt.Sprintf("format=%s;", c.Format)
 	}
 	if c.Source != "" {
@@ -75,11 +112,11 @@ func (c *CacheOpts) Type() string {
 }
 
 func populateMissing(c *CacheOpts) error {
-	if c.CacheType == "" {
-		c.CacheType = "build"
+	if c.CacheType.String() == "" {
+		c.CacheType = Build
 	}
-	if c.Format == "" {
-		c.Format = "volume"
+	if c.Format.String() == "" {
+		c.Format = CacheVolume
 	}
 	if c.Source == "" {
 		return errors.Errorf("cache 'name' is required")

internal/cache/cache_opts_test.go

@@ -105,5 +105,41 @@ func testCacheOpts(t *testing.T, when spec.G, it spec.S) {
 				}
 			}
 		})
+
+		it("image cache format with invalid options", func() {
+			testcases := []CacheOptTestCase{
+				{
+					name:       "Invalid cache type",
+					input:      "type=invalid_cache;format=image;name=io.test.io/myorg/my-cache:build",
+					output:     "invalid cache type 'invalid_cache'",
+					shouldFail: true,
+				},
+				{
+					name:       "Invalid cache format",
+					input:      "type=launch;format=invalid_format;name=io.test.io/myorg/my-cache:build",
+					output:     "invalid cache format 'invalid_format'",
+					shouldFail: true,
+				},
+				{
+					name:       "Not a key=value pair",
+					input:      "launch;format=image;name=io.test.io/myorg/my-cache:build",
+					output:     "invalid field 'launch' must be a key=value pair",
+					shouldFail: true,
+				},
+				{
+					name:       "Extra semicolon",
+					input:      "type=launch;format=image;name=io.test.io/myorg/my-cache:build;",
+					output:     "invalid field '' must be a key=value pair",
+					shouldFail: true,
+				},
+			}
+
+			for _, testcase := range testcases {
+				var cacheFlags CacheOpts
+				t.Logf("Testing cache type: %s", testcase.name)
+				err := cacheFlags.Set(testcase.input)
+				h.AssertError(t, err, testcase.output)
+			}
+		})
 	})
 }

internal/commands/build.go

@@ -241,15 +241,15 @@ func validateBuildFlags(flags *BuildFlags, cfg config.Config, packClient PackCli
 		return client.NewExperimentError("Support for buildpack registries is currently experimental.")
 	}
 
-	if flags.Cache.CacheType == "launch" && flags.Cache.Format == "image" {
+	if flags.Cache.CacheType == cache.Launch && flags.Cache.Format == cache.CacheImage {
 		logger.Warn("cache definition: 'launch' cache in format 'image' is not supported.")
 	}
 
-	if flags.Cache.String() != "" && flags.CacheImage != "" {
-		return errors.New("'cache' flag cannot be used with 'cache-image' flag.")
+	if flags.Cache.Format == cache.CacheImage && flags.CacheImage != "" {
+		return errors.New("'cache' flag with 'image' format cannot be used with 'cache-image' flag.")
 	}
 
-	if flags.Cache.Format == "image" && !flags.Publish {
+	if flags.Cache.Format == cache.CacheImage && !flags.Publish {
 		return errors.New("image cache format requires the 'publish' flag")
 	}
 

internal/commands/build_test.go

@@ -369,7 +369,18 @@ func testBuildCommand(t *testing.T, when spec.G, it spec.S) {
 				it("errors", func() {
 					command.SetArgs([]string{"--builder", "my-builder", "image", "--cache-image", "some-cache-image", "--cache", "type=build;format=image;name=myorg/myimage:cache"})
 					err := command.Execute()
-					h.AssertError(t, err, "'cache' flag cannot be used with 'cache-image' flag.")
+					h.AssertError(t, err, "'cache' flag with 'image' format cannot be used with 'cache-image' flag")
+				})
+			})
+			when("''type=launch;format=image' is used", func() {
+				it("warns", func() {
+					mockClient.EXPECT().
+						Build(gomock.Any(), EqBuildOptionsWithCacheFlags("type=launch;format=image;name=myorg/myimage:cache")).
+						Return(nil)
+
+					command.SetArgs([]string{"--builder", "my-builder", "image", "--cache", "type=launch;format=image;name=myorg/myimage:cache", "--publish"})
+					h.AssertNil(t, command.Execute())
+					h.AssertContains(t, outBuf.String(), "Warning: cache definition: 'launch' cache in format 'image' is not supported.")
 				})
 			})
 		})