Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make the pack build warn that the positional argument will not be treated as the source directory path #2256

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+53 −4
Open

Make the pack build warn that the positional argument will not be treated as the source directory path #2256

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0a5cf9c
Make the 'pack build' command warn that the positional argument will …
hhiroshell Sep 2, 2024
7066e52
Add a test case for when the 'pack build' is called with the '--path'…
hhiroshell Sep 5, 2024
66982d0
Improve descriptions for some test cases.
hhiroshell Sep 5, 2024
cee65f2
Make it warns only when a local path with the same string as the spec…
hhiroshell Sep 17, 2024
59a0abf
rephrase the test description
hhiroshell Sep 17, 2024
569e992
avoid using os.MkdirTemp() to prevent test failures on macOS
hhiroshell Sep 17, 2024
98c32ed
Merge branch 'main' into fix-pack-2231
jjbustamante Sep 27, 2024
3aee96e
Use an existing directory instead of creating and deleting directorie…
hhiroshell Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions internal/commands/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -328,6 +327,11 @@ func validateBuildFlags(flags *BuildFlags, cfg config.Config, inputImageRef clie
return client.NewExperimentError("Exporting to OCI layout is currently experimental.")
}

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 <args> --path %s'",
inputImageRef.Name(), inputImageRef.Name())
}

return nil
}

Expand Down
62 changes: 60 additions & 2 deletions internal/commands/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -931,6 +930,56 @@ builder = "my-builder"
})
})

when("path to app dir or zip-formatted file is provided", 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())
})
})

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)
Copy link
Contributor Author

@hhiroshell hhiroshell Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A path created by os.MkdirTemp() on macOS is like: /var/folders/vh/0l94sp8n6hx8qwhb6wsy_xrh0000gr/T/tmp.4ffiJJd5rq
It triggers an error in the validation of the image name when specified as a positional argument (image name), and results in test failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we creating the my-app-dir folder in the current directory? if for some reason the test fails and I am running the tests locally, will I see this new my-app-dir like a new file not being tracked by git?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that creating a new directory in the local source is a bit weird. An alternative would be to invoke pack build testdata since (I'm pretty sure) this test runs in the context of /internal/commands/. We could also try some combination of os.MkdirTemp and os.Chdir to create a directory with a sensible reference, but that might be a bit heavy-handed for what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjbustamante @natalieparellano
Thank you for your comments and suggestions! I'll try to fix it to use the testdata. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 3aee96e

Now the code looks clean, and there will be no residue left in case of test failures. 😌

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 <args> --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 <args> --path "+dir+"'")
})
})
})

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"})
Expand Down Expand Up @@ -1175,6 +1224,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),
Expand Down
Loading