From 3f442309fc134320d3770af7b90f892336865249 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 14 Nov 2025 15:22:40 +0100 Subject: [PATCH 1/3] main: embed manifestgen.options into manifestOptions When we generate the osbuild manifest we need to take a bunch of commandline options into account. These are collected and passed via `manifestOptions` in the CLI. There is a (small) overlap with options that are then "converted" to options that need to be passed to `manifestgen` via `manifestgen.Options`. Previously there was manual code for this but its slightly nicer to just embedd the `manifestgen.Options` into the more general `manifestOptions` of the CLI. This avoid some boilerplate code and might serve as a useful pattern in other places. So this commit does that now (even though the wins are not that big). --- cmd/image-builder/main.go | 20 ++++++++++++++------ cmd/image-builder/manifest.go | 21 +++++---------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/cmd/image-builder/main.go b/cmd/image-builder/main.go index 166e67d8..628ba36b 100644 --- a/cmd/image-builder/main.go +++ b/cmd/image-builder/main.go @@ -21,6 +21,7 @@ import ( "github.com/osbuild/images/pkg/customizations/subscription" "github.com/osbuild/images/pkg/distro/bootc" "github.com/osbuild/images/pkg/imagefilter" + "github.com/osbuild/images/pkg/manifestgen" "github.com/osbuild/images/pkg/osbuild" "github.com/osbuild/images/pkg/ostree" @@ -138,6 +139,9 @@ type cmdManifestWrapperOptions struct { useBootstrapIfNeeded bool } +// used in tests +var manifestgenDepsolver manifestgen.DepsolveFunc + func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []string, w io.Writer, wd io.Writer, wrapperOpts *cmdManifestWrapperOptions) (*imagefilter.Result, error) { if wrapperOpts == nil { wrapperOpts = &cmdManifestWrapperOptions{} @@ -304,27 +308,31 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st } opts := &manifestOptions{ + ManifestgenOptions: manifestgen.Options{ + Cachedir: rpmmdCacheDir, + CustomSeed: customSeed, + RpmDownloader: rpmDownloader, + DepsolveWarningsOutput: wd, + Depsolve: manifestgenDepsolver, + }, OutputDir: outputDir, OutputFilename: outputFilename, BlueprintPath: blueprintPath, Ostree: ostreeImgOpts, BootcRef: bootcRef, BootcInstallerPayloadRef: bootcInstallerPayloadRef, - RpmDownloader: rpmDownloader, WithSBOM: withSBOM, IgnoreWarnings: ignoreWarnings, - CustomSeed: customSeed, Subscription: subscription, - RpmmdCacheDir: rpmmdCacheDir, ForceRepos: forceRepos, } - opts.UseBootstrapContainer = wrapperOpts.useBootstrapIfNeeded && (img.ImgType.Arch().Name() != arch.Current().String()) - if opts.UseBootstrapContainer { + opts.ManifestgenOptions.UseBootstrapContainer = wrapperOpts.useBootstrapIfNeeded && (img.ImgType.Arch().Name() != arch.Current().String()) + if opts.ManifestgenOptions.UseBootstrapContainer { fmt.Fprintf(os.Stderr, "WARNING: using experimental cross-architecture building to build %q\n", img.ImgType.Arch().Name()) } - err = generateManifest(dataDir, extraRepos, img, w, wd, opts) + err = generateManifest(dataDir, extraRepos, img, w, opts) return img, err } diff --git a/cmd/image-builder/manifest.go b/cmd/image-builder/manifest.go index 249ae620..229b44d4 100644 --- a/cmd/image-builder/manifest.go +++ b/cmd/image-builder/manifest.go @@ -20,6 +20,8 @@ import ( ) type manifestOptions struct { + ManifestgenOptions manifestgen.Options + OutputDir string OutputFilename string BlueprintPath string @@ -30,11 +32,8 @@ type manifestOptions struct { RpmDownloader osbuild.RpmDownloader WithSBOM bool IgnoreWarnings bool - CustomSeed *int64 - RpmmdCacheDir string - ForceRepos []string - UseBootstrapContainer bool + ForceRepos []string } func sbomWriter(outputDir, filename string, content io.Reader) error { @@ -55,23 +54,13 @@ func sbomWriter(outputDir, filename string, content io.Reader) error { return f.Sync() } -// used in tests -var manifestgenDepsolver manifestgen.DepsolveFunc - // XXX: just return []byte instead of using output writer -func generateManifest(dataDir string, extraRepos []string, img *imagefilter.Result, output io.Writer, depsolveWarningsOutput io.Writer, opts *manifestOptions) error { +func generateManifest(dataDir string, extraRepos []string, img *imagefilter.Result, output io.Writer, opts *manifestOptions) error { repos, err := newRepoRegistry(dataDir, extraRepos) if err != nil { return err } - manifestGenOpts := &manifestgen.Options{ - DepsolveWarningsOutput: depsolveWarningsOutput, - RpmDownloader: opts.RpmDownloader, - UseBootstrapContainer: opts.UseBootstrapContainer, - CustomSeed: opts.CustomSeed, - Depsolve: manifestgenDepsolver, - Cachedir: opts.RpmmdCacheDir, - } + manifestGenOpts := &opts.ManifestgenOptions if opts.WithSBOM { outputDir := basenameFor(img, opts.OutputDir) manifestGenOpts.SBOMWriter = func(filename string, content io.Reader, docType sbom.StandardType) error { From 283f40711a03f1166ad89f86c26fd21fb72be063 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 17 Dec 2025 10:42:03 +0100 Subject: [PATCH 2/3] main: always use a bootstrap buildroot (but support disabling) We recently had two issues where creating our buildroot was broken in certain situations because the local rpm would not be compatible with the requirements of the package that got installed in the buildroot: https://issues.redhat.com/browse/RHEL-128741 https://github.com/osbuild/image-builder-cli/issues/413 The issue here is that the local rpm is something we do not control but we need a way to "bootstrap" our buildroot (once we have a buildroot we use that for everything else). This commit enables the "bootstrap" container feature of the images library by default to avoid this dependency. This means that we "podman pull" a minimal container (e.g. ubi for rhel) that contains python3 and rpm to then use it to install our real buildroot. Note that this enables it all the time even if the host distribution and target distribution match. The reason is rebuildability - ie. the same manifest should always produce the same result and in the general case we do not know if e.g. a manifest that was part of `image-builder build --with-manifest` is used somewhere else again. For restricted environment where pulling a container is a problem or for situations where its known that the host rpm is fine we provide: `--without-bootstrap-container` to disable this function. --- cmd/image-builder/main.go | 26 +++++++----------- cmd/image-builder/main_test.go | 48 +++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 19 deletions(-) diff --git a/cmd/image-builder/main.go b/cmd/image-builder/main.go index 628ba36b..70b85b02 100644 --- a/cmd/image-builder/main.go +++ b/cmd/image-builder/main.go @@ -135,17 +135,10 @@ func subscriptionImageOptions(cmd *cobra.Command) (*subscription.ImageOptions, e return regs.Redhat.Subscription, nil } -type cmdManifestWrapperOptions struct { - useBootstrapIfNeeded bool -} - // used in tests var manifestgenDepsolver manifestgen.DepsolveFunc -func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []string, w io.Writer, wd io.Writer, wrapperOpts *cmdManifestWrapperOptions) (*imagefilter.Result, error) { - if wrapperOpts == nil { - wrapperOpts = &cmdManifestWrapperOptions{} - } +func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []string, w io.Writer, wd io.Writer) (*imagefilter.Result, error) { dataDir, err := cmd.Flags().GetString("force-data-dir") if err != nil { return nil, err @@ -217,6 +210,10 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st if err != nil { return nil, err } + disableBootstrapContainer, err := cmd.Flags().GetBool("without-bootstrap-container") + if err != nil { + return nil, err + } bootcRef, err := cmd.Flags().GetString("bootc-ref") if err != nil { return nil, err @@ -314,6 +311,7 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st RpmDownloader: rpmDownloader, DepsolveWarningsOutput: wd, Depsolve: manifestgenDepsolver, + UseBootstrapContainer: !disableBootstrapContainer, }, OutputDir: outputDir, OutputFilename: outputFilename, @@ -327,8 +325,7 @@ func cmdManifestWrapper(pbar progress.ProgressBar, cmd *cobra.Command, args []st ForceRepos: forceRepos, } - opts.ManifestgenOptions.UseBootstrapContainer = wrapperOpts.useBootstrapIfNeeded && (img.ImgType.Arch().Name() != arch.Current().String()) - if opts.ManifestgenOptions.UseBootstrapContainer { + if img.ImgType.Arch().Name() != arch.Current().String() { fmt.Fprintf(os.Stderr, "WARNING: using experimental cross-architecture building to build %q\n", img.ImgType.Arch().Name()) } @@ -341,7 +338,7 @@ func cmdManifest(cmd *cobra.Command, args []string) error { if err != nil { return err } - _, err = cmdManifestWrapper(pbar, cmd, args, osStdout, io.Discard, nil) + _, err = cmdManifestWrapper(pbar, cmd, args, osStdout, io.Discard) return err } @@ -409,13 +406,9 @@ func cmdBuild(cmd *cobra.Command, args []string) error { }() var mf bytes.Buffer - opts := &cmdManifestWrapperOptions{ - useBootstrapIfNeeded: true, - } - // We discard any warnings from the depsolver until we figure out a better // idea (likely in manifestgen) - res, err := cmdManifestWrapper(pbar, cmd, args, &mf, io.Discard, opts) + res, err := cmdManifestWrapper(pbar, cmd, args, &mf, io.Discard) if err != nil { return err } @@ -584,6 +577,7 @@ operating systems like Fedora, CentOS and RHEL with easy customizations support. manifestCmd.Flags().Bool("ignore-warnings", false, `ignore warnings during manifest generation`) manifestCmd.Flags().String("registrations", "", `filename of a registrations file with e.g. subscription details`) manifestCmd.Flags().String("rpmmd-cache", "", `osbuild directory to cache rpm metadata`) + manifestCmd.Flags().Bool("without-bootstrap-container", false, `disable using a "bootstrap" container to generate the initial buildroot`) rootCmd.AddCommand(manifestCmd) uploadCmd := &cobra.Command{ diff --git a/cmd/image-builder/main_test.go b/cmd/image-builder/main_test.go index 6ce43916..bab340da 100644 --- a/cmd/image-builder/main_test.go +++ b/cmd/image-builder/main_test.go @@ -899,18 +899,60 @@ func TestBuildCrossArchSmoke(t *testing.T) { assert.NoError(t, err) pipelines, err := manifesttest.PipelineNamesFrom(manifest) assert.NoError(t, err) - crossArchPipeline := "bootstrap-buildroot" + bootstrapPipeline := "bootstrap-buildroot" crossArchWarning := `WARNING: using experimental cross-architecture building to build "aarch64"` + // the bootstrap pipeline is the default for all builds + assert.Contains(t, pipelines, bootstrapPipeline) if withCrossArch { - assert.Contains(t, pipelines, crossArchPipeline) assert.Contains(t, stderr, crossArchWarning) } else { - assert.NotContains(t, pipelines, crossArchPipeline) assert.NotContains(t, stderr, crossArchWarning) } } } +func TestManifestBootstrapContainer(t *testing.T) { + if testing.Short() { + t.Skip("manifest generation takes a while") + } + if !hasDepsolveDnf() { + t.Skip("no osbuild-depsolve-dnf binary found") + } + + restore := main.MockNewRepoRegistry(testrepos.New) + defer restore() + + for _, disableBootstrapContainer := range []bool{false, true} { + cmd := []string{ + "manifest", + "tar", + "--distro", "centos-10", + } + if disableBootstrapContainer { + cmd = append(cmd, "--without-bootstrap-container") + } + restore = main.MockOsArgs(cmd) + defer restore() + + var fakeStdout bytes.Buffer + restore = main.MockOsStdout(&fakeStdout) + defer restore() + + // XXX: capture stderr here too to ensure no cross build warning + err := main.Run() + assert.NoError(t, err) + + pipelines, err := manifesttest.PipelineNamesFrom(fakeStdout.Bytes()) + assert.NoError(t, err) + bootstrapPipeline := "bootstrap-buildroot" + if disableBootstrapContainer { + assert.NotContains(t, pipelines, bootstrapPipeline) + } else { + assert.Contains(t, pipelines, bootstrapPipeline) + } + } +} + func TestBuildIntegrationOutputFilename(t *testing.T) { if testing.Short() { t.Skip("manifest generation takes a while") From ac1e05fd195b0f03c963709d19c76119a0593766 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 17 Dec 2025 11:46:18 +0100 Subject: [PATCH 3/3] main: unif/refactor std{out,err} test handling For historic reasons we have two way to test the std{out,err} output. One is to just monkey patch `var osStdout = os.Stdout` and then replace it in the tests, the other to have a helper that works like a `with` in python that replaces the stdout/stderr dynamically, e.g.: ```go stdout, stderr := testutil.CaptureStdio(t, func() { err = main.Run() require.NoError(t, err) }) ``` The trouble is that they are not quite compatible. In places where we use the monkey patches `osStdout` the testutil.CaptureStdio() will not work as it (temporarely) replaces the "real" `os.Stdout` (and vice-versa). So this commit unifies all tests to use testutil.CaptureStdio() which feels slightly nicer than the other approach in the sense that one does not need to remember to use `osStdout` in the code. Its mostly mechanical. --- cmd/image-builder/bib_cloud.go | 4 +- cmd/image-builder/distro.go | 3 +- cmd/image-builder/distro_test.go | 16 +-- cmd/image-builder/export_test.go | 17 --- cmd/image-builder/list.go | 4 +- cmd/image-builder/main.go | 14 +- cmd/image-builder/main_test.go | 235 ++++++++++++------------------- cmd/image-builder/upload.go | 8 +- cmd/image-builder/upload_test.go | 39 ++--- 9 files changed, 123 insertions(+), 217 deletions(-) diff --git a/cmd/image-builder/bib_cloud.go b/cmd/image-builder/bib_cloud.go index 3943828f..cac066f6 100644 --- a/cmd/image-builder/bib_cloud.go +++ b/cmd/image-builder/bib_cloud.go @@ -42,11 +42,11 @@ func bibUpload(uploader cloud.Uploader, path string, flags *pflag.FlagSet) error size = st.Size() pbar.SetTotal(size) pbar.Set(pb.Bytes, true) - pbar.SetWriter(osStdout) + pbar.SetWriter(os.Stdout) r = pbar.NewProxyReader(file) pbar.Start() defer pbar.Finish() } - return uploader.UploadAndRegister(r, uint64(size), osStderr) + return uploader.UploadAndRegister(r, uint64(size), os.Stderr) } diff --git a/cmd/image-builder/distro.go b/cmd/image-builder/distro.go index 7e66c4ad..a39e5e0c 100644 --- a/cmd/image-builder/distro.go +++ b/cmd/image-builder/distro.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "os" "github.com/osbuild/images/pkg/distro" ) @@ -25,6 +26,6 @@ func findDistro(argDistroName, bpDistroName string) (string, error) { if err != nil { return "", fmt.Errorf("error deriving host distro %w", err) } - fmt.Fprintf(osStderr, "No distro name specified, selecting %q based on host, use --distro to override\n", distroStr) + fmt.Fprintf(os.Stderr, "No distro name specified, selecting %q based on host, use --distro to override\n", distroStr) return distroStr, nil } diff --git a/cmd/image-builder/distro_test.go b/cmd/image-builder/distro_test.go index c35d1c21..2aa8e1c2 100644 --- a/cmd/image-builder/distro_test.go +++ b/cmd/image-builder/distro_test.go @@ -1,12 +1,12 @@ package main_test import ( - "bytes" "testing" "github.com/stretchr/testify/assert" main "github.com/osbuild/image-builder-cli/cmd/image-builder" + "github.com/osbuild/image-builder-cli/internal/testutil" ) func TestFindDistro(t *testing.T) { @@ -32,17 +32,17 @@ func TestFindDistro(t *testing.T) { } func TestFindDistroAutoDetect(t *testing.T) { - var buf bytes.Buffer - restore := main.MockOsStderr(&buf) - defer restore() - - restore = main.MockDistroGetHostDistroName(func() (string, error) { + restore := main.MockDistroGetHostDistroName(func() (string, error) { return "mocked-host-distro", nil }) defer restore() - distro, err := main.FindDistro("", "") + var err error + var distro string + _, stderr := testutil.CaptureStdio(t, func() { + distro, err = main.FindDistro("", "") + }) assert.NoError(t, err) assert.Equal(t, "mocked-host-distro", distro) - assert.Equal(t, "No distro name specified, selecting \"mocked-host-distro\" based on host, use --distro to override\n", buf.String()) + assert.Equal(t, "No distro name specified, selecting \"mocked-host-distro\" based on host, use --distro to override\n", stderr) } diff --git a/cmd/image-builder/export_test.go b/cmd/image-builder/export_test.go index 68364634..08508eda 100644 --- a/cmd/image-builder/export_test.go +++ b/cmd/image-builder/export_test.go @@ -2,7 +2,6 @@ package main import ( "fmt" - "io" "os" "github.com/osbuild/images/pkg/cloud" @@ -31,22 +30,6 @@ func MockOsArgs(new []string) (restore func()) { } } -func MockOsStdout(new io.Writer) (restore func()) { - saved := osStdout - osStdout = new - return func() { - osStdout = saved - } -} - -func MockOsStderr(new io.Writer) (restore func()) { - saved := osStderr - osStderr = new - return func() { - osStderr = saved - } -} - func MockNewRepoRegistry(f func() (*reporegistry.RepoRegistry, error)) (restore func()) { saved := newRepoRegistry newRepoRegistry = func(dataDir string, extraRepos []string) (*reporegistry.RepoRegistry, error) { diff --git a/cmd/image-builder/list.go b/cmd/image-builder/list.go index 9ef362b7..23e26224 100644 --- a/cmd/image-builder/list.go +++ b/cmd/image-builder/list.go @@ -1,6 +1,8 @@ package main import ( + "os" + "github.com/osbuild/images/pkg/imagefilter" ) @@ -19,7 +21,7 @@ func listImages(dataDir string, extraRepos []string, output string, filterExprs if err != nil { return err } - if err := fmter.Output(osStdout, filteredResult); err != nil { + if err := fmter.Output(os.Stdout, filteredResult); err != nil { return err } diff --git a/cmd/image-builder/main.go b/cmd/image-builder/main.go index 70b85b02..e2c77620 100644 --- a/cmd/image-builder/main.go +++ b/cmd/image-builder/main.go @@ -30,11 +30,6 @@ import ( "github.com/osbuild/image-builder-cli/pkg/setup" ) -var ( - osStdout io.Writer = os.Stdout - osStderr io.Writer = os.Stderr -) - // basenameFor returns the basename for directory and filenames // for the given imageType. This can be user overriden via userBasename. func basenameFor(img *imagefilter.Result, userBasename string) string { @@ -338,7 +333,7 @@ func cmdManifest(cmd *cobra.Command, args []string) error { if err != nil { return err } - _, err = cmdManifestWrapper(pbar, cmd, args, osStdout, io.Discard) + _, err = cmdManifestWrapper(pbar, cmd, args, os.Stdout, io.Discard) return err } @@ -446,7 +441,7 @@ func cmdBuild(cmd *cobra.Command, args []string) error { } pbar.Stop() - fmt.Fprintf(osStdout, "Image build successful: %s\n", imagePath) + fmt.Fprintf(os.Stdout, "Image build successful: %s\n", imagePath) if uploader != nil { // XXX: integrate better into the progress, see bib @@ -487,7 +482,7 @@ func cmdDescribeImg(cmd *cobra.Command, args []string) error { return err } - return describeImage(res, osStdout) + return describeImage(res, os.Stdout) } func normalizeRootArgs(_ *pflag.FlagSet, name string) pflag.NormalizedName { @@ -538,9 +533,6 @@ operating systems like Fedora, CentOS and RHEL with easy customizations support. rootCmd.PersistentFlags().String("output-dir", "", `Put output into the specified directory`) rootCmd.PersistentFlags().BoolP("verbose", "v", false, `Switch to verbose mode (more logging on stderr and verbose progress)`) - rootCmd.SetOut(osStdout) - rootCmd.SetErr(osStderr) - listCmd := &cobra.Command{ Use: "list", Short: "List buildable images, use --filter to limit further", diff --git a/cmd/image-builder/main_test.go b/cmd/image-builder/main_test.go index bab340da..e332f978 100644 --- a/cmd/image-builder/main_test.go +++ b/cmd/image-builder/main_test.go @@ -1,7 +1,6 @@ package main_test import ( - "bytes" "crypto/sha256" "encoding/json" "fmt" @@ -37,16 +36,14 @@ func TestListImagesNoArguments(t *testing.T) { restore = main.MockOsArgs(append([]string{"list"}, args...)) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) + stdout, _ := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) // we expect at least this canary - assert.Contains(t, fakeStdout.String(), "rhel-10.0 type:qcow2 arch:x86_64\n") + assert.Contains(t, stdout, "rhel-10.0 type:qcow2 arch:x86_64\n") // output is sorted, i.e. 8.8 comes before 8.10 - assert.Regexp(t, `(?ms)rhel-8.8.*rhel-8.10`, fakeStdout.String()) + assert.Regexp(t, `(?ms)rhel-8.8.*rhel-8.10`, stdout) } } @@ -57,17 +54,15 @@ func TestListImagesNoArgsOutputJSON(t *testing.T) { restore = main.MockOsArgs([]string{"list", "--format=json"}) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) + stdout, _ := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) // smoke test only, we expect valid json and at least the // distro/arch/image_type keys in the json var jo []map[string]interface{} - err = json.Unmarshal(fakeStdout.Bytes(), &jo) + err := json.Unmarshal([]byte(stdout), &jo) assert.NoError(t, err) res := jo[0] for _, key := range []string{"distro", "arch", "image_type"} { @@ -82,30 +77,26 @@ func TestListImagesFilteringSmoke(t *testing.T) { restore = main.MockOsArgs([]string{"list", "--filter=centos*"}) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) + stdout, _ := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) // we have centos - assert.Contains(t, fakeStdout.String(), "centos-9 type:qcow2 arch:x86_64\n") + assert.Contains(t, stdout, "centos-9 type:qcow2 arch:x86_64\n") // but not rhel - assert.NotContains(t, fakeStdout.String(), "rhel") + assert.NotContains(t, stdout, "rhel") } func TestBadCmdErrorsNoExtraCobraNoise(t *testing.T) { - var fakeStderr bytes.Buffer - restore := main.MockOsStderr(&fakeStderr) + restore := main.MockOsArgs([]string{"bad-command"}) defer restore() - restore = main.MockOsArgs([]string{"bad-command"}) - defer restore() - - err := main.Run() - assert.EqualError(t, err, `unknown command "bad-command" for "image-builder"`) + _, stderr := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.EqualError(t, err, `unknown command "bad-command" for "image-builder"`) + }) // no extra output from cobra - assert.Equal(t, "", fakeStderr.String()) + assert.Equal(t, "", stderr) } func TestListImagesErrorsOnExtraArgs(t *testing.T) { @@ -115,10 +106,6 @@ func TestListImagesErrorsOnExtraArgs(t *testing.T) { restore = main.MockOsArgs(append([]string{"list"}, "extra-arg")) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - err := main.Run() assert.EqualError(t, err, `unknown command "extra-arg" for "image-builder list"`) } @@ -130,10 +117,6 @@ func TestBootcAnacondaIsoNotSupportedForImageBuilder(t *testing.T) { restore = main.MockOsArgs(append([]string{"manifest"}, "anaconda-iso", "--bootc-ref=example.com/cnt")) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - err := main.Run() assert.EqualError(t, err, `image type bootc "anaconda-iso" is not supported with image-builder, please consider switching to "bootc-installer" or use bootc-image-builder`) } @@ -189,22 +172,19 @@ func TestManifestIntegrationSmoke(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) - - pipelineNames, err := manifesttest.PipelineNamesFrom(fakeStdout.Bytes()) + stdout, _ := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) + pipelineNames, err := manifesttest.PipelineNamesFrom([]byte(stdout)) assert.NoError(t, err) assert.Contains(t, pipelineNames, "qcow2") // XXX: provide helpers in manifesttest to extract this in a nicer way - assert.Contains(t, fakeStdout.String(), `{"type":"org.osbuild.users","options":{"users":{"alice":{}}}}`) - assert.Contains(t, fakeStdout.String(), `"image":{"name":"registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/fedora-minimal"`) + assert.Contains(t, stdout, `{"type":"org.osbuild.users","options":{"users":{"alice":{}}}}`) + assert.Contains(t, stdout, `"image":{"name":"registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/fedora-minimal"`) - assert.Equal(t, strings.Contains(fakeStdout.String(), "org.osbuild.librepo"), useLibrepo) + assert.Equal(t, strings.Contains(stdout, "org.osbuild.librepo"), useLibrepo) }) } } @@ -228,19 +208,16 @@ func TestManifestIntegrationCrossArch(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) - - pipelineNames, err := manifesttest.PipelineNamesFrom(fakeStdout.Bytes()) + stdout, _ := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) + pipelineNames, err := manifesttest.PipelineNamesFrom([]byte(stdout)) assert.NoError(t, err) assert.Contains(t, pipelineNames, "archive") // XXX: provide helpers in manifesttest to extract this in a nicer way - assert.Contains(t, fakeStdout.String(), `.el9.s390x.rpm`) + assert.Contains(t, stdout, `.el9.s390x.rpm`) } func TestManifestIntegrationOstreeSmoke(t *testing.T) { @@ -271,19 +248,17 @@ func TestManifestIntegrationOstreeSmoke(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err = main.Run() - assert.NoError(t, err) + stdout, _ := testutil.CaptureStdio(t, func() { + err = main.Run() + assert.NoError(t, err) + }) - pipelineNames, err := manifesttest.PipelineNamesFrom(fakeStdout.Bytes()) + pipelineNames, err := manifesttest.PipelineNamesFrom([]byte(stdout)) assert.NoError(t, err) assert.Contains(t, pipelineNames, "ostree-deployment") // XXX: provide helpers in manifesttest to extract this in a nicer way - assert.Contains(t, fakeStdout.String(), `{"type":"org.osbuild.ostree.init-fs"`) + assert.Contains(t, stdout, `{"type":"org.osbuild.ostree.init-fs"`) } func TestManifestIntegrationOstreeSmokeErrors(t *testing.T) { @@ -317,12 +292,11 @@ func TestManifestIntegrationOstreeSmokeErrors(t *testing.T) { restore = main.MockOsArgs(args) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.EqualError(t, err, tc.expectedErr) + // just silence the output + _, _ = testutil.CaptureStdio(t, func() { + err := main.Run() + assert.EqualError(t, err, tc.expectedErr) + }) } } @@ -377,10 +351,6 @@ func TestBuildIntegrationHappy(t *testing.T) { restore := main.MockNewRepoRegistry(testrepos.New) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - tmpdir := t.TempDir() restore = main.MockOsArgs([]string{ "build", @@ -394,15 +364,16 @@ func TestBuildIntegrationHappy(t *testing.T) { script := makeFakeOsbuildScript() fakeOsbuildCmd := testutil.MockCommand(t, "osbuild", script) - var err error - // run inside the tmpdir to validate that the default output dir - // creation works - testutil.Chdir(t, tmpdir, func() { - err = main.Run() + stdout, _ := testutil.CaptureStdio(t, func() { + // run inside the tmpdir to validate that the default output dir + // creation works + testutil.Chdir(t, tmpdir, func() { + err := main.Run() + assert.NoError(t, err) + }) }) - assert.NoError(t, err) - assert.Contains(t, fakeStdout.String(), `Image build successful: centos-9-qcow2-x86_64/centos-9-qcow2-x86_64.qcow2`) + assert.Contains(t, stdout, `Image build successful: centos-9-qcow2-x86_64/centos-9-qcow2-x86_64.qcow2`) // ensure osbuild was run exactly one require.Equal(t, 1, len(fakeOsbuildCmd.CallArgsList())) @@ -674,12 +645,11 @@ func TestManifestIntegrationWithSBOMWithOutputDir(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) + // silence output + _, _ = testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) sboms, err := filepath.Glob(filepath.Join(outputDir, "*.spdx.json")) assert.NoError(t, err) @@ -700,14 +670,11 @@ func TestDescribeImageSmoke(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) - - assert.Contains(t, fakeStdout.String(), `distro: centos-9 + stdout, _ := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) + assert.Contains(t, stdout, `distro: centos-9 type: qcow2 arch: x86_64`) } @@ -727,14 +694,12 @@ func TestDescribeImageMinimal(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err := main.Run() - assert.NoError(t, err) + stdout, _ := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) - assert.Contains(t, fakeStdout.String(), fmt.Sprintf(`distro: centos-9 + assert.Contains(t, stdout, fmt.Sprintf(`distro: centos-9 type: qcow2 arch: %s`, arch.Current().String())) } @@ -809,16 +774,14 @@ func TestManifestExtraRepos(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err = main.Run() - require.NoError(t, err) + stdout, _ := testutil.CaptureStdio(t, func() { + err = main.Run() + require.NoError(t, err) + }) // our local repo got added - assert.Contains(t, fakeStdout.String(), `"path":"dummy-1.0.0-0.noarch.rpm"`) - assert.Contains(t, fakeStdout.String(), fmt.Sprintf(`"url":"file://%s"`, localRepoDir)) + assert.Contains(t, stdout, `"path":"dummy-1.0.0-0.noarch.rpm"`) + assert.Contains(t, stdout, fmt.Sprintf(`"url":"file://%s"`, localRepoDir)) }) } } @@ -831,11 +794,7 @@ func TestManifestOverrideRepo(t *testing.T) { t.Skip("no osbuild-depsolve-dnf binary found") } - var fakeStderr bytes.Buffer - restore := main.MockOsStderr(&fakeStderr) - defer restore() - - restore = main.MockOsArgs([]string{ + restore := main.MockOsArgs([]string{ "manifest", "qcow2", "--distro=centos-9", @@ -934,15 +893,13 @@ func TestManifestBootstrapContainer(t *testing.T) { restore = main.MockOsArgs(cmd) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - // XXX: capture stderr here too to ensure no cross build warning - err := main.Run() - assert.NoError(t, err) + stdout, stderr := testutil.CaptureStdio(t, func() { + err := main.Run() + assert.NoError(t, err) + }) + assert.Equal(t, "Manifest generation step\nBuilding manifest for centos-10-tar\n", stderr) - pipelines, err := manifesttest.PipelineNamesFrom(fakeStdout.Bytes()) + pipelines, err := manifesttest.PipelineNamesFrom([]byte(stdout)) assert.NoError(t, err) bootstrapPipeline := "bootstrap-buildroot" if disableBootstrapContainer { @@ -964,10 +921,6 @@ func TestBuildIntegrationOutputFilename(t *testing.T) { restore := main.MockNewRepoRegistry(testrepos.New) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - tmpdir := t.TempDir() outputDir := filepath.Join(tmpdir, "output") restore = main.MockOsArgs([]string{ @@ -1104,17 +1057,15 @@ func TestManifestIntegrationWithRegistrations(t *testing.T) { }) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - - err = main.Run() - assert.NoError(t, err) + stdout, _ := testutil.CaptureStdio(t, func() { + err = main.Run() + assert.NoError(t, err) + }) // XXX: manifesttest really needs to grow more helpers - assert.Contains(t, fakeStdout.String(), `{"type":"org.osbuild.insights-client.config","options":{"config":{"proxy":"proxy_123"}}}`) - assert.Contains(t, fakeStdout.String(), `"type":"org.osbuild.systemd.unit.create","options":{"filename":"osbuild-subscription-register.service"`) - assert.Contains(t, fakeStdout.String(), `server_url_123`) + assert.Contains(t, stdout, `{"type":"org.osbuild.insights-client.config","options":{"config":{"proxy":"proxy_123"}}}`) + assert.Contains(t, stdout, `"type":"org.osbuild.systemd.unit.create","options":{"filename":"osbuild-subscription-register.service"`) + assert.Contains(t, stdout, `server_url_123`) } func TestManifestIntegrationWarningsHandling(t *testing.T) { @@ -1146,10 +1097,6 @@ customizations.FIPS = true }, tc.extraCmdline...)) defer restore() - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - var err error _, stderr := testutil.CaptureStdio(t, func() { err = main.Run() diff --git a/cmd/image-builder/upload.go b/cmd/image-builder/upload.go index 6c4d76f2..c752a7f1 100644 --- a/cmd/image-builder/upload.go +++ b/cmd/image-builder/upload.go @@ -54,12 +54,12 @@ func uploadImageWithProgress(uploader cloud.Uploader, imagePath string) error { size := uint64(st.Size()) pbar := pb.New64(st.Size()) pbar.Set(pb.Bytes, true) - pbar.SetWriter(osStdout) + pbar.SetWriter(os.Stdout) r := pbar.NewProxyReader(f) pbar.Start() defer pbar.Finish() - return uploader.UploadAndRegister(r, size, osStderr) + return uploader.UploadAndRegister(r, size, os.Stderr) } func uploaderCheckWithProgress(pbar progress.ProgressBar, uploader cloud.Uploader) error { @@ -264,11 +264,11 @@ func cmdUpload(cmd *cobra.Command, args []string) error { if targetArch == "" { targetArch = detectArchFromImagePath(imagePath) if targetArch != "" { - fmt.Fprintf(osStderr, "Note: using architecture %q based on image filename (use --arch to override)\n", targetArch) + fmt.Fprintf(os.Stderr, "Note: using architecture %q based on image filename (use --arch to override)\n", targetArch) } if targetArch == "" { targetArch = arch.Current().String() - fmt.Fprintf(osStderr, "WARNING: no upload architecture specified, using %q (use --arch to override)\n", targetArch) + fmt.Fprintf(os.Stderr, "WARNING: no upload architecture specified, using %q (use --arch to override)\n", targetArch) } } diff --git a/cmd/image-builder/upload_test.go b/cmd/image-builder/upload_test.go index a11b9d5f..2896eb4f 100644 --- a/cmd/image-builder/upload_test.go +++ b/cmd/image-builder/upload_test.go @@ -1,7 +1,6 @@ package main_test import ( - "bytes" "fmt" "os" "path/filepath" @@ -53,12 +52,6 @@ func TestUploadWithAWSMock(t *testing.T) { }) defer restore() - var fakeStdout, fakeStderr bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - restore = main.MockOsStderr(&fakeStderr) - defer restore() - restore = main.MockOsArgs([]string{ "upload", "--to=aws", @@ -70,8 +63,10 @@ func TestUploadWithAWSMock(t *testing.T) { }) defer restore() - err = main.Run() - require.NoError(t, err) + stdout, stderr := testutil.CaptureStdio(t, func() { + err = main.Run() + require.NoError(t, err) + }) assert.Equal(t, regionName, "aws-region-1") assert.Equal(t, bucketName, "aws-bucket-2") @@ -85,19 +80,15 @@ func TestUploadWithAWSMock(t *testing.T) { assert.Equal(t, 1, fa.uploadAndRegisterCalls) assert.Equal(t, fakeDiskContent, fa.uploadAndRegisterRead.String()) // progress was rendered - assert.Contains(t, fakeStdout.String(), "--] 100.00%") + assert.Contains(t, stdout, "--] 100.00%") // warning was passed - assert.Equal(t, fakeStderr.String(), tc.expectedWarning) + assert.Equal(t, stderr, tc.expectedWarning) } } func TestUploadCmdlineErrors(t *testing.T) { - var fakeStderr bytes.Buffer - restore := main.MockOsStderr(&fakeStderr) - defer restore() - for _, tc := range []struct { cmdline []string expectedErr string @@ -154,10 +145,6 @@ func TestBuildAndUploadWithAWSMock(t *testing.T) { fakeOsbuildScript := makeFakeOsbuildScript() testutil.MockCommand(t, "osbuild", fakeOsbuildScript) - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - restore = main.MockOsArgs([]string{ "build", "--output-dir", outputDir, @@ -169,8 +156,10 @@ func TestBuildAndUploadWithAWSMock(t *testing.T) { }) defer restore() - err := main.Run() - require.NoError(t, err) + _, _ = testutil.CaptureStdio(t, func() { + err := main.Run() + require.NoError(t, err) + }) assert.Equal(t, regionName, "aws-region-1") assert.Equal(t, bucketName, "aws-bucket-2") @@ -206,10 +195,6 @@ func TestBuildAndUploadFedoraWithAWSMock(t *testing.T) { fakeOsbuildScript := makeFakeOsbuildScript() testutil.MockCommand(t, "osbuild", fakeOsbuildScript) - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - restore = main.MockOsArgs([]string{ "build", "--output-dir", outputDir, @@ -254,10 +239,6 @@ func TestBuildAmiButNotUpload(t *testing.T) { fakeOsbuildScript := makeFakeOsbuildScript() testutil.MockCommand(t, "osbuild", fakeOsbuildScript) - var fakeStdout bytes.Buffer - restore = main.MockOsStdout(&fakeStdout) - defer restore() - restore = main.MockOsArgs([]string{ "build", "--output-dir", outputDir,