From 4b8bff8404d0aa9fb8c6cf1c390e40646c9a4d99 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 4 Dec 2024 16:49:23 +0100 Subject: [PATCH] cmd: rework argument handling This commit tweaks the argument handling based on the suggestion by Achilleas and Ondrej (thanks!). Now it takes ```console $ image-builder manifest qcow2 ./path/to/blueprint ... $ image-builder manifest --arch s390x --distro centos-9 qcow2 ... ``` If no arch is specified the hostname arch is used. If no distro is specified in either the blueprint or the commandline it is auto-detected based on the host. Note that if the distro from the comandline and the blueprint diverge an error is raised. We can relax this rule and just add precedence, e.g. commandline always overrides the blueprint but ideally we would have a clear use-case here so we start conservative and can always relax this rule later (the inverse is much harder). This means it is no longer copy/paste friendly from `list-images` by default but instead we can provide a new `list-images --output=shell` option that outputs in exactly the format that `image-builder [manifest|build]` need. --- cmd/image-builder/distro.go | 30 ++++++++++++++++++++ cmd/image-builder/distro_test.go | 48 ++++++++++++++++++++++++++++++++ cmd/image-builder/export_test.go | 9 ++++++ cmd/image-builder/main.go | 41 ++++++++++++++++----------- cmd/image-builder/main_test.go | 41 +++++++++++++++++++++++++-- 5 files changed, 150 insertions(+), 19 deletions(-) create mode 100644 cmd/image-builder/distro.go create mode 100644 cmd/image-builder/distro_test.go diff --git a/cmd/image-builder/distro.go b/cmd/image-builder/distro.go new file mode 100644 index 0000000..deaae7a --- /dev/null +++ b/cmd/image-builder/distro.go @@ -0,0 +1,30 @@ +package main + +import ( + "fmt" + + "github.com/osbuild/images/pkg/distro" +) + +var distroGetHostDistroName = distro.GetHostDistroName + +// findDistro will ensure that the given distro argument do not +// diverge. If no distro is set via the blueprint or the argument +// the host is used to derive the distro. +func findDistro(argDistroName, bpDistroName string) (string, error) { + switch { + case argDistroName != "" && bpDistroName != "" && argDistroName != bpDistroName: + return "", fmt.Errorf("error selecting distro name, cmdline argument %q is different from blueprint %q", argDistroName, bpDistroName) + case argDistroName != "": + return argDistroName, nil + case bpDistroName != "": + return bpDistroName, nil + } + // nothing selected by the user, derive from host + distroStr, err := distroGetHostDistroName() + 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", distroStr) + return distroStr, nil +} diff --git a/cmd/image-builder/distro_test.go b/cmd/image-builder/distro_test.go new file mode 100644 index 0000000..7c187f8 --- /dev/null +++ b/cmd/image-builder/distro_test.go @@ -0,0 +1,48 @@ +package main_test + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/osbuild/image-builder-cli/cmd/image-builder" +) + +func TestFindDistro(t *testing.T) { + for _, tc := range []struct { + argDistro string + bpDistro string + expectedDistro string + expectedErr string + }{ + {"arg", "", "arg", ""}, + {"", "bp", "bp", ""}, + {"arg", "bp", "", `error selecting distro name, cmdline argument "arg" is different from blueprint "bp"`}, + // the argDistro,bpDistro == "" case is tested below + } { + distro, err := main.FindDistro(tc.argDistro, tc.bpDistro) + if tc.expectedErr != "" { + assert.Equal(t, tc.expectedErr, err.Error()) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expectedDistro, distro) + } + } +} + +func TestFindDistroAutoDetect(t *testing.T) { + var buf bytes.Buffer + restore := main.MockOsStderr(&buf) + defer restore() + + restore = main.MockDistroGetHostDistroName(func() (string, error) { + return "mocked-host-distro", nil + }) + defer restore() + + 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`, buf.String()) +} diff --git a/cmd/image-builder/export_test.go b/cmd/image-builder/export_test.go index d5a7dbe..5b3005f 100644 --- a/cmd/image-builder/export_test.go +++ b/cmd/image-builder/export_test.go @@ -11,6 +11,7 @@ import ( var ( GetOneImage = getOneImage Run = run + FindDistro = findDistro ) func MockOsArgs(new []string) (restore func()) { @@ -49,3 +50,11 @@ func MockNewRepoRegistry(f func() (*reporegistry.RepoRegistry, error)) (restore newRepoRegistry = saved } } + +func MockDistroGetHostDistroName(f func() (string, error)) (restore func()) { + saved := distroGetHostDistroName + distroGetHostDistroName = f + return func() { + distroGetHostDistroName = saved + } +} diff --git a/cmd/image-builder/main.go b/cmd/image-builder/main.go index b2ba2ce..39f24cb 100644 --- a/cmd/image-builder/main.go +++ b/cmd/image-builder/main.go @@ -41,19 +41,32 @@ func cmdManifest(cmd *cobra.Command, args []string) error { if err != nil { return err } - blueprintPath, err := cmd.Flags().GetString("blueprint") + archStr, err := cmd.Flags().GetString("arch") + if err != nil { + return err + } + if archStr == "" { + archStr = arch.Current().String() + } + distroStr, err := cmd.Flags().GetString("distro") if err != nil { return err } - distroStr := args[0] - imgTypeStr := args[1] - var archStr string - if len(args) > 2 { - archStr = args[2] - } else { - archStr = arch.Current().String() + var blueprintPath string + imgTypeStr := args[0] + if len(args) > 1 { + blueprintPath = args[1] } + bp, err := blueprintload.Load(blueprintPath) + if err != nil { + return err + } + distroStr, err = findDistro(distroStr, bp.Distro) + if err != nil { + return err + } + res, err := getOneImage(dataDir, distroStr, imgTypeStr, archStr) if err != nil { return err @@ -69,10 +82,6 @@ func cmdManifest(cmd *cobra.Command, args []string) error { if err != nil { return err } - bp, err := blueprintload.Load(blueprintPath) - if err != nil { - return err - } return mg.Generate(bp, res.Distro, res.ImgType, res.Arch, nil) } @@ -108,15 +117,15 @@ operating sytsems like centos and RHEL with easy customizations support.`, rootCmd.AddCommand(listImagesCmd) manifestCmd := &cobra.Command{ - Use: "manifest []", + Use: "manifest [blueprint]", Short: "Build manifest for the given distro/image-type, e.g. centos-9 qcow2", RunE: cmdManifest, SilenceUsage: true, - Args: cobra.MinimumNArgs(2), + Args: cobra.RangeArgs(1, 2), Hidden: true, } - // XXX: share with build - manifestCmd.Flags().String("blueprint", "", `pass a blueprint file`) + manifestCmd.Flags().String("arch", "", `build manifest for a different architecture`) + manifestCmd.Flags().String("distro", "", `build manifest for a different distroname (e.g. centos-9)`) rootCmd.AddCommand(manifestCmd) return rootCmd.Execute() diff --git a/cmd/image-builder/main_test.go b/cmd/image-builder/main_test.go index da45298..a265ef4 100644 --- a/cmd/image-builder/main_test.go +++ b/cmd/image-builder/main_test.go @@ -170,9 +170,10 @@ func TestManifestIntegrationSmoke(t *testing.T) { restore = main.MockOsArgs([]string{ "manifest", - "--blueprint", makeTestBlueprint(t, testBlueprint), - "centos-9", "qcow2"}, - ) + "qcow2", + "--distro=centos-9", + makeTestBlueprint(t, testBlueprint), + }) defer restore() var fakeStdout bytes.Buffer @@ -190,3 +191,37 @@ func TestManifestIntegrationSmoke(t *testing.T) { 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"`) } + +func TestManifestIntegrationCrossArch(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() + + restore = main.MockOsArgs([]string{ + "manifest", + "tar", + "--distro", "centos-9", + "--arch", "s390x", + }) + 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()) + 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`) +}