build: allow setting buildkit outputs #1766
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1766 +/- ##
==========================================
- Coverage 56.32% 56.21% -0.12%
==========================================
Files 307 307
Lines 21177 21225 +48
==========================================
+ Hits 11928 11931 +3
- Misses 8382 8427 +45
Partials 867 867 |
| if len(parts) != 2 { | ||
| return nil, errors.Errorf("invalid value %s", field) | ||
| } | ||
| key := strings.ToLower(parts[0]) |
There was a problem hiding this comment.
@thaJeztah doesn't like ToLower. He wants it strict.
There was a problem hiding this comment.
too bad for @thaJeztah then
cli/cli/command/image/build_buildkit.go
Line 397 in 774d78f
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| } | ||
| if len(fields) == 1 && fields[0] == s { | ||
| outs = append(outs, types.ImageBuildOutput{ | ||
| Type: "local", |
There was a problem hiding this comment.
off-topic: do we want to port over this default convention to buildctl?
There was a problem hiding this comment.
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.
| if len(parts) != 2 { | ||
| return nil, errors.Errorf("invalid value %s", field) | ||
| } | ||
| key := strings.ToLower(parts[0]) |
There was a problem hiding this comment.
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
| for _, field := range fields { | ||
| parts := strings.SplitN(field, "=", 2) | ||
| if len(parts) != 2 { | ||
| return nil, errors.Errorf("invalid value %s", field) |
There was a problem hiding this comment.
We don't have (or will have) options that don't require a value? Otherwise we may defer this to the daemon, and use
Lines 40 to 53 in 9e71207
There was a problem hiding this comment.
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.
| @@ -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)") | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
-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
Signed-off-by: Tonis Tiigi <[email protected]>
Introduce
-o/--outputtodocker buildallowing configuration of how the build output should be handled on buildkit mode.Currently only supports
localexporter.-o type=local,dest=outpath-o ./path/to/out- non csv value defaults to output pathWould be good to add
tarballexporter as well before we ship this because local exporter can not preserve file uid/gid. Other exporters that are supported by buildkit need to wait for the containerd support to land in moby first.@tiborvass @AkihiroSuda
Signed-off-by: Tonis Tiigi [email protected]