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

build: allow setting buildkit outputs #1766

Merged
merged 1 commit into from
Mar 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 53 additions & 0 deletions cli/command/image/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"bufio"
"bytes"
"context"
"encoding/csv"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -73,6 +74,7 @@ type buildOptions struct {
untrusted bool
secrets []string
ssh []string
outputs []string
}

// dockerfileFromStdin returns true when the user specified that the Dockerfile
Expand Down Expand Up @@ -176,6 +178,11 @@ func NewBuildCommand(dockerCli command.Cli) *cobra.Command {
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)

flags.StringArrayVarP(&options.outputs, "output", "o", []string{}, "Output destination (format: type=local,dest=path)")
Copy link
Member

Choose a reason for hiding this comment

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

did we decide on the shorthand -o option? (just to be sure; I'm cautious adding shorthands if other options may be added that also start with the same letter)

Copy link
Member Author

Choose a reason for hiding this comment

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

-o is a common convention in many other build tools for output. I think it would be weird to ever use it for anything else in docker build

flags.SetAnnotation("output", "version", []string{"1.40"})
flags.SetAnnotation("output", "buildkit", nil)

return cmd
}

Expand Down Expand Up @@ -643,3 +650,49 @@ func imageBuildOptions(dockerCli command.Cli, options buildOptions) types.ImageB
Platform: options.platform,
}
}

func parseOutputs(inp []string) ([]types.ImageBuildOutput, error) {
var outs []types.ImageBuildOutput
if len(inp) == 0 {
return nil, nil
}
for _, s := range inp {
csvReader := csv.NewReader(strings.NewReader(s))
fields, err := csvReader.Read()
if err != nil {
return nil, err
}
if len(fields) == 1 && fields[0] == s {
outs = append(outs, types.ImageBuildOutput{
Type: "local",
Copy link
Collaborator

Choose a reason for hiding this comment

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

off-topic: do we want to port over this default convention to buildctl?

Copy link
Member Author

Choose a reason for hiding this comment

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

buildctl doesn't necessarily need to be the most user-friendly but expose all API capabilities. If this becomes common I see no reason not to do it. But I wouldn't want to go over the other flags(eg. cache import/export) of buildctl trying to find similar patterns atm.

Attrs: map[string]string{
"dest": s,
},
})
continue
}

out := types.ImageBuildOutput{
Attrs: map[string]string{},
}
for _, field := range fields {
parts := strings.SplitN(field, "=", 2)
if len(parts) != 2 {
return nil, errors.Errorf("invalid value %s", field)
Copy link
Member

Choose a reason for hiding this comment

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

We don't have (or will have) options that don't require a value? Otherwise we may defer this to the daemon, and use

cli/opts/parse.go

Lines 40 to 53 in 9e71207

// ConvertKVStringsToMap converts ["key=value"] to {"key":"value"}
func ConvertKVStringsToMap(values []string) map[string]string {
result := make(map[string]string, len(values))
for _, value := range values {
kv := strings.SplitN(value, "=", 2)
if len(kv) == 1 {
result[kv[0]] = ""
} else {
result[kv[0]] = kv[1]
}
}
return result
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The API already accepts these values in a structured form and correctly formatted. We also need to parse out the values that are handled on the client side.

}
key := strings.ToLower(parts[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

@thaJeztah doesn't like ToLower. He wants it strict.

Copy link
Member Author

Choose a reason for hiding this comment

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

too bad for @thaJeztah then

key := strings.ToLower(parts[0])

Copy link
Member

Choose a reason for hiding this comment

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

Yes, ToNIs, noT reAlLy a FaN of toLOWER heRE

Better keep it strict, and we can loosen up if we really would see a need

Copy link
Member Author

Choose a reason for hiding this comment

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

What I referred above is that everything else already is case insensitive. This includes csv for build secrets, swarm secrets, swarm config, mounts, network and ports. CSV in Dockerfile flags in daemon side is also case insensitive.

Btw, I'm not in favor of documenting it.

value := parts[1]
switch key {
case "type":
out.Type = value
default:
out.Attrs[key] = value
}
}
if out.Type == "" {
return nil, errors.Errorf("type is required for output")
}
outs = append(outs, out)
}
return outs, nil
}
18 changes: 18 additions & 0 deletions cli/command/image/build_buildkit.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,23 @@ func runBuildBuildKit(dockerCli command.Cli, options buildOptions) error {
defer os.RemoveAll(dockerfileDir)
}

outputs, err := parseOutputs(options.outputs)
if err != nil {
return errors.Wrapf(err, "failed to parse outputs")
}

for _, out := range outputs {
if out.Type == "local" {
// dest is handled on client side for local exporter
outDir, ok := out.Attrs["dest"]
if !ok {
return errors.Errorf("dest is required for local output")
}
delete(out.Attrs, "dest")
tonistiigi marked this conversation as resolved.
Show resolved Hide resolved
s.Allow(filesync.NewFSSyncTargetDir(outDir))
}
}

if dockerfileDir != "" {
s.Allow(filesync.NewFSSyncProvider([]filesync.SyncedDir{
{
Expand Down Expand Up @@ -182,6 +199,7 @@ func runBuildBuildKit(dockerCli command.Cli, options buildOptions) error {
buildOptions.RemoteContext = remote
buildOptions.SessionID = s.ID()
buildOptions.BuildID = buildID
buildOptions.Outputs = outputs
return doBuild(ctx, eg, dockerCli, options, buildOptions)
})

Expand Down