cli icon indicating copy to clipboard operation
cli copied to clipboard

build: print error if BuildKit/non-BuildKit-specific flags are used

Open thaJeztah opened this issue 5 years ago • 10 comments

~depends on https://github.com/docker/cli/pull/2842~ merged fixes https://github.com/docker/cli/issues/2680 implements https://github.com/docker/cli/pull/1427#discussion_r224380101

With this patch, the --progress, --secret, --ssh, and --output flags trigger an error when trying to use without BuildKit enabled;

DOCKER_BUILDKIT=0 docker build --progress=plain .
--progress is only supported with BuildKit enabled. Enable BuildKit with DOCKER_BUILDKIT=1

DOCKER_BUILDKIT=0 docker build --output=foo . --output is only supported with BuildKit enabled. Enable BuildKit with DOCKER_BUILDKIT=1

Likewise, options that are not supported yet by BuildKit, now trigger an error:

DOCKER_BUILDKIT=1 docker build --memory=500M .
--memory is not supported with BuildKit enabled. Disable BuildKit with DOCKER_BUILDKIT=0

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

thaJeztah avatar Sep 22 '20 13:09 thaJeztah

@tiborvass @tonistiigi @silvin-lubecki PTAL

thaJeztah avatar Sep 22 '20 13:09 thaJeztah

Pushed a second commit to try integrating it with areFlagsSupported() however, that function is called by isSupported(), which is called by PersistentPreRunE, but I think we override that for build; checking now

thaJeztah avatar Sep 22 '20 14:09 thaJeztah

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2736      +/-   ##
==========================================
- Coverage   57.15%   57.13%   -0.02%     
==========================================
  Files         297      297              
  Lines       18657    18663       +6     
==========================================
  Hits        10663    10663              
- Misses       7132     7136       +4     
- Partials      862      864       +2     

codecov-commenter avatar Sep 22 '20 14:09 codecov-commenter

opened https://github.com/docker/cli/pull/2737 to remove the pre-run hack

thaJeztah avatar Sep 22 '20 15:09 thaJeztah

Squashed the commits, and rebased on top of https://github.com/docker/cli/pull/2737 (that one needs to be merged first)

thaJeztah avatar Sep 22 '20 15:09 thaJeztah

@thaJeztah this PR needs a rebase 🦁

silvin-lubecki avatar Oct 28 '20 10:10 silvin-lubecki

@tiborvass PTAL

thaJeztah avatar Dec 08 '20 15:12 thaJeztah

The flags marked with no-buildkit annotation do not change the outcome of the build, with the exception of security-opt maybe. So I'm not in favor of erroring on them. Hiding should be enough to recommend not using them.

tonistiigi avatar Dec 08 '20 15:12 tonistiigi

Do you think we should print a warning? e.g. if someone expects to have set memory limits, but they are not applied?

thaJeztah avatar Dec 08 '20 16:12 thaJeztah

Codecov Report

Merging #2736 (e2986f4) into master (51a0914) will decrease coverage by 0.01%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2736      +/-   ##
==========================================
- Coverage   57.10%   57.08%   -0.02%     
==========================================
  Files         297      297              
  Lines       18635    18641       +6     
==========================================
  Hits        10642    10642              
- Misses       7134     7138       +4     
- Partials      859      861       +2     

codecov-io avatar Mar 30 '21 10:03 codecov-io