From 44f55dc59a4abbb5540315eb5908786da808d1be Mon Sep 17 00:00:00 2001 From: Nitish Gupta Date: Wed, 6 Jul 2022 13:52:45 +0530 Subject: [PATCH] flags: Move cache flag constants and tests to enums - 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 Signed-off-by: Nitish Gupta --- acceptance/acceptance_test.go | 1 + acceptance/invoke/pack.go | 12 ++++- internal/build/lifecycle_execution.go | 2 +- internal/build/lifecycle_execution_test.go | 8 +-- internal/cache/cache_opts.go | 61 +++++++++++++++++----- internal/commands/build.go | 8 +-- internal/commands/build_test.go | 2 +- 7 files changed, 70 insertions(+), 24 deletions(-) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 3e1dbe329b..b8f865eb2f 100644 --- a/acceptance/acceptance_test.go +++ b/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) }) diff --git a/acceptance/invoke/pack.go b/acceptance/invoke/pack.go index c33cafc6ef..333dbb0543 100644 --- a/acceptance/invoke/pack.go +++ b/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 { diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go index eee61c0018..c4bd6ba442 100644 --- a/internal/build/lifecycle_execution.go +++ b/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 diff --git a/internal/build/lifecycle_execution_test.go b/internal/build/lifecycle_execution_test.go index 1fea8d31f4..f1038f65f6 100644 --- a/internal/build/lifecycle_execution_test.go +++ b/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: %%")) }) }) }) diff --git a/internal/cache/cache_opts.go b/internal/cache/cache_opts.go index 03471c8550..81c40782b2 100644 --- a/internal/cache/cache_opts.go +++ b/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 = ';' @@ -30,15 +61,21 @@ func (c *CacheOpts) Set(value string) error { 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 } @@ -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") diff --git a/internal/commands/build.go b/internal/commands/build.go index 856ccaa93e..571798718a 100644 --- a/internal/commands/build.go +++ b/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") } diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index b4b059e93d..6250fd53be 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -369,7 +369,7 @@ 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") }) }) })