feat(build): print error on build flag --output=type=something

Signed-off-by: iTrooz <hey@itrooz.fr>

define/types.go

@@ -110,6 +110,7 @@ type Secret struct {
 }
 
 // BuildOutputOptions contains the the outcome of parsing the value of a build --output flag
+// Deprecated: This structure is now internal
 type BuildOutputOption struct {
 	Path     string // Only valid if !IsStdout
 	IsDir    bool

imagebuildah/stage_executor.go

@@ -20,6 +20,7 @@ import (
 	buildahdocker "github.com/containers/buildah/docker"
 	"github.com/containers/buildah/internal"
 	"github.com/containers/buildah/internal/metadata"
+	internalParse "github.com/containers/buildah/internal/parse"
 	"github.com/containers/buildah/internal/sanitize"
 	"github.com/containers/buildah/internal/tmpdir"
 	internalUtil "github.com/containers/buildah/internal/util"
@@ -1310,11 +1311,11 @@ func (s *stageExecutor) execute(ctx context.Context, base string) (imgID string,
 	}
 
 	// Parse and populate buildOutputOption if needed
-	var buildOutputOptions []define.BuildOutputOption
+	var buildOutputOptions []internalParse.BuildOutputOption
 	if lastStage && len(s.executor.buildOutputs) > 0 {
 		for _, buildOutput := range s.executor.buildOutputs {
 			logrus.Debugf("generating custom build output with options %q", buildOutput)
-			buildOutputOption, err := parse.GetBuildOutput(buildOutput)
+			buildOutputOption, err := internalParse.GetBuildOutput(buildOutput)
 			if err != nil {
 				return "", nil, false, fmt.Errorf("failed to parse build output %q: %w", buildOutput, err)
 			}
@@ -2621,7 +2622,7 @@ func (s *stageExecutor) commit(ctx context.Context, createdBy string, emptyLayer
 	return results.ImageID, results, nil
 }
 
-func (s *stageExecutor) generateBuildOutput(buildOutputOpts define.BuildOutputOption) error {
+func (s *stageExecutor) generateBuildOutput(buildOutputOpts internalParse.BuildOutputOption) error {
 	forceTimestamp := s.executor.timestamp
 	if s.executor.sourceDateEpoch != nil {
 		forceTimestamp = s.executor.sourceDateEpoch

internal/parse/build_output.go

@@ -0,0 +1,102 @@
+package parse
+
+import (
+	"fmt"
+	"strings"
+)
+
+type BuildOutputType int
+
+const (
+	BuildOutputStdout   BuildOutputType = 0 // stream tar to stdout
+	BuildOutputLocalDir BuildOutputType = 1
+	BuildOutputTar      BuildOutputType = 2
+)
+
+// BuildOutputOptions contains the the outcome of parsing the value of a build --output flag
+type BuildOutputOption struct {
+	Type BuildOutputType
+	Path string // Only valid if Type is local dir or tar
+}
+
+// GetBuildOutput is responsible for parsing custom build output argument i.e `build --output` flag.
+// Takes `buildOutput` as string and returns BuildOutputOption
+func GetBuildOutput(buildOutput string) (BuildOutputOption, error) {
+	// Support simple values, in the form --output ./mydir
+	if !strings.Contains(buildOutput, ",") && !strings.Contains(buildOutput, "=") {
+		if buildOutput == "-" {
+			// Feature parity with buildkit, output tar to stdout
+			// Read more here: https://docs.docker.com/engine/reference/commandline/build/#custom-build-outputs
+			return BuildOutputOption{
+				Type: BuildOutputStdout,
+				Path: "",
+			}, nil
+		}
+
+		return BuildOutputOption{
+			Type: BuildOutputLocalDir,
+			Path: buildOutput,
+		}, nil
+	}
+
+	// Support complex values, in the form --output type=local,dest=./mydir
+	var typeSelected BuildOutputType = -1
+	pathSelected := ""
+	for option := range strings.SplitSeq(buildOutput, ",") {
+		key, value, found := strings.Cut(option, "=")
+		if !found {
+			return BuildOutputOption{}, fmt.Errorf("invalid build output options %q, expected format key=value", buildOutput)
+		}
+		switch key {
+		case "type":
+			if typeSelected != -1 {
+				return BuildOutputOption{}, fmt.Errorf("duplicate %q not supported", key)
+			}
+			switch value {
+			case "local":
+				typeSelected = BuildOutputLocalDir
+			case "tar":
+				typeSelected = BuildOutputTar
+			default:
+				return BuildOutputOption{}, fmt.Errorf("invalid type %q selected for build output options %q", value, buildOutput)
+			}
+		case "dest":
+			if pathSelected != "" {
+				return BuildOutputOption{}, fmt.Errorf("duplicate %q not supported", key)
+			}
+			pathSelected = value
+		default:
+			return BuildOutputOption{}, fmt.Errorf("unrecognized key %q in build output option: %q", key, buildOutput)
+		}
+	}
+
+	// Validate there is a type
+	if typeSelected == -1 {
+		return BuildOutputOption{}, fmt.Errorf("missing required key %q in build output option: %q", "type", buildOutput)
+	}
+
+	// Validate path
+	if typeSelected == BuildOutputLocalDir || typeSelected == BuildOutputTar {
+		if pathSelected == "" {
+			return BuildOutputOption{}, fmt.Errorf("missing required key %q in build output option: %q", "dest", buildOutput)
+		}
+	} else {
+		// Clear path when not needed by type
+		pathSelected = ""
+	}
+
+	// Handle redirecting stdout for tar output
+	if pathSelected == "-" {
+		if typeSelected == BuildOutputTar {
+			typeSelected = BuildOutputStdout
+			pathSelected = ""
+		} else {
+			return BuildOutputOption{}, fmt.Errorf(`invalid build output option %q, only "type=tar" can be used with "dest=-"`, buildOutput)
+		}
+	}
+
+	return BuildOutputOption{
+		Type: typeSelected,
+		Path: pathSelected,
+	}, nil
+}

internal/parse/build_output_test.go

@@ -0,0 +1,47 @@
+package parse
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGetBuildOutput(t *testing.T) {
+	testCases := []struct {
+		description string
+		input       string
+		output      BuildOutputOption
+	}{
+		{
+			description: "hyphen",
+			input:       "-",
+			output: BuildOutputOption{
+				Type: BuildOutputStdout,
+			},
+		},
+		{
+			description: "just-a-path",
+			input:       "/tmp",
+			output: BuildOutputOption{
+				Type: BuildOutputLocalDir,
+				Path: "/tmp",
+			},
+		},
+		{
+			description: "normal-path",
+			input:       "type=local,dest=/tmp",
+			output: BuildOutputOption{
+				Type: BuildOutputLocalDir,
+				Path: "/tmp",
+			},
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			result, err := GetBuildOutput(testCase.input)
+			require.NoErrorf(t, err, "expected to be able to parse %q", testCase.input)
+			assert.Equal(t, testCase.output, result)
+		})
+	}
+}

internal/util/util.go

@@ -6,7 +6,7 @@ import (
 	"os"
 	"path/filepath"
 
-	"github.com/containers/buildah/define"
+	"github.com/containers/buildah/internal/parse"
 	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"go.podman.io/common/libimage"
 	lplatform "go.podman.io/common/libimage/platform"
@@ -51,14 +51,14 @@ func NormalizePlatform(platform v1.Platform) v1.Platform {
 }
 
 // ExportFromReader reads bytes from given reader and exports to external tar, directory or stdout.
-func ExportFromReader(input io.Reader, opts define.BuildOutputOption) error {
+func ExportFromReader(input io.Reader, opts parse.BuildOutputOption) error {
 	var err error
 	if !filepath.IsAbs(opts.Path) {
 		if opts.Path, err = filepath.Abs(opts.Path); err != nil {
 			return err
 		}
 	}
-	if opts.IsDir {
+	if opts.Type == parse.BuildOutputLocalDir {
 		// In order to keep this feature as close as possible to
 		// buildkit it was decided to preserve ownership when
 		// invoked as root since caller already has access to artifacts
@@ -80,7 +80,7 @@ func ExportFromReader(input io.Reader, opts define.BuildOutputOption) error {
 		}
 	} else {
 		outFile := os.Stdout
-		if !opts.IsStdout {
+		if opts.Type != parse.BuildOutputStdout {
 			if outFile, err = os.Create(opts.Path); err != nil {
 				return fmt.Errorf("failed while creating destination tar at %q: %w", opts.Path, err)
 			}

pkg/cli/build.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"github.com/containers/buildah/define"
+	internalParse "github.com/containers/buildah/internal/parse"
 	"github.com/containers/buildah/pkg/parse"
 	"github.com/containers/buildah/pkg/util"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -278,11 +279,11 @@ func GenBuildOptions(c *cobra.Command, inputArgs []string, iopts BuildOptions) (
 		for _, buildOutput := range iopts.BuildOutputs {
 			// if any of these go to stdout, we need to avoid
 			// interspersing our random output in with it
-			buildOption, err := parse.GetBuildOutput(buildOutput)
+			buildOption, err := internalParse.GetBuildOutput(buildOutput)
 			if err != nil {
 				return options, nil, nil, err
 			}
-			if buildOption.IsStdout {
+			if buildOption.Type == internalParse.BuildOutputStdout {
 				iopts.Quiet = true
 			}
 		}

pkg/parse/parse.go

@@ -734,6 +734,7 @@ func AuthConfig(creds string) (*types.DockerAuthConfig, error) {
 
 // GetBuildOutput is responsible for parsing custom build output argument i.e `build --output` flag.
 // Takes `buildOutput` as string and returns BuildOutputOption
+// Deprecated: This function is now internal
 func GetBuildOutput(buildOutput string) (define.BuildOutputOption, error) {
 	if buildOutput == "-" {
 		// Feature parity with buildkit, output tar to stdout

tests/bud.bats

@@ -2773,6 +2773,11 @@ _EOF
   # verify tar content
   run tar -tf $mytmpdir/rootfs.tar
   expect_output --substring 'hello'
+
+  # test with long syntax as well
+  buildah build $WITH_POLICY_JSON --output type=tar,dest=- -t test-bud -f $mytmpdir/Containerfile . > $mytmpdir/rootfs2.tar
+  run tar -tf $mytmpdir/rootfs2.tar
+  expect_output --substring 'hello'
 }
 
 @test "build with custom build output and output rootfs to tar with no additional step" {
@@ -2801,6 +2806,8 @@ _EOF
 FROM alpine
 RUN echo 'hello'> hello
 _EOF
+  run_buildah 125 build --output type=tar $WITH_POLICY_JSON -t test-bud -f $mytmpdir/Containerfile .
+  expect_output --substring 'missing required key "dest"'
   run_buildah 125 build --output type=tar, $WITH_POLICY_JSON -t test-bud -f $mytmpdir/Containerfile .
   expect_output --substring 'invalid'
   run_buildah 125 build --output type=wrong,dest=hello $WITH_POLICY_JSON -t test-bud -f $mytmpdir/Containerfile .