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.

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

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

Conversation

hhiroshell
Copy link
Contributor

@hhiroshell hhiroshell commented Sep 2, 2024

Summary

This PR makes the pack build warn that the positional argument specifying the image name will not be treated as the source directory path, addressing #2231.

Output

Before

$ pack build sample-app --builder cnbs/sample-builder:jammy
jammy: Pulling from cnbs/sample-builder
Digest: sha256:9db376e26252b6cbc75190c88ea24571da36ec0d373136be233b32fd174962fc
Status: Image is up to date for cnbs/sample-builder:jammy
...

After

pack build warns it if both of the following criteria are met:

  • The build artifact is not specified as an OCI file layout.
  • The --path flag is not used.
$ pack build sample-app --builder cnbs/sample-builder:jammy
Warning: You are building an image named 'sample-app'. If you mean it as an app directory path, run 'pack build <args> --path sample-app'
jammy: Pulling from cnbs/sample-builder
Digest: sha256:9db376e26252b6cbc75190c88ea24571da36ec0d373136be233b32fd174962fc
Status: Image is up to date for cnbs/sample-builder:jammy
...

pack build doesn't warn it if the build artifact is specified as an OCI file layout.

$ pack config experimental true
Experimental features enabled!

$ pack build oci:sample-app --builder cnbs/sample-builder:jammy
jammy: Pulling from cnbs/sample-builder
Digest: sha256:9db376e26252b6cbc75190c88ea24571da36ec0d373136be233b32fd174962fc
Status: Image is up to date for cnbs/sample-builder:jammy
...

pack build doesn't warn it if the --path flag is specified.

$ pack build sample-app --builder cnbs/sample-builder:jammy --path apps/java-maven
jammy: Pulling from cnbs/sample-builder
Digest: sha256:9db376e26252b6cbc75190c88ea24571da36ec0d373136be233b32fd174962fc
Status: Image is up to date for cnbs/sample-builder:jammy
...

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #2231

…not be treated as the source directory path.

Signed-off-by: Hiroshi Hayakawa <[email protected]>
@hhiroshell hhiroshell requested review from a team as code owners September 2, 2024 07:12
@github-actions github-actions bot added this to the 0.36.0 milestone Sep 2, 2024
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Sep 2, 2024
@hhiroshell hhiroshell marked this pull request as draft September 3, 2024 16:07
@hhiroshell
Copy link
Contributor Author

hhiroshell commented Sep 3, 2024

I'm thinking about adding some test cases for when the pack build is executed with the --path flag.
Please give me some time before proceeding with the review.

→ Done.

@hhiroshell hhiroshell marked this pull request as ready for review September 5, 2024 11:58
@hhiroshell
Copy link
Contributor Author

All the changes are complete, so please review them!

@hhiroshell
Copy link
Contributor Author

hhiroshell commented Sep 17, 2024

With the latest two commits(cee65f2, 59a0abf) I made, the behavior of the pack build has changed as below:

pack build warns it if both of the following criteria are met:

  • A local path with the same string as the specified image name exists
  • The --path flag is not used.
$ test -e apps/java-maven && echo "exists" || echo "does not exist"
exists

$ pack build apps/java-maven --builder cnbs/sample-builder:jammy
Warning: You are building an image named 'apps/java-maven'. If you mean it as an app directory path, run 'pack build <args> --path apps/java-maven'
jammy: Pulling from cnbs/sample-builder
Digest: sha256:9db376e26252b6cbc75190c88ea24571da36ec0d373136be233b32fd174962fc
Status: Image is up to date for cnbs/sample-builder:jammy
...

pack build doesn't warn it if a local path with the same string as the specified image name doesn't exist.

$ test -e aaaa && echo "exists" || echo "does not exist"
does not exist

$ pack build aaaa --builder cnbs/sample-builder:jammy
jammy: Pulling from cnbs/sample-builder
Digest: sha256:9db376e26252b6cbc75190c88ea24571da36ec0d373136be233b32fd174962fc
Status: Image is up to date for cnbs/sample-builder:jammy
...

pack build doesn't warn it if a local path with the same string as the specified image name exists, and --path is used.

$ test -e apps/java-maven && echo "exists" || echo "does not exist"
exists

$ pack build apps/java-maven --path apps/java-maven --builder cnbs/sample-builder:jammy
jammy: Pulling from cnbs/sample-builder
Digest: sha256:9db376e26252b6cbc75190c88ea24571da36ec0d373136be233b32fd174962fc
Status: Image is up to date for cnbs/sample-builder:jammy
...

Comment on lines 948 to 950
// 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

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. 😌

@hhiroshell
Copy link
Contributor Author

@natalieparellano
I believe all comments have been resolved. Would you please confirm that?

If I need to explain some recent changes (#2256 (comment), #2256 (comment) ), I'll happily do so. So please let me know.

@natalieparellano
Copy link
Member

@hhiroshell thank you for your work on this, and apologies for being slow to get back to you with a review. I think we should address the concern here: #2256 (comment) (added some suggestions)

…s for each '--path' flag case to ensure no temp directories are left behind in case of a test failure.

Signed-off-by: Hiroshi Hayakawa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UX bug, warn when using the image name the same as a local directory
3 participants