Skip to content

build: only show buildkit-specific flags if buildkit is enabled#1427

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Oct 11, 2018
Merged

build: only show buildkit-specific flags if buildkit is enabled #1427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions cli/command/cli.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,6 +8,7 @@ import (
"os"
"path/filepath"
"runtime"
"strconv"
"time"

"github.com/docker/cli/cli"
Expand DownExpand Up@@ -133,6 +134,20 @@ func (cli *DockerCli) ContentTrustEnabled() bool {
return cli.contentTrust
}

// BuildKitEnabled returns whether buildkit is enabled either through a daemon setting
// or otherwise the client-side DOCKER_BUILDKIT environment variable
func BuildKitEnabled(si ServerInfo) (bool, error) {
buildkitEnabled := si.BuildkitVersion == types.BuilderBuildKit
if buildkitEnv := os.Getenv("DOCKER_BUILDKIT"); buildkitEnv != "" {
var err error
buildkitEnabled, err = strconv.ParseBool(buildkitEnv)
if err != nil {
return false, errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value")
}
}
return buildkitEnabled, nil
}

// ManifestStore returns a store for local manifests
func (cli *DockerCli) ManifestStore() manifeststore.Store {
// TODO: support override default location from config file
Expand Down
24 changes: 12 additions & 12 deletions cli/command/image/build.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,7 +13,6 @@ import (
"path/filepath"
"regexp"
"runtime"
"strconv"
"strings"

"github.com/docker/cli/cli"
Expand DownExpand Up@@ -137,6 +136,8 @@ func NewBuildCommand(dockerCli command.Cli) *cobra.Command {
flags.BoolVar(&options.pull, "pull", false, "Always attempt to pull a newer version of the image")
flags.StringSliceVar(&options.cacheFrom, "cache-from", []string{}, "Images to consider as cache sources")
flags.BoolVar(&options.compress, "compress", false, "Compress the build context using gzip")
flags.SetAnnotation("compress", "no-buildkit", nil)

flags.StringSliceVar(&options.securityOpt, "security-opt", []string{}, "Security options")
flags.StringVar(&options.networkMode, "network", "default", "Set the networking mode for the RUN instructions during build")
flags.SetAnnotation("network", "version", []string{"1.25"})
Expand All@@ -154,14 +155,18 @@ func NewBuildCommand(dockerCli command.Cli) *cobra.Command {
flags.BoolVar(&options.stream, "stream", false, "Stream attaches to server to negotiate build context")
flags.SetAnnotation("stream", "experimental", nil)
flags.SetAnnotation("stream", "version", []string{"1.31"})
flags.SetAnnotation("stream", "no-buildkit", nil)

flags.StringVar(&options.progress, "progress", "auto", "Set type of progress output (only if BuildKit enabled) (auto, plain, tty). Use plain to show container output")
flags.StringVar(&options.progress, "progress", "auto", "Set type of progress output (auto, plain, tty). Use plain to show container output")
flags.SetAnnotation("progress", "buildkit", nil)

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)

This comment was marked as resolved.

Sign in to view


flags.StringArrayVar(&options.ssh, "ssh", []string{}, "SSH agent socket or keys to expose to the build (only if BuildKit enabled) (format: default|<id>[=<socket>|<key>[,<key>]])")
flags.SetAnnotation("ssh", "version", []string{"1.39"})
flags.SetAnnotation("ssh", "buildkit", nil)
return cmd
}

Expand All@@ -183,22 +188,17 @@ func (out *lastProgressOutput) WriteProgress(prog progress.Progress) error {

// nolint: gocyclo
func runBuild(dockerCli command.Cli, options buildOptions) error {
if buildkitEnv := os.Getenv("DOCKER_BUILDKIT"); buildkitEnv != "" {
enableBuildkit, err := strconv.ParseBool(buildkitEnv)
if err != nil {
return errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value")
}
if enableBuildkit {
return runBuildBuildKit(dockerCli, options)
}
} else if dockerCli.ServerInfo().BuildkitVersion == types.BuilderBuildKit {
buildkitEnabled, err := command.BuildKitEnabled(dockerCli.ServerInfo())
if err != nil {
return err
}
if buildkitEnabled {
return runBuildBuildKit(dockerCli, options)
}

var (
buildCtx io.ReadCloser
dockerfileCtx io.ReadCloser
err error
contextDir string
tempDir string
relDockerfile string
Expand Down
18 changes: 15 additions & 3 deletions cmd/docker/docker.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -100,8 +100,10 @@ func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.
ccmd.Println(err)
return
}

hideUnsupportedFeatures(ccmd, dockerCli)
if err := hideUnsupportedFeatures(ccmd, dockerCli); err != nil {
ccmd.Println(err)
return
}
defaultHelpFunc(ccmd, args)
})
}
Expand DownExpand Up@@ -235,15 +237,21 @@ func hideFeatureSubCommand(subcmd *cobra.Command, hasFeature bool, annotation st
}
}

func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) {
func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
clientVersion := details.Client().ClientVersion()
osType := details.ServerInfo().OSType
hasExperimental := details.ServerInfo().HasExperimental
hasExperimentalCLI := details.ClientInfo().HasExperimental
hasBuildKit, err := command.BuildKitEnabled(details.ServerInfo())
if err != nil {
return err
}

cmd.Flags().VisitAll(func(f *pflag.Flag) {
hideFeatureFlag(f, hasExperimental, "experimental")
hideFeatureFlag(f, hasExperimentalCLI, "experimentalCLI")
hideFeatureFlag(f, hasBuildKit, "buildkit")
Comment thread
tiborvass marked this conversation as resolved.
hideFeatureFlag(f, !hasBuildKit, "no-buildkit")
// hide flags not supported by the server
if !isOSTypeSupported(f, osType) || !isVersionSupported(f, clientVersion) {
f.Hidden = true
Expand All@@ -259,6 +267,8 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) {
for _, subcmd := range cmd.Commands() {
hideFeatureSubCommand(subcmd, hasExperimental, "experimental")
hideFeatureSubCommand(subcmd, hasExperimentalCLI, "experimentalCLI")
hideFeatureSubCommand(subcmd, hasBuildKit, "buildkit")
hideFeatureSubCommand(subcmd, !hasBuildKit, "no-buildkit")
// hide subcommands not supported by the server
if subcmdVersion, ok := subcmd.Annotations["version"]; ok && versions.LessThan(clientVersion, subcmdVersion) {
subcmd.Hidden = true
Expand All@@ -267,6 +277,7 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) {
subcmd.Hidden = true
}
}
return nil
}

// Checks if a command or one of its ancestors is in the list
Expand DownExpand Up@@ -313,6 +324,7 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
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

@AkihiroSudaAkihiroSudaOct 11, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@thaJeztahthaJeztahOct 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

}
})
if len(errs) > 0 {
Expand Down