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

Deprecation: change --stream error to a warning, and update deprecated.md #2809

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 23, 2020

Relates to #2105
Relates to moby/moby#39983

Addresses docker-archive/docker-ce#660 (comment)

builder: print deprecation warning instead of failing for --stream

While performance will be worse, we can safely ignore the --stream option when used, and print a deprecation warning instead of failing the build.

With this patch:

echo -e "FROM scratch\nLABEL foo=bar" | docker build --stream -
DEPRECATED: The experimental --stream flag has been removed and the build context
            will be sent non-streaming. Enable BuildKit instead with DOCKER_BUILDKIT=1
            to stream build context, see https://docs.docker.com/go/buildkit/

Sending build context to Docker daemon  2.048kB
Step 1/2 : FROM scratch
 --->
Step 2/2 : LABEL foo=bar
 ---> Running in 99e4021085b6
Removing intermediate container 99e4021085b6
 ---> 1a7a41be241f
Successfully built 1a7a41be241f

Deprecation: add experimental docker build --stream option

Docker v17.07 introduced an experimental --stream flag on docker build which
allowed the build-context to be incrementally sent to the daemon, instead of
unconditionally sending the whole build-context.

This functionality has been reimplemented as part of BuildKit, which uses streaming
by default and the --stream option will be ignored when using the classic builder,
printing a deprecation warning instead.

Users that want to use this feature are encouraged to enable BuildKit by setting
the DOCKER_BUILDKIT=1 environment variable or through the daemon or CLI configuration
files.

@thaJeztah
Copy link
Member Author

@tiborvass @cpuguy83 @tonistiigi PTAL

@codecov-io
Copy link

Codecov Report

Merging #2809 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2809      +/-   ##
==========================================
- Coverage   57.14%   57.13%   -0.01%     
==========================================
  Files         297      297              
  Lines       18639    18642       +3     
==========================================
  Hits        10651    10651              
- Misses       7129     7132       +3     
  Partials      859      859              

@@ -246,7 +246,11 @@ func runBuild(dockerCli command.Cli, options buildOptions) error {
)

if options.stream {
return errors.New("Experimental flag --stream was removed, enable BuildKit instead with DOCKER_BUILDKIT=1")
_, _ = fmt.Fprint(dockerCli.Err(), `DEPRECATED: The experimental --stream flag is deprecated and the build context
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/deprecated/removed/

Copy link
Member Author

Choose a reason for hiding this comment

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

I was a bit in doubt, as the flag is "technically" still there, but yeah from a user's perspective, I guess we could say it's removed.

@@ -246,7 +246,11 @@ func runBuild(dockerCli command.Cli, options buildOptions) error {
)

if options.stream {
return errors.New("Experimental flag --stream was removed, enable BuildKit instead with DOCKER_BUILDKIT=1")
_, _ = fmt.Fprint(dockerCli.Err(), `DEPRECATED: The experimental --stream flag is deprecated and the build context
will be sent non-streaming. Enable BuildKit instead with DOCKER_BUILDKIT=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Consider using BuildKit instead", maybe with a doc link?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion; I recently added docker/docs#11460, and this would be a good candidate for that. Need to check if we have a good location already to point users to (on how to enable buildkit).

can then add a URL like https://docs.docker.com/go/buildkit

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened docker/docs#11621 to implement https://docs.docker.com/go/buildkit/ as a redirect

While performance will be worse, we can safely ignore the --stream
option when used, and print a deprecation warning instead of failing
the build.

With this patch:

    echo -e "FROM scratch\nLABEL foo=bar" | docker build --stream -
    DEPRECATED: The experimental --stream flag has been removed and the build context
                will be sent non-streaming. Enable BuildKit instead with DOCKER_BUILDKIT=1
                to stream build context, see https://docs.docker.com/go/buildkit/

    Sending build context to Docker daemon  2.048kB
    Step 1/2 : FROM scratch
     --->
    Step 2/2 : LABEL foo=bar
     ---> Running in 99e4021085b6
    Removing intermediate container 99e4021085b6
     ---> 1a7a41be241f
    Successfully built 1a7a41be241f

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Docker v17.07 introduced an experimental `--stream` flag on `docker build` which
allowed the build-context to be incrementally sent to the daemon, instead of
unconditionally sending the whole build-context.

This functionality has been reimplemented as part of BuildKit, which uses streaming
by default and the `--stream` option will be ignored when using the classic builder,
printing a deprecation warning instead.

Users that want to use this feature are encouraged to enable BuildKit by setting
the `DOCKER_BUILDKIT=1` environment variable or through the daemon or CLI configuration
files.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

Updated; PTAL

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants