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

Clarify documentation of path argument when building an image #3043

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

i18n-tribe
Copy link

Current documentation of some important arguments to the build command is:

path (str) – Path to the directory containing the Dockerfile
dockerfile (str) – path within the build context to the Dockerfile

The documentation for path seems a bit ambiguous.

Does this mean:

  1. the immediate parent directory containing the Dockerfile?
  2. a directory that ultimately contains the Dockerfile, which might be nested in child folder(s)?

When I first read the docs I thought 1. was the right interpretation.

But from my knowledge of docker, and double-checking the source - it seems 2. is the right intpretation.

So this PR updates the documentation to be more clear. I added this text:

Typically, the Dockerfile is a direct child of `path`,
but it can also be in a nested directory.

This will avoid misunderstandings by developers in future.

To further clarify - I made the link between the path and dockerfile arguments more explicit.

dockerfile is the path within the build context to the Dockerfile

But what exactly is the build context?

It's closely related to path but this is not mentioned in the documentation for path.

So I added this text:

A copy of the `path` directory, excluding files specified
in `.dockerignore`, will be used as the build context.

This makes a clear link between the path and dockerfile arguments.

It also makes a clear link between path and build context, and explains what build context is.

@i18n-tribe
Copy link
Author

i18n-tribe commented Nov 11, 2022

We believe that when the authors of the AWS SAM CLI wrote an integration with the python docker client, they misinterpreted this documentation. The result was they thought the SAM CLI didn't support path args that were a path to a Dockerfile via nested subfolders.

Knock on effect of this was when the AWS SAM CLI team wrote an integration with the AWS CDK, it was not fully compatible.

This issue is addressed in this PR Which has now been merged and released with SAM CLI 1.62.0. This is a good outcome, which took quite a lot of time and work to get to.

There is a very long discussion on the PR. If you want to see why exactly we believe misinterpretation of this documentation was the root cause, I would recommend reading the full discussion. Or you can take my word for it :)

@milas if you were someone else has a chance to review this PR at some point – I believe it will be really helpful to future developers. The small change could save a lot of time for other people in the future.

Copy link
Contributor

@milas milas 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 the PR! This is definitely misleading - really the argument should be named context.

I reworded things a bit to explicitly use the phrase "build context" and point to the other args if you're not going to use Dockerfile from the context root - "it can also be placed in a nested directory" was a bit confusing on its own.

⚠️ One thing to note: you will need to sign-off on the PR before it can be merged by following the instructions at https://github.com/docker/docker-py/pull/3043/checks?check_run_id=8272433468.

Comment on lines +62 to +66
path (str): Path to the directory containing the Dockerfile.
Typically, the Dockerfile is a direct child of `path`,
but it can also be in a nested directory.
A copy of the `path` directory, excluding files specified
in `.dockerignore`, will be used as the build context.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path (str): Path to the directory containing the Dockerfile.
Typically, the Dockerfile is a direct child of `path`,
but it can also be in a nested directory.
A copy of the `path` directory, excluding files specified
in `.dockerignore`, will be used as the build context.
path (str): Build context (either path to directory or Git URL).
By default, the Dockerfile in the context root will be used.
Alternatively, specify either the dockerfile or fileobj
arguments.

Comment on lines +237 to +241
path (str): Path to the directory containing the Dockerfile.
Typically, the Dockerfile is a direct child of `path`,
but it can also be in a nested directory.
A copy of the `path` directory, excluding files specified
in `.dockerignore`, will be used as the build context.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path (str): Path to the directory containing the Dockerfile.
Typically, the Dockerfile is a direct child of `path`,
but it can also be in a nested directory.
A copy of the `path` directory, excluding files specified
in `.dockerignore`, will be used as the build context.
path (str): Build context (either path to directory or Git URL).
By default, the Dockerfile in the context root will be used.
Alternatively, specify either the dockerfile or fileobj
arguments.

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.

2 participants