-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
add more buildkit docs #1493
add more buildkit docs #1493
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1493 +/- ##
=======================================
Coverage 55.23% 55.23%
=======================================
Files 289 289
Lines 19374 19374
=======================================
Hits 10702 10702
Misses 7977 7977
Partials 695 695 |
f0f9375
to
0794a26
Compare
docs/reference/builder.md
Outdated
@@ -226,8 +248,61 @@ following lines are all treated identically: | |||
|
|||
The following parser directive is supported: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following parser directives are supported:
cc @ahh-docker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
docs/reference/builder.md
Outdated
|
||
## BuildKit | ||
|
||
Starting from version 18.09, Docker supports a new backend for executing your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/18.09/18.06/ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is too complicated to explain the need to enable experimental for 18.06
. If you want to just use BuildKit the recommendation is to upgrade to at least 18.09
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks god; left some comments inline
also ping @ahh-docker PTAL
docs/reference/builder.md
Outdated
## BuildKit | ||
|
||
Starting from version 18.09, Docker supports a new backend for executing your | ||
builds that is provided by [moby/buildkit](https://github.com/moby/buildkit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provided by -> provided by the
docs/reference/builder.md
Outdated
* Incrementally transfer only the changed files in your build context between builds | ||
* Detect and skip transferring unused files in your build context | ||
* Use external Dockerfile implementations with many new features | ||
* Avoid leaking dangling images and containers to the rest of the API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"leaking dangling images" makes it sound like it's a bug, but it's a limitation of the old builder. Perhaps something like; "Use a dedicated, configurable build cache, omitting the use of intermediate images as a caching mechanism".
* Detect and skip transferring unused files in your build context | ||
* Use external Dockerfile implementations with many new features | ||
* Avoid leaking dangling images and containers to the rest of the API | ||
* Prioritize your build cache for automatic pruning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the "Prioritize". Is this the configurable garbage collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is gc and prune --keep-storage
that removes unused based on priority.
docs/reference/builder.md
Outdated
* Avoid leaking dangling images and containers to the rest of the API | ||
* Prioritize your build cache for automatic pruning | ||
|
||
To use the BuildKit backend in 18.09, you need to set an environment variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the in 18.09
here; we generally try to avoid calling out which version something was introduced in (as the reference documentation is already versioned), and we already mention this in the introduction.
* Prioritize your build cache for automatic pruning | ||
|
||
To use the BuildKit backend in 18.09, you need to set an environment variable | ||
`DOCKER_BUILDKIT=1` on the CLI before invoking `docker build`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is now also configurable in the CLI configuration file? (if so, we should have a link to that configuration somewhere). Could be in a new section "configure the Docker CLI to use the BuildKit builder" (e.g.)
Wondering which one we consider the "preferred" mechanism, and/or if we should call out (dis)advantages of both ways to configure.
(I'm ok with addressing that in a follow-up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of it was covered by #1314
docs/reference/builder.md
Outdated
external implementations of builders that are distributed as Docker images and | ||
execute inside a container sandbox environment. | ||
|
||
Using a custom Dockerfile implementation allows you to: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Using a" could be removed here;
Custom Dockerfile implementations allow you to:
Also, could you add a blank line between this line and the bullet-list?
docs/reference/builder.md
Outdated
|
||
Here is the complete list of the different build `ARG` variables: | ||
|
||
* TARGETPLATFORM - platform of the build result. Eg `linux/amd64`, `linux/arm/v7`, `windows/amd64`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put the arg's in backticks? TARGETPLATFORM
, TARGETOS
(and so on)
docs/reference/builder.md
Outdated
resulting image (target platform). The target platform can be specified with | ||
the `--platform` flag on `docker build`. | ||
|
||
Here is the complete list of the different build `ARG` variables: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps; "The following ARG
variables are set automatically:"
docs/reference/builder.md
Outdated
|
||
For example: | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add Dockerfile
code hint?
```Dockerfile
docs/reference/builder.md
Outdated
@@ -1931,6 +2038,14 @@ required such as `zsh`, `csh`, `tcsh` and others. | |||
|
|||
The `SHELL` feature was added in Docker 1.12. | |||
|
|||
## Experimental features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs a mention in the experimental docs; https://github.com/docker/cli/blob/master/experimental/README.md (ok for a follow-up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the same experimental. Changed to external to avoid confusion.
@tonistiigi please add me as a reviewer. Thanks! |
@thaJeztah updated |
@thaJeztah ping. can we merge this as 18.09 is out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐯
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Tibor Vass <tibor@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
4e922d5
to
83aeb21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
squashed the commits, and fixed one typo
@tiborvass @AkihiroSuda @andrewhsu @thaJeztah
wip: the links to buildkit repo need moby/buildkit#710mergedFeel free to carry or change directly on lot of changes.
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com