From b1c9ff5f32bcbf795bc060004ae26cf94ed0de86 Mon Sep 17 00:00:00 2001 From: iTrooz Date: Tue, 18 Nov 2025 13:18:15 +0100 Subject: [PATCH] feat(build): print error on build flag --output=type=something Signed-off-by: iTrooz --- define/types.go | 1 + imagebuildah/stage_executor.go | 7 +- internal/parse/build_output.go | 102 ++++++++++++++++++++++++++++ internal/parse/build_output_test.go | 47 +++++++++++++ internal/util/util.go | 8 +-- pkg/cli/build.go | 5 +- pkg/parse/parse.go | 1 + tests/bud.bats | 7 ++ 8 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 internal/parse/build_output.go create mode 100644 internal/parse/build_output_test.go diff --git a/define/types.go b/define/types.go index a1f3946f4..9c7da4cd8 100644 --- a/define/types.go +++ b/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 diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index 876e558f5..05d9731e4 100644 --- a/imagebuildah/stage_executor.go +++ b/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 diff --git a/internal/parse/build_output.go b/internal/parse/build_output.go new file mode 100644 index 000000000..ca59ba921 --- /dev/null +++ b/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 +} diff --git a/internal/parse/build_output_test.go b/internal/parse/build_output_test.go new file mode 100644 index 000000000..a5e637107 --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/util/util.go b/internal/util/util.go index 9094a7023..ef6ebeb4a 100644 --- a/internal/util/util.go +++ b/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) } diff --git a/pkg/cli/build.go b/pkg/cli/build.go index 2b7c2ff7c..c4e205af7 100644 --- a/pkg/cli/build.go +++ b/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 } } diff --git a/pkg/parse/parse.go b/pkg/parse/parse.go index aa2a9672b..c1e8cd9ea 100644 --- a/pkg/parse/parse.go +++ b/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 diff --git a/tests/bud.bats b/tests/bud.bats index 61d8ec893..ddf388efc 100644 --- a/tests/bud.bats +++ b/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 .