From 0a5cf9ce9176fafc547dcd7bad0021817b201db7 Mon Sep 17 00:00:00 2001 From: Hiroshi Hayakawa Date: Mon, 2 Sep 2024 12:44:44 +0900 Subject: [PATCH 1/7] Make the 'pack build' command warn that the positional argument will not be treated as the source directory path. Signed-off-by: Hiroshi Hayakawa --- internal/commands/build.go | 8 ++++++-- internal/commands/build_test.go | 7 +++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/internal/commands/build.go b/internal/commands/build.go index c80123d77..ef2400876 100644 --- a/internal/commands/build.go +++ b/internal/commands/build.go @@ -8,14 +8,13 @@ import ( "strings" "time" - "github.com/buildpacks/pack/pkg/cache" - "github.com/google/go-containerregistry/pkg/name" "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/buildpacks/pack/internal/config" "github.com/buildpacks/pack/internal/style" + "github.com/buildpacks/pack/pkg/cache" "github.com/buildpacks/pack/pkg/client" "github.com/buildpacks/pack/pkg/image" "github.com/buildpacks/pack/pkg/logging" @@ -328,6 +327,11 @@ func validateBuildFlags(flags *BuildFlags, cfg config.Config, inputImageRef clie return client.NewExperimentError("Exporting to OCI layout is currently experimental.") } + if !inputImageRef.Layout() && flags.AppPath == "" { + logger.Warnf("You are building an image named '%s'. If you mean it as an app directory path, run 'pack build --path %s'", + inputImageRef.Name(), inputImageRef.Name()) + } + return nil } diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 4e4d03c69..06cf50a06 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -17,11 +17,10 @@ import ( "github.com/sclevine/spec/report" "github.com/spf13/cobra" - "github.com/buildpacks/pack/internal/paths" - "github.com/buildpacks/pack/internal/commands" "github.com/buildpacks/pack/internal/commands/testmocks" "github.com/buildpacks/pack/internal/config" + "github.com/buildpacks/pack/internal/paths" "github.com/buildpacks/pack/pkg/client" "github.com/buildpacks/pack/pkg/image" "github.com/buildpacks/pack/pkg/logging" @@ -77,6 +76,7 @@ func testBuildCommand(t *testing.T, when spec.G, it spec.S) { command.SetArgs([]string{"--builder", "my-builder", "image"}) h.AssertNil(t, command.Execute()) + h.AssertContains(t, outBuf.String(), "Warning: You are building an image named 'image'. If you mean it as an app directory path, run 'pack build --path image'") }) it("builds an image with a builder short command arg", func() { @@ -968,6 +968,7 @@ builder = "my-builder" command.SetArgs([]string{"oci:image", "--builder", "my-builder"}) err := command.Execute() h.AssertNil(t, err) + h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) }) }) @@ -982,6 +983,7 @@ builder = "my-builder" command.SetArgs([]string{"oci:image", "--previous-image", "oci:my-previous-image", "--builder", "my-builder"}) err := command.Execute() h.AssertNil(t, err) + h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) }) }) @@ -995,6 +997,7 @@ builder = "my-builder" command.SetArgs([]string{"oci:image", "--sparse", "--builder", "my-builder"}) err := command.Execute() h.AssertNil(t, err) + h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) }) }) }) From 7066e525f3cad9bbde4a3f837505b245e548e986 Mon Sep 17 00:00:00 2001 From: Hiroshi Hayakawa Date: Thu, 5 Sep 2024 20:08:52 +0900 Subject: [PATCH 2/7] Add a test case for when the 'pack build' is called with the '--path' flag. Signed-off-by: Hiroshi Hayakawa --- internal/commands/build_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 06cf50a06..ca0e3fb63 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -931,6 +931,19 @@ builder = "my-builder" }) }) + when("path to app dir or zip-formatted file is provided", func() { + it("builds with the specified path"+ + "and doesn't warn that the positional argument will not be treated as the source path", func() { + mockClient.EXPECT(). + Build(gomock.Any(), EqBuildOptionsWithPath("my-source")). + Return(nil) + + command.SetArgs([]string{"image", "--builder", "my-builder", "--path", "my-source"}) + h.AssertNil(t, command.Execute()) + h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) + }) + }) + when("export to OCI layout is expected but experimental isn't set in the config", func() { it("errors with a descriptive message", func() { command.SetArgs([]string{"oci:image", "--builder", "my-builder"}) @@ -1178,6 +1191,15 @@ func EqBuildOptionsWithDateTime(t *time.Time) interface{} { } } +func EqBuildOptionsWithPath(path string) interface{} { + return buildOptionsMatcher{ + description: fmt.Sprintf("AppPath=%s", path), + equals: func(o client.BuildOptions) bool { + return o.AppPath == path + }, + } +} + func EqBuildOptionsWithLayoutConfig(image, previousImage string, sparse bool, layoutDir string) interface{} { return buildOptionsMatcher{ description: fmt.Sprintf("image=%s, previous-image=%s, sparse=%t, layout-dir=%s", image, previousImage, sparse, layoutDir), From 66982d038924de48c4328787fa085c16711601b2 Mon Sep 17 00:00:00 2001 From: Hiroshi Hayakawa Date: Thu, 5 Sep 2024 20:09:54 +0900 Subject: [PATCH 3/7] Improve descriptions for some test cases. Signed-off-by: Hiroshi Hayakawa --- internal/commands/build_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index ca0e3fb63..920969881 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -69,7 +69,8 @@ func testBuildCommand(t *testing.T, when spec.G, it spec.S) { }) when("a builder and image are set", func() { - it("builds an image with a builder", func() { + it("builds an image with a builder"+ + "and warns that the positional argument will not be treated as the source path", func() { mockClient.EXPECT(). Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", "image")). Return(nil) @@ -972,7 +973,8 @@ builder = "my-builder" }) when("path to save the image is provided", func() { - it("build is called with oci layout configuration", func() { + it("builds with oci layout configuration"+ + "and it doesn't warn that the positional argument will not be treated as the source path", func() { sparse = false mockClient.EXPECT(). Build(gomock.Any(), EqBuildOptionsWithLayoutConfig("image", previousImage, sparse, layoutDir)). @@ -986,7 +988,8 @@ builder = "my-builder" }) when("previous-image flag is provided", func() { - it("build is called with oci layout configuration", func() { + it("builds with oci layout configuration"+ + "and it doesn't warn that the positional argument will not be treated as the source path", func() { sparse = false previousImage = "my-previous-image" mockClient.EXPECT(). @@ -1001,7 +1004,8 @@ builder = "my-builder" }) when("-sparse flag is provided", func() { - it("build is called with oci layout configuration and sparse true", func() { + it("build with oci layout configuration and sparse true"+ + "and it doesn't warn that the positional argument will not be treated as the source path", func() { sparse = true mockClient.EXPECT(). Build(gomock.Any(), EqBuildOptionsWithLayoutConfig("image", previousImage, sparse, layoutDir)). From cee65f255cb18c15ffeee45634537e59e4d56dc4 Mon Sep 17 00:00:00 2001 From: Hiroshi Hayakawa Date: Tue, 17 Sep 2024 12:47:27 +0900 Subject: [PATCH 4/7] Make it warns only when a local path with the same string as the specified image name exists Signed-off-by: Hiroshi Hayakawa --- internal/commands/build.go | 2 +- internal/commands/build_test.go | 58 ++++++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/internal/commands/build.go b/internal/commands/build.go index ef2400876..1d34c2ccc 100644 --- a/internal/commands/build.go +++ b/internal/commands/build.go @@ -327,7 +327,7 @@ func validateBuildFlags(flags *BuildFlags, cfg config.Config, inputImageRef clie return client.NewExperimentError("Exporting to OCI layout is currently experimental.") } - if !inputImageRef.Layout() && flags.AppPath == "" { + if _, err := os.Stat(inputImageRef.Name()); err == nil && flags.AppPath == "" { logger.Warnf("You are building an image named '%s'. If you mean it as an app directory path, run 'pack build --path %s'", inputImageRef.Name(), inputImageRef.Name()) } diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 920969881..5af7933e3 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -69,15 +69,13 @@ func testBuildCommand(t *testing.T, when spec.G, it spec.S) { }) when("a builder and image are set", func() { - it("builds an image with a builder"+ - "and warns that the positional argument will not be treated as the source path", func() { + it("builds an image with a builder", func() { mockClient.EXPECT(). Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", "image")). Return(nil) command.SetArgs([]string{"--builder", "my-builder", "image"}) h.AssertNil(t, command.Execute()) - h.AssertContains(t, outBuf.String(), "Warning: You are building an image named 'image'. If you mean it as an app directory path, run 'pack build --path image'") }) it("builds an image with a builder short command arg", func() { @@ -933,15 +931,51 @@ builder = "my-builder" }) when("path to app dir or zip-formatted file is provided", func() { - it("builds with the specified path"+ - "and doesn't warn that the positional argument will not be treated as the source path", func() { + it("builds with the specified path", func() { mockClient.EXPECT(). Build(gomock.Any(), EqBuildOptionsWithPath("my-source")). Return(nil) command.SetArgs([]string{"image", "--builder", "my-builder", "--path", "my-source"}) h.AssertNil(t, command.Execute()) - h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) + }) + }) + + when("a path with the same name as the image exists locally", func() { + var dir string + + it.Before(func() { + var err error + dir, err = os.MkdirTemp("", "my-app-dir") + h.AssertNil(t, err) + }) + + it.After(func() { + h.AssertNil(t, os.RemoveAll(dir)) + }) + + when("an app path is specified", func() { + it("doesn't warn that the positional argument will not be treated as the source path", func() { + mockClient.EXPECT(). + Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", dir)). + Return(nil) + + command.SetArgs([]string{dir, "--builder", "my-builder", "--path", "my-source"}) + h.AssertNil(t, command.Execute()) + h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) + }) + }) + + when("no app path is specified", func() { + it("warns that the positional argument will not be treated as the source path", func() { + mockClient.EXPECT(). + Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", dir)). + Return(nil) + + command.SetArgs([]string{dir, "--builder", "my-builder"}) + h.AssertNil(t, command.Execute()) + h.AssertContains(t, outBuf.String(), "Warning: You are building an image named '"+dir+"'. If you mean it as an app directory path, run 'pack build --path "+dir+"'") + }) }) }) @@ -973,8 +1007,7 @@ builder = "my-builder" }) when("path to save the image is provided", func() { - it("builds with oci layout configuration"+ - "and it doesn't warn that the positional argument will not be treated as the source path", func() { + it("build is called with oci layout configuration", func() { sparse = false mockClient.EXPECT(). Build(gomock.Any(), EqBuildOptionsWithLayoutConfig("image", previousImage, sparse, layoutDir)). @@ -983,13 +1016,11 @@ builder = "my-builder" command.SetArgs([]string{"oci:image", "--builder", "my-builder"}) err := command.Execute() h.AssertNil(t, err) - h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) }) }) when("previous-image flag is provided", func() { - it("builds with oci layout configuration"+ - "and it doesn't warn that the positional argument will not be treated as the source path", func() { + it("build is called with oci layout configuration", func() { sparse = false previousImage = "my-previous-image" mockClient.EXPECT(). @@ -999,13 +1030,11 @@ builder = "my-builder" command.SetArgs([]string{"oci:image", "--previous-image", "oci:my-previous-image", "--builder", "my-builder"}) err := command.Execute() h.AssertNil(t, err) - h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) }) }) when("-sparse flag is provided", func() { - it("build with oci layout configuration and sparse true"+ - "and it doesn't warn that the positional argument will not be treated as the source path", func() { + it("build is called with oci layout configuration and sparse true", func() { sparse = true mockClient.EXPECT(). Build(gomock.Any(), EqBuildOptionsWithLayoutConfig("image", previousImage, sparse, layoutDir)). @@ -1014,7 +1043,6 @@ builder = "my-builder" command.SetArgs([]string{"oci:image", "--sparse", "--builder", "my-builder"}) err := command.Execute() h.AssertNil(t, err) - h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) }) }) }) From 59a0abf322e27d387673aa61e8cc961a7a9b0e93 Mon Sep 17 00:00:00 2001 From: Hiroshi Hayakawa Date: Tue, 17 Sep 2024 12:56:42 +0900 Subject: [PATCH 5/7] rephrase the test description Signed-off-by: Hiroshi Hayakawa --- internal/commands/build_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 5af7933e3..799830977 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -941,7 +941,7 @@ builder = "my-builder" }) }) - when("a path with the same name as the image exists locally", func() { + when("a local path with the same string as the specified image name exists", func() { var dir string it.Before(func() { From 569e992b95fa9f5c72474b196b912b9a72849fff Mon Sep 17 00:00:00 2001 From: hhiroshell Date: Tue, 17 Sep 2024 23:11:25 +0900 Subject: [PATCH 6/7] avoid using os.MkdirTemp() to prevent test failures on macOS Signed-off-by: hhiroshell --- internal/commands/build_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 799830977..443c366bc 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -945,8 +945,9 @@ builder = "my-builder" var dir string it.Before(func() { - var err error - dir, err = os.MkdirTemp("", "my-app-dir") + // avoid using paths generated by os.MkdirTemp() as they cause test failures on macOS. + dir = "my-app-dir" + h.RandString(8) + err := os.Mkdir(dir, 0700) h.AssertNil(t, err) }) From 3aee96ea4f82385680d76108099c24b393182fea Mon Sep 17 00:00:00 2001 From: Hiroshi Hayakawa Date: Fri, 4 Oct 2024 22:35:16 +0900 Subject: [PATCH 7/7] Use an existing directory instead of creating and deleting directories for each '--path' flag case to ensure no temp directories are left behind in case of a test failure. Signed-off-by: Hiroshi Hayakawa --- internal/commands/build_test.go | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 443c366bc..1d4f4b529 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -942,26 +942,13 @@ builder = "my-builder" }) when("a local path with the same string as the specified image name exists", func() { - var dir string - - it.Before(func() { - // avoid using paths generated by os.MkdirTemp() as they cause test failures on macOS. - dir = "my-app-dir" + h.RandString(8) - err := os.Mkdir(dir, 0700) - h.AssertNil(t, err) - }) - - it.After(func() { - h.AssertNil(t, os.RemoveAll(dir)) - }) - when("an app path is specified", func() { it("doesn't warn that the positional argument will not be treated as the source path", func() { mockClient.EXPECT(). - Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", dir)). + Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", "testdata")). Return(nil) - command.SetArgs([]string{dir, "--builder", "my-builder", "--path", "my-source"}) + command.SetArgs([]string{"testdata", "--builder", "my-builder", "--path", "my-source"}) h.AssertNil(t, command.Execute()) h.AssertNotContainsMatch(t, outBuf.String(), `Warning: You are building an image named '([^']+)'\. If you mean it as an app directory path, run 'pack build --path ([^']+)'`) }) @@ -970,12 +957,12 @@ builder = "my-builder" when("no app path is specified", func() { it("warns that the positional argument will not be treated as the source path", func() { mockClient.EXPECT(). - Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", dir)). + Build(gomock.Any(), EqBuildOptionsWithImage("my-builder", "testdata")). Return(nil) - command.SetArgs([]string{dir, "--builder", "my-builder"}) + command.SetArgs([]string{"testdata", "--builder", "my-builder"}) h.AssertNil(t, command.Execute()) - h.AssertContains(t, outBuf.String(), "Warning: You are building an image named '"+dir+"'. If you mean it as an app directory path, run 'pack build --path "+dir+"'") + h.AssertContains(t, outBuf.String(), "Warning: You are building an image named 'testdata'. If you mean it as an app directory path, run 'pack build --path testdata'") }) }) })