docker-py icon indicating copy to clipboard operation
docker-py copied to clipboard

Clarify documentation of `path` argument when building an image

Open i18n-tribe opened this issue 3 years ago • 1 comments

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 avatar Sep 09 '22 14:09 i18n-tribe

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.

i18n-tribe avatar Nov 11 '22 09:11 i18n-tribe