build: only show buildkit-specific flags if buildkit is enabled by tiborvass · Pull Request #1427 · docker/cli

tiborvass · 2018-10-10T02:13:47Z

Signed-off-by: Tibor Vass tibor@docker.com

codecov-io · 2018-10-10T02:19:14Z

Codecov Report

Merging #1427 into master will decrease coverage by 0.02%.
The diff coverage is 23.33%.

@@ Coverage Diff @@## master #1427 +/- ##
=========================================
- Coverage 54.22% 54.2% -0.03% 
=========================================
Files 289 289 Lines 19359 19378 +19 =========================================
+ Hits 10498 10503 +5 - Misses 8185 8199 +14 
Partials 676 676

thaJeztah

changes LGTM, but left a command on updating the flag descriptions

Sign in to view


 	flags.StringArrayVar(&options.secrets, "secret", []string{}, "Secret file to expose to the build (only if BuildKit enabled): id=mysecret,src=/local/secret")
 	flags.SetAnnotation("secret", "version", []string{"1.39"})
+	flags.SetAnnotation("secret", "buildkit", nil)


tiborvass · 2018-10-10T18:12:00Z

@thaJeztah @AkihiroSuda fixed!

AkihiroSuda · 2018-10-10T18:24:20Z

--squash seems also ignored in BuildKit mode now?

Probably, we should explicitly reject --squash for security purpose.

cc @tonistiigi

AkihiroSuda · 2018-10-10T18:30:28Z

--compress also needs no-buildkit

tiborvass · 2018-10-10T19:04:26Z

@AkihiroSuda fixed. I didn't error out for squash yet, unless we agree it's needed.
@tonistiigi changed the BuildKitEnabled() method to a utility function.

thaJeztah · 2018-10-10T19:19:03Z

 	serverInfo ServerInfo
 	clientInfo ClientInfo
 	contentTrust bool
+	buildkitEnabled *bool


linting is failing on this one;
cli/command/cli.go:73:2:warning: field buildkitEnabled is unused (U1000) (unused)

tonistiigi · 2018-10-10T19:51:39Z


+// BuildKitEnabled returns whether buildkit is enabled either through a daemon setting
+// or otherwise the client-side DOCKER_BUILDKIT environment variable
+func BuildKitEnabled(cli interface{ ServerInfo() ServerInfo }) (bool, error) {


func BuildKitEnabled(si ServerInfo) (bool, error) {

Signed-off-by: Tibor Vass <tibor@docker.com>

AkihiroSuda · 2018-10-11T02:39:21Z

 if _, ok := f.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
 errs = append(errs, fmt.Sprintf("\"--%s\" is on a Docker cli with experimental cli features enabled", f.Name))
 }
+ // buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case


Maybe we can still print logrus.Warn warnings, especially for NOP --squash with BuildKit mode

Could make sense to inform them that an option is only supported with/without buildkit, and will be ignored.
Given that we're already ignoring those flags in the current code, I think that'd be good to do separately, in a follow-up;
DOCKER_BUILDKIT=1 docker build --squash . [+] Building 0.3s (4/4) FINISHED => local://context (.dockerignore) 0.0s => => transferring context: 2B 0.0s => local://dockerfile (Dockerfile) 0.0s => => transferring dockerfile: 31B 0.0s => CACHED docker-image://docker.io/library/alpine:latest 0.0s => exporting to image 0.0s => => exporting layers 0.0s => => writing image sha256:0b4ed36840824a86314da6264a95047c664f3c2fe90d0b1a6ddb118e20f106a2
DOCKER_BUILDKIT=0 docker build --progress=plain . Sending build context to Docker daemon 2.048kB Step 1/1 : FROM alpine ---> 11cd0b38bc3c Successfully built 11cd0b38bc3c

thaJeztah

LGTM, thanks!

thaJeztah · 2018-10-11T09:34:04Z

 if _, ok := f.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
 errs = append(errs, fmt.Sprintf("\"--%s\" is on a Docker cli with experimental cli features enabled", f.Name))
 }
+ // buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case


Could make sense to inform them that an option is only supported with/without buildkit, and will be ignored.
Given that we're already ignoring those flags in the current code, I think that'd be good to do separately, in a follow-up;
DOCKER_BUILDKIT=1 docker build --squash . [+] Building 0.3s (4/4) FINISHED => local://context (.dockerignore) 0.0s => => transferring context: 2B 0.0s => local://dockerfile (Dockerfile) 0.0s => => transferring dockerfile: 31B 0.0s => CACHED docker-image://docker.io/library/alpine:latest 0.0s => exporting to image 0.0s => => exporting layers 0.0s => => writing image sha256:0b4ed36840824a86314da6264a95047c664f3c2fe90d0b1a6ddb118e20f106a2
DOCKER_BUILDKIT=0 docker build --progress=plain . Sending build context to Docker daemon 2.048kB Step 1/1 : FROM alpine ---> 11cd0b38bc3c Successfully built 11cd0b38bc3c

GordonTheTurtle added the status/0-triage label Oct 10, 2018

tiborvass requested review from cpuguy83, thaJeztah and tonistiigi October 10, 2018 02:13

thaJeztah added status/2-code-review area/builder and removed status/0-triage labels Oct 10, 2018

thaJeztah requested changes Oct 10, 2018
View reviewed changes

Comment threadcmd/docker/docker.go

AkihiroSuda reviewed Oct 10, 2018
View reviewed changes

tiborvass force-pushed the hide-buildkit-flags-if-not-enabled branch from 6a509ee to efeec06Compare October 10, 2018 18:11

tiborvass force-pushed the hide-buildkit-flags-if-not-enabled branch from efeec06 to 37ac307Compare October 10, 2018 19:03

thaJeztah reviewed Oct 10, 2018
View reviewed changes

tonistiigi reviewed Oct 10, 2018
View reviewed changes

tiborvass force-pushed the hide-buildkit-flags-if-not-enabled branch 2 times, most recently from fcefefd to b98f5d5Compare October 10, 2018 21:08

build: only show buildkit-specific flags if buildkit is enabled
bbd01fe
Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the hide-buildkit-flags-if-not-enabled branch from b98f5d5 to bbd01feCompare October 10, 2018 21:09

tonistiigi approved these changes Oct 10, 2018
View reviewed changes

AkihiroSuda reviewed Oct 11, 2018
View reviewed changes

thaJeztah approved these changes Oct 11, 2018
View reviewed changes

thaJeztah merged commit 6153a09 into docker:masterOct 11, 2018

GordonTheTurtle added this to the 19.03.0 milestone Oct 11, 2018

thaJeztah mentioned this pull request Oct 11, 2018
[18.09 backport] builder / buildkit updates #1438
Merged

thaJeztah mentioned this pull request Sep 22, 2020
build: print error if BuildKit/non-BuildKit-specific flags are used #2736
Open

Uh oh!

Conversation

tiborvass commented Oct 10, 2018

Uh oh!

codecov-io commented Oct 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

tiborvass commented Oct 10, 2018

Uh oh!

AkihiroSuda commented Oct 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Oct 10, 2018

Uh oh!

tiborvass commented Oct 10, 2018

Uh oh!

thaJeztahOct 10, 2018

Choose a reason for hiding this comment

Uh oh!

tonistiigiOct 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSudaOct 11, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztahOct 11, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztahOct 11, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Oct 10, 2018•
edited
Loading

AkihiroSuda commented Oct 10, 2018•
edited
Loading

tonistiigiOct 10, 2018•
edited
Loading