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

Modify a 'docker build' command with no context specified to infer "." as the context. #2947

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

marzhall
Copy link

@marzhall marzhall commented Jan 23, 2021

Also update documentation to match.

Signed-off-by: Marshall Conover [email protected]

- What I did
Removed the requirement to specify a context when running docker build.
- How I did it
Modified the options parsing for docker build to use the current working directory as the context when no context is specified.
- How to verify it
In a directory with a Dockerfile, and with an updated binary, run:

    docker build

You will see a line to stdout with the message

    No build context was specified. Using the current working directory '<current working directory>' as our build context.

followed by the docker build command executing as if you had run docker build ..

This is in contrast to the previous behavior, in which case running docker build would return:

    "docker build" requires exactly 1 argument.

followed by the client exiting.

- Description for the changelog

Infer the local directory as the context to use if no context is specified. docker build is now the same as docker build .

- A picture of a cute animal (not mandatory but encouraged)
image

Important Notes

  • I was not able to figure out how to run the unit tests from within the docker container development environment; I'm cracking at it, but wanted to note that the current documentation (at least that I saw) didn't have a straightforward "just run these commands" in it. I'm happy to update the doc once I have it working.
  • I'm unsure of how to test the error case in the code, as it requires os.GetCWD to fail. Pointers on how to do so would be helpful, though (like the above issue) I can continue digging.

Thanks!

(PR for #2946 )

@marzhall marzhall requested a review from thaJeztah as a code owner January 23, 2021 20:08
@marzhall marzhall changed the title Modify a 'docker build' command with no context specified to infer a … Modify a 'docker build' command with no context specified to infer "." as the context. Jan 23, 2021
…context of '.'. Also update documentation to match.

Signed-off-by: Marshall Conover <[email protected]>
@marzhall marzhall force-pushed the 2946-infer-context-when-not-specified branch from 89659b8 to 37402b4 Compare January 23, 2021 20:34
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! This change will need further discussion. We've had a look at similar changes in the past, and there's some reasons for not making this change;

  • keeping it explicit makes sure that unintended missing paths produce an error, instead of "silently" picking the current directory. These situations happen, e.g., when using an environment variable to specify the path (docker build $SOME_VARIABLE)
  • we want to keep the option open to use docker build (without specifying options or a path) to perform a more "interactive" build. No decisions were made on this yet, but changing the behavior to default to . would no longer allow us to move in that direction without introducing a breaking change.

For reasons above, I'm "-1" on merging this; I do understand where you're coming from though 🤗

@marzhall
Copy link
Author

Hi thaJeztah!

I don't know if that's considered the final word or not, but in case not, I'll just throw in a few thoughts:

  • For the first point, that's totally reasonable; would a line to stdout or stderr saying "no context was specified, so we're defaulting to the current directory" address that? I can add that later today.
  • For the second point, while the idea is interesting, I'm not sure sacrificing a clear and easy-to-implement feature that improves QoL immediately for a feature that might potentially be implemented in the future is the best priority planning-wise. In the case interactive mode was desired after this change went in, it could be implemented as a switch (e.g. --interactive), which I think would also better match its frequency of use by developers; intuitively, I think developers who forget to specify a context are more likely to intend to have typed . than to want a manual prompt. In my example of making the mistake in my builds, for example, it would've been only slightly less frustrating. Having a prompt appear when no context is specified also makes the client continue to have 'surprising' behavior vs. its other command-line cousins like ls who require directories be specified, and infer you meant the local directory should you not specify one.

Let me know if it would be more convenient to discuss this in another spot, and thanks for your time!

@marzhall marzhall force-pushed the 2946-infer-context-when-not-specified branch from acd9820 to eafac7c Compare January 25, 2021 23:01
@marzhall
Copy link
Author

I've added informational output to inform the user when the context has been inferred, e.g.:

$ build/docker-linux-amd64 build
No build context was specified. Using the current working directory '/home/mars/fun/cli' as our build context.
Sending build context to Docker daemon  45.37MB
Step 1/2 : FROM ubuntu
 ---> f63181f19b2f
Step 2/2 : RUN echo test
 ---> Using cache
 ---> c8e2247d0739
Successfully built c8e2247d0739

@codecov-commenter
Copy link

Codecov Report

Merging #2947 (2332b77) into master (a49d70c) will decrease coverage by 0.03%.
The diff coverage is 23.07%.

❗ Current head 2332b77 differs from pull request most recent head e10cb44. Consider uploading reports for the commit e10cb44 to get more accurate results

@@            Coverage Diff             @@
##           master    #2947      +/-   ##
==========================================
- Coverage   57.10%   57.06%   -0.04%     
==========================================
  Files         299      299              
  Lines       18665    18674       +9     
==========================================
- Hits        10658    10657       -1     
- Misses       7146     7155       +9     
- Partials      861      862       +1     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants