swap 'pkg push' for 'pkg build --push', keeping 'pkg push' as deprecated but still working (#4141)

Signed-off-by: Avi Deitcher <avi@deitcher.net>

src/cmd/linuxkit/pkg.go

@@ -75,9 +75,11 @@ func pkgCmd() *cobra.Command {
 		},
 	}
 
-	cmd.AddCommand(pkgBuildCmd())
+	// because there is an alias 'pkg push' for 'pkg build --push', we need to add the build command first
+	buildCmd := pkgBuildCmd()
+	cmd.AddCommand(buildCmd)
 	cmd.AddCommand(pkgBuilderCmd())
-	cmd.AddCommand(pkgPushCmd())
+	cmd.AddCommand(pkgPushCmd(buildCmd))
 	cmd.AddCommand(pkgShowTagCmd())
 	cmd.AddCommand(pkgManifestCmd())
 	cmd.AddCommand(pkgRemoteTagCmd())

src/cmd/linuxkit/pkg_build.go

@@ -20,21 +20,21 @@ const (
 )
 
 // some logic clarification:
-// pkg build                   - builds unless is in cache or published in registry
+// pkg build                           - builds unless is in cache or published in registry
-// pkg build --pull            - builds unless is in cache or published in registry; pulls from registry if not in cache
+// pkg build --pull                    - builds unless is in cache or published in registry; pulls from registry if not in cache
-// pkg build --force           - always builds even if is in cache or published in registry
+// pkg build --force                   - always builds even if is in cache or published in registry
-// pkg build --force --pull    - always builds even if is in cache or published in registry; --pull ignored
+// pkg build --force --pull            - always builds even if is in cache or published in registry; --pull ignored
-// pkg push                    - always builds unless is in cache
+// pkg build --push 		           - always builds unless is in cache or published in registry; pushes to registry
-// pkg push --force            - always builds even if is in cache
+// pkg build --push --force            - always builds even if is in cache
-// pkg push --nobuild          - skips build; if not in cache, fails
+// pkg build --push --nobuild          - skips build; if not in cache, fails
-// pkg push --nobuild --force  - nonsensical
+// pkg build --push --nobuild --force  - nonsensical
+// pkg push                            - equivalent to pkg build --push
 
-// addCmdRunPkgBuildPush adds the RunE function and flags to a cobra.Command
+func pkgBuildCmd() *cobra.Command {
-// for "pkg build" or "pkg push".
-func addCmdRunPkgBuildPush(cmd *cobra.Command, withPush bool) *cobra.Command {
 	var (
 		force          bool
 		pull           bool
+		push           bool
 		ignoreCache    bool
 		docker         bool
 		platforms      string
@@ -53,220 +53,228 @@ func addCmdRunPkgBuildPush(cmd *cobra.Command, withPush bool) *cobra.Command {
 		progress       string
 		ssh            []string
 	)
-
+	cmd := &cobra.Command{
-	cmd.RunE = func(cmd *cobra.Command, args []string) error {
+		Use:   "build",
-		pkgs, err := pkglib.NewFromConfig(pkglibConfig, args...)
+		Short: "build an OCI package from a directory with a yaml configuration file",
-		if err != nil {
+		Long: `Build an OCI package from a directory with a yaml configuration file.
-			return err
+		'path' specifies the path to the package source directory.
-		}
+`,
-
+		Example: `  linuxkit pkg build [options] pkg/dir/`,
-		if nobuild && force {
+		Args:    cobra.MinimumNArgs(1),
-			return errors.New("flags -force and -nobuild conflict")
+		RunE: func(cmd *cobra.Command, args []string) error {
-		}
+			pkgs, err := pkglib.NewFromConfig(pkglibConfig, args...)
-		if pull && force {
-			return errors.New("flags -force and -pull conflict")
-		}
-
-		var opts []pkglib.BuildOpt
-		if force {
-			opts = append(opts, pkglib.WithBuildForce())
-		}
-		if ignoreCache {
-			opts = append(opts, pkglib.WithBuildIgnoreCache())
-		}
-		if pull {
-			opts = append(opts, pkglib.WithBuildPull())
-		}
-
-		opts = append(opts, pkglib.WithBuildCacheDir(cacheDir.String()))
-
-		if withPush {
-			opts = append(opts, pkglib.WithBuildPush())
-			if nobuild {
-				opts = append(opts, pkglib.WithBuildSkip())
-			}
-			if release != "" {
-				opts = append(opts, pkglib.WithRelease(release))
-			}
-			if manifest {
-				opts = append(opts, pkglib.WithBuildManifest())
-			}
-		}
-		if docker {
-			opts = append(opts, pkglib.WithBuildTargetDockerCache())
-		}
-
-		if sbomScanner != "false" {
-			opts = append(opts, pkglib.WithBuildSbomScanner(sbomScanner))
-		}
-		opts = append(opts, pkglib.WithDockerfile(dockerfile))
-
-		// read any build arg files
-		var buildArgs []string
-		for _, filename := range buildArgFiles {
-			f, err := os.Open(filename)
 			if err != nil {
-				return fmt.Errorf("error opening build args file %s: %w", filename, err)
+				return err
 			}
-			defer func() { _ = f.Close() }()
-			scanner := bufio.NewScanner(f)
-			for scanner.Scan() {
-				buildArgs = append(buildArgs, scanner.Text())
-			}
-			if err := scanner.Err(); err != nil {
-				return fmt.Errorf("error reading build args file %s: %w", filename, err)
-			}
-		}
-		opts = append(opts, pkglib.WithBuildArgs(buildArgs))
 
-		// skipPlatformsMap contains platforms that should be skipped
+			if nobuild && force {
-		skipPlatformsMap := make(map[string]bool)
+				return errors.New("flags -force and -nobuild conflict")
-		if skipPlatforms != "" {
-			for _, platform := range strings.Split(skipPlatforms, ",") {
-				parts := strings.SplitN(platform, "/", 2)
-				if len(parts) != 2 || parts[0] == "" || parts[0] != "linux" || parts[1] == "" {
-					return fmt.Errorf("invalid target platform specification '%s'", platform)
-				}
-				skipPlatformsMap[strings.Trim(parts[1], " ")] = true
 			}
-		}
+			if pull && force {
-		// if requested specific platforms, build those. If not, then we will
+				return errors.New("flags -force and -pull conflict")
-		// retrieve the defaults in the loop over each package.
-		var plats []imagespec.Platform
-		// don't allow the use of --skip-platforms with --platforms
-		if platforms != "" && skipPlatforms != "" {
-			return errors.New("--skip-platforms and --platforms may not be used together")
-		}
-		// process the platforms if provided
-		if platforms != "" {
-			for _, p := range strings.Split(platforms, ",") {
-				parts := strings.SplitN(p, "/", 2)
-				if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
-					fmt.Fprintf(os.Stderr, "invalid target platform specification '%s'\n", p)
-					os.Exit(1)
-				}
-				plats = append(plats, imagespec.Platform{OS: parts[0], Architecture: parts[1]})
 			}
-		}
 
-		// build the builders map
+			var opts []pkglib.BuildOpt
-		buildersMap := map[string]string{}
+			if force {
-		// look for builders env var
+				opts = append(opts, pkglib.WithBuildForce())
-		buildersMap, err = buildPlatformBuildersMap(os.Getenv(buildersEnvVar), buildersMap)
+			}
-		if err != nil {
+			if ignoreCache {
-			return fmt.Errorf("error in environment variable %s: %w", buildersEnvVar, err)
+				opts = append(opts, pkglib.WithBuildIgnoreCache())
-		}
+			}
-		// any CLI options override env var
+			if pull {
-		buildersMap, err = buildPlatformBuildersMap(builders, buildersMap)
+				opts = append(opts, pkglib.WithBuildPull())
-		if err != nil {
-			return fmt.Errorf("error in --builders flag: %w", err)
-		}
-		if builderConfig != "" {
-			if _, err := os.Stat(builderConfig); err != nil {
-				return fmt.Errorf("error reading builder config file %s: %w", builderConfig, err)
 			}
-			opts = append(opts, pkglib.WithBuildBuilderConfig(builderConfig))
-		}
 
-		opts = append(opts, pkglib.WithBuildBuilders(buildersMap))
+			opts = append(opts, pkglib.WithBuildCacheDir(cacheDir.String()))
-		opts = append(opts, pkglib.WithBuildBuilderImage(builderImage))
+
-		opts = append(opts, pkglib.WithBuildBuilderRestart(builderRestart))
+			if push {
-		opts = append(opts, pkglib.WithProgress(progress))
+				opts = append(opts, pkglib.WithBuildPush())
-		if len(ssh) > 0 {
+				if nobuild {
-			opts = append(opts, pkglib.WithSSH(ssh))
+					opts = append(opts, pkglib.WithBuildSkip())
-		}
-		if len(registryCreds) > 0 {
-			registryCredMap := make(map[string]spec.RegistryAuth)
-			for _, cred := range registryCreds {
-				parts := strings.SplitN(cred, "=", 2)
-				if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
-					return fmt.Errorf("invalid registry auth specification '%s'", cred)
 				}
-				registryPart := strings.TrimSpace(parts[0])
+				if release != "" {
-				authPart := strings.TrimSpace(parts[1])
+					opts = append(opts, pkglib.WithRelease(release))
-				var auth spec.RegistryAuth
-				// if the auth is a token, we don't need a username
-				credParts := strings.SplitN(authPart, ":", 2)
-				var userPart, credPart string
-				userPart = strings.TrimSpace(credParts[0])
-				if len(credParts) == 2 {
-					credPart = strings.TrimSpace(credParts[1])
 				}
+				if manifest {
+					opts = append(opts, pkglib.WithBuildManifest())
+				}
+			}
+			if docker {
+				opts = append(opts, pkglib.WithBuildTargetDockerCache())
+			}
+
+			if sbomScanner != "false" {
+				opts = append(opts, pkglib.WithBuildSbomScanner(sbomScanner))
+			}
+			opts = append(opts, pkglib.WithDockerfile(dockerfile))
+
+			// read any build arg files
+			var buildArgs []string
+			for _, filename := range buildArgFiles {
+				f, err := os.Open(filename)
+				if err != nil {
+					return fmt.Errorf("error opening build args file %s: %w", filename, err)
+				}
+				defer func() { _ = f.Close() }()
+				scanner := bufio.NewScanner(f)
+				for scanner.Scan() {
+					buildArgs = append(buildArgs, scanner.Text())
+				}
+				if err := scanner.Err(); err != nil {
+					return fmt.Errorf("error reading build args file %s: %w", filename, err)
+				}
+			}
+			opts = append(opts, pkglib.WithBuildArgs(buildArgs))
+
+			// skipPlatformsMap contains platforms that should be skipped
+			skipPlatformsMap := make(map[string]bool)
+			if skipPlatforms != "" {
+				for _, platform := range strings.Split(skipPlatforms, ",") {
+					parts := strings.SplitN(platform, "/", 2)
+					if len(parts) != 2 || parts[0] == "" || parts[0] != "linux" || parts[1] == "" {
+						return fmt.Errorf("invalid target platform specification '%s'", platform)
+					}
+					skipPlatformsMap[strings.Trim(parts[1], " ")] = true
+				}
+			}
+			// if requested specific platforms, build those. If not, then we will
+			// retrieve the defaults in the loop over each package.
+			var plats []imagespec.Platform
+			// don't allow the use of --skip-platforms with --platforms
+			if platforms != "" && skipPlatforms != "" {
+				return errors.New("--skip-platforms and --platforms may not be used together")
+			}
+			// process the platforms if provided
+			if platforms != "" {
+				for _, p := range strings.Split(platforms, ",") {
+					parts := strings.SplitN(p, "/", 2)
+					if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
+						fmt.Fprintf(os.Stderr, "invalid target platform specification '%s'\n", p)
+						os.Exit(1)
+					}
+					plats = append(plats, imagespec.Platform{OS: parts[0], Architecture: parts[1]})
+				}
+			}
+
+			// build the builders map
+			buildersMap := map[string]string{}
+			// look for builders env var
+			buildersMap, err = buildPlatformBuildersMap(os.Getenv(buildersEnvVar), buildersMap)
+			if err != nil {
+				return fmt.Errorf("error in environment variable %s: %w", buildersEnvVar, err)
+			}
+			// any CLI options override env var
+			buildersMap, err = buildPlatformBuildersMap(builders, buildersMap)
+			if err != nil {
+				return fmt.Errorf("error in --builders flag: %w", err)
+			}
+			if builderConfig != "" {
+				if _, err := os.Stat(builderConfig); err != nil {
+					return fmt.Errorf("error reading builder config file %s: %w", builderConfig, err)
+				}
+				opts = append(opts, pkglib.WithBuildBuilderConfig(builderConfig))
+			}
+
+			opts = append(opts, pkglib.WithBuildBuilders(buildersMap))
+			opts = append(opts, pkglib.WithBuildBuilderImage(builderImage))
+			opts = append(opts, pkglib.WithBuildBuilderRestart(builderRestart))
+			opts = append(opts, pkglib.WithProgress(progress))
+			if len(ssh) > 0 {
+				opts = append(opts, pkglib.WithSSH(ssh))
+			}
+			if len(registryCreds) > 0 {
+				registryCredMap := make(map[string]spec.RegistryAuth)
+				for _, cred := range registryCreds {
+					parts := strings.SplitN(cred, "=", 2)
+					if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
+						return fmt.Errorf("invalid registry auth specification '%s'", cred)
+					}
+					registryPart := strings.TrimSpace(parts[0])
+					authPart := strings.TrimSpace(parts[1])
+					var auth spec.RegistryAuth
+					// if the auth is a token, we don't need a username
+					credParts := strings.SplitN(authPart, ":", 2)
+					var userPart, credPart string
+					userPart = strings.TrimSpace(credParts[0])
+					if len(credParts) == 2 {
+						credPart = strings.TrimSpace(credParts[1])
+					}
+					switch {
+					case len(registryPart) == 0:
+						return fmt.Errorf("invalid registry auth specification '%s', registry must not be blank", cred)
+					case len(credParts) == 2 && (len(userPart) == 0 || len(credPart) == 0):
+						return fmt.Errorf("invalid registry auth specification '%s', username and password must not be blank", cred)
+					case len(credParts) == 1 && len(userPart) == 0:
+						return fmt.Errorf("invalid registry auth specification '%s', token must not be blank", cred)
+					case len(credParts) == 2:
+						auth = spec.RegistryAuth{
+							Username: userPart,
+							Password: credPart,
+						}
+					case len(credParts) == 1:
+						auth = spec.RegistryAuth{
+							RegistryToken: authPart,
+						}
+					default:
+						return fmt.Errorf("invalid registry auth specification '%s'", cred)
+					}
+					registryCredMap[registryPart] = auth
+				}
+				opts = append(opts, pkglib.WithRegistryAuth(registryCredMap))
+			}
+
+			for _, p := range pkgs {
+				// things we need our own copies of
+				var (
+					pkgOpts  = make([]pkglib.BuildOpt, len(opts))
+					pkgPlats = make([]imagespec.Platform, len(plats))
+				)
+				copy(pkgOpts, opts)
+				copy(pkgPlats, plats)
+				// unless overridden, platforms are specific to a package, so this needs to be inside the for loop
+				if len(pkgPlats) == 0 {
+					for _, a := range p.Arches() {
+						if _, ok := skipPlatformsMap[a]; ok {
+							continue
+						}
+						pkgPlats = append(pkgPlats, imagespec.Platform{OS: "linux", Architecture: a})
+					}
+				}
+
+				// if there are no platforms to build for, do nothing.
+				// note that this is *not* an error; we simply skip it
+				if len(pkgPlats) == 0 {
+					fmt.Printf("Skipping %s with no architectures to build\n", p.Tag())
+					continue
+				}
+
+				pkgOpts = append(pkgOpts, pkglib.WithBuildPlatforms(pkgPlats...))
+
+				var msg, action string
 				switch {
-				case len(registryPart) == 0:
+				case !push:
-					return fmt.Errorf("invalid registry auth specification '%s', registry must not be blank", cred)
+					msg = fmt.Sprintf("Building %q", p.Tag())
-				case len(credParts) == 2 && (len(userPart) == 0 || len(credPart) == 0):
+					action = "building"
-					return fmt.Errorf("invalid registry auth specification '%s', username and password must not be blank", cred)
+				case nobuild:
-				case len(credParts) == 1 && len(userPart) == 0:
+					msg = fmt.Sprintf("Pushing %q without building", p.Tag())
-					return fmt.Errorf("invalid registry auth specification '%s', token must not be blank", cred)
+					action = "building and pushing"
-				case len(credParts) == 2:
-					auth = spec.RegistryAuth{
-						Username: userPart,
-						Password: credPart,
-					}
-				case len(credParts) == 1:
-					auth = spec.RegistryAuth{
-						RegistryToken: authPart,
-					}
 				default:
-					return fmt.Errorf("invalid registry auth specification '%s'", cred)
+					msg = fmt.Sprintf("Building and pushing %q", p.Tag())
+					action = "building and pushing"
 				}
-				registryCredMap[registryPart] = auth
-			}
-			opts = append(opts, pkglib.WithRegistryAuth(registryCredMap))
-		}
 
-		for _, p := range pkgs {
+				fmt.Println(msg)
-			// things we need our own copies of
+
-			var (
+				if err := p.Build(pkgOpts...); err != nil {
-				pkgOpts  = make([]pkglib.BuildOpt, len(opts))
+					return fmt.Errorf("error %s %q: %w", action, p.Tag(), err)
-				pkgPlats = make([]imagespec.Platform, len(plats))
-			)
-			copy(pkgOpts, opts)
-			copy(pkgPlats, plats)
-			// unless overridden, platforms are specific to a package, so this needs to be inside the for loop
-			if len(pkgPlats) == 0 {
-				for _, a := range p.Arches() {
-					if _, ok := skipPlatformsMap[a]; ok {
-						continue
-					}
-					pkgPlats = append(pkgPlats, imagespec.Platform{OS: "linux", Architecture: a})
 				}
 			}
-
+			return nil
-			// if there are no platforms to build for, do nothing.
+		},
-			// note that this is *not* an error; we simply skip it
-			if len(pkgPlats) == 0 {
-				fmt.Printf("Skipping %s with no architectures to build\n", p.Tag())
-				continue
-			}
-
-			pkgOpts = append(pkgOpts, pkglib.WithBuildPlatforms(pkgPlats...))
-
-			var msg, action string
-			switch {
-			case !withPush:
-				msg = fmt.Sprintf("Building %q", p.Tag())
-				action = "building"
-			case nobuild:
-				msg = fmt.Sprintf("Pushing %q without building", p.Tag())
-				action = "building and pushing"
-			default:
-				msg = fmt.Sprintf("Building and pushing %q", p.Tag())
-				action = "building and pushing"
-			}
-
-			fmt.Println(msg)
-
-			if err := p.Build(pkgOpts...); err != nil {
-				return fmt.Errorf("error %s %q: %w", action, p.Tag(), err)
-			}
-		}
-		return nil
 	}
-
 	cmd.Flags().BoolVar(&force, "force", false, "Force rebuild even if image is in local cache")
 	cmd.Flags().BoolVar(&pull, "pull", false, "Pull image if in registry but not in local cache; conflicts with --force")
+	cmd.Flags().BoolVar(&push, "push", false, "After building, if successful, push the image to the registry; if --nobuild is set, just push")
 	cmd.Flags().BoolVar(&ignoreCache, "ignore-cached", false, "Ignore cached intermediate images, always pulling from registry")
 	cmd.Flags().BoolVar(&docker, "docker", false, "Store the built image in the docker image cache instead of the default linuxkit cache")
 	cmd.Flags().StringVar(&platforms, "platforms", "", "Which platforms to build for, defaults to all of those for which the package can be built")
@@ -287,18 +295,6 @@ func addCmdRunPkgBuildPush(cmd *cobra.Command, withPush bool) *cobra.Command {
 
 	return cmd
 }
-func pkgBuildCmd() *cobra.Command {
-	cmd := &cobra.Command{
-		Use:   "build",
-		Short: "build an OCI package from a directory with a yaml configuration file",
-		Long: `Build an OCI package from a directory with a yaml configuration file.
-		'path' specifies the path to the package source directory.
-`,
-		Example: `  linuxkit pkg build [options] pkg/dir/`,
-		Args:    cobra.MinimumNArgs(1),
-	}
-	return addCmdRunPkgBuildPush(cmd, false)
-}
 
 func buildPlatformBuildersMap(inputs string, existing map[string]string) (map[string]string, error) {
 	if inputs == "" {

src/cmd/linuxkit/pkg_push.go

@@ -1,18 +1,35 @@
 package main
 
-import "github.com/spf13/cobra"
+import (
+	"fmt"
 
-func pkgPushCmd() *cobra.Command {
+	"github.com/spf13/cobra"
+)
+
+func pkgPushCmd(buildCmd *cobra.Command) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "push",
-		Short: "build and push an OCI package from a directory with a yaml configuration file",
+		Short: "Alias for 'pkg build --push'",
 		Long: `Build and push an OCI package from a directory with a yaml configuration file.
 		'path' specifies the path to the package source directory.
 
 		The package may or may not be built first, depending on options
 `,
-		Example: `  linuxkit pkg push [options] pkg/dir/`,
+		Example:    `  linuxkit pkg push [options] pkg/dir/`,
-		Args:    cobra.MinimumNArgs(1),
+		SuggestFor: []string{"build"},
+		Args:       cobra.MinimumNArgs(1),
+		Deprecated: "use 'pkg build --push' instead",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			// Create a copy of buildCmd with push=true
+			if err := buildCmd.Flags().Set("push", "true"); err != nil {
+				return fmt.Errorf("'pkg push' unable to set 'pkg build --push': %w", err)
+			}
+
+			// Pass the args to the build command
+			buildCmd.SetArgs(args)
+			return buildCmd.RunE(buildCmd, args)
+		},
 	}
-	return addCmdRunPkgBuildPush(cmd, true)
+	cmd.Flags().AddFlagSet(buildCmd.Flags())
+	return cmd
 }