remove stdout from pull/push options in favor of progress handler

Signed-off-by: Mrudul Harwani <[email protected]>

cmd/nerdctl/container_create.go

@@ -24,6 +24,7 @@ import (
 	"github.com/containerd/nerdctl/pkg/clientutil"
 	"github.com/containerd/nerdctl/pkg/cmd/container"
 	"github.com/containerd/nerdctl/pkg/containerutil"
+	"github.com/containerd/nerdctl/pkg/imgutil/jobs"
 	"github.com/spf13/cobra"
 )
 
@@ -396,10 +397,10 @@ func processContainerCreateOptions(cmd *cobra.Command) (opt types.ContainerCreat
 		return
 	}
 	opt.ImagePullOpt = types.ImagePullOptions{
-		GOptions:      opt.GOptions,
-		VerifyOptions: imageVerifyOpt,
-		IPFSAddress:   opt.IPFSAddress,
-		Stdout:        opt.Stderr,
+		GOptions:        opt.GOptions,
+		VerifyOptions:   imageVerifyOpt,
+		IPFSAddress:     opt.IPFSAddress,
+		ProgressHandler: jobs.PrintProgress(opt.Stderr),
 	}
 	// #endregion
 

cmd/nerdctl/image_pull.go

@@ -17,9 +17,12 @@
 package main
 
 import (
+	"fmt"
+
 	"github.com/containerd/nerdctl/pkg/api/types"
 	"github.com/containerd/nerdctl/pkg/clientutil"
 	"github.com/containerd/nerdctl/pkg/cmd/image"
+	"github.com/containerd/nerdctl/pkg/imgutil/jobs"
 	"github.com/spf13/cobra"
 )
 
@@ -94,14 +97,14 @@ func processPullCommandFlags(cmd *cobra.Command) (types.ImagePullOptions, error)
 		return types.ImagePullOptions{}, err
 	}
 	return types.ImagePullOptions{
-		GOptions:      globalOptions,
-		VerifyOptions: verifyOptions,
-		AllPlatforms:  allPlatforms,
-		Platform:      platform,
-		Unpack:        unpackStr,
-		Quiet:         quiet,
-		IPFSAddress:   ipfsAddressStr,
-		Stdout:        cmd.OutOrStderr(),
+		GOptions:        globalOptions,
+		VerifyOptions:   verifyOptions,
+		AllPlatforms:    allPlatforms,
+		Platform:        platform,
+		Unpack:          unpackStr,
+		Quiet:           quiet,
+		IPFSAddress:     ipfsAddressStr,
+		ProgressHandler: jobs.PrintProgress(cmd.OutOrStderr()),
 	}, nil
 }
 
@@ -117,5 +120,9 @@ func pullAction(cmd *cobra.Command, args []string) error {
 	}
 	defer cancel()
 
-	return image.Pull(ctx, client, args[0], options)
+	ref, err := image.Pull(ctx, client, args[0], options)
+	if options.Quiet {
+		fmt.Fprintln(cmd.OutOrStderr(), ref)
+	}
+	return err
 }

cmd/nerdctl/image_push.go

@@ -17,9 +17,12 @@
 package main
 
 import (
+	"fmt"
+
 	"github.com/containerd/nerdctl/pkg/api/types"
 	"github.com/containerd/nerdctl/pkg/clientutil"
 	"github.com/containerd/nerdctl/pkg/cmd/image"
+	"github.com/containerd/nerdctl/pkg/imgutil/jobs"
 	"github.com/spf13/cobra"
 )
 
@@ -111,7 +114,7 @@ func processImagePushOptions(cmd *cobra.Command) (types.ImagePushOptions, error)
 		IpfsAddress:                    ipfsAddress,
 		Quiet:                          quiet,
 		AllowNondistributableArtifacts: allowNonDist,
-		Stdout:                         cmd.OutOrStdout(),
+		ProgressHandler:                jobs.PrintProgress(cmd.OutOrStdout()),
 	}, nil
 }
 
@@ -128,7 +131,11 @@ func pushAction(cmd *cobra.Command, args []string) error {
 	}
 	defer cancel()
 
-	return image.Push(ctx, client, rawRef, options)
+	ref, err := image.Push(ctx, client, rawRef, options)
+	if options.Quiet {
+		fmt.Fprintln(cmd.OutOrStdout(), ref)
+	}
+	return err
 }
 
 func pushShellComplete(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {

pkg/api/types/image_types.go

@@ -153,7 +153,6 @@ type ImageInspectOptions struct {
 
 // ImagePushOptions specifies options for `nerdctl (image) push`.
 type ImagePushOptions struct {
-	Stdout      io.Writer
 	GOptions    GlobalCommandOptions
 	SignOptions ImageSignOptions
 	// Platforms convert content for a specific platform
@@ -169,15 +168,14 @@ type ImagePushOptions struct {
 	IpfsAddress string
 	// Suppress verbose output
 	Quiet bool
-	// If non-nil, the Push job will send upload statuses to the handler instead of Stdout
+	// If non-nil, the Push job will pass upload statuses to the handler in small intervals
 	ProgressHandler jobs.StatusHandler
 	// AllowNondistributableArtifacts allow pushing non-distributable artifacts
 	AllowNondistributableArtifacts bool
 }
 
 // ImagePullOptions specifies options for `nerdctl (image) pull`.
 type ImagePullOptions struct {
-	Stdout        io.Writer
 	GOptions      GlobalCommandOptions
 	VerifyOptions ImageVerifyOptions
 	// Unpack the image for the current single platform (auto/true/false)
@@ -188,7 +186,7 @@ type ImagePullOptions struct {
 	AllPlatforms bool
 	// Suppress verbose output
 	Quiet bool
-	// If non-nil, the Pull job will send download statuses to the handler instead of Stdout
+	// If non-nil, the Push job will pass download statuses to the handler in small intervals
 	ProgressHandler jobs.StatusHandler
 	// multiaddr of IPFS API (default uses $IPFS_PATH env variable if defined or local directory ~/.ipfs)
 	IPFSAddress string

pkg/cmd/image/pull.go

@@ -25,7 +25,6 @@ import (
 	"github.com/containerd/containerd"
 	"github.com/containerd/nerdctl/pkg/api/types"
 	"github.com/containerd/nerdctl/pkg/imgutil"
-	"github.com/containerd/nerdctl/pkg/imgutil/jobs"
 	"github.com/containerd/nerdctl/pkg/ipfs"
 	"github.com/containerd/nerdctl/pkg/platformutil"
 	"github.com/containerd/nerdctl/pkg/referenceutil"
@@ -35,35 +34,29 @@ import (
 )
 
 // Pull pulls an image specified by `rawRef`.
-func Pull(ctx context.Context, client *containerd.Client, rawRef string, options types.ImagePullOptions) error {
+func Pull(ctx context.Context, client *containerd.Client, rawRef string, options types.ImagePullOptions) (imageRef string, err error) {
 	ocispecPlatforms, err := platformutil.NewOCISpecPlatformSlice(options.AllPlatforms, options.Platform)
 	if err != nil {
-		return err
+		return "", err
 	}
 
 	unpack, err := strutil.ParseBoolOrAuto(options.Unpack)
 	if err != nil {
-		return err
+		return "", err
 	}
 
-	_, err = EnsureImage(ctx, client, rawRef, ocispecPlatforms, "always", unpack, options.Quiet, options)
+	img, err := EnsureImage(ctx, client, rawRef, ocispecPlatforms, "always", unpack, options.Quiet, options)
 	if err != nil {
-		return err
+		return "", err
 	}
 
-	return nil
+	return img.Ref, nil
 }
 
 // EnsureImage pulls an image either from ipfs or from registry.
 func EnsureImage(ctx context.Context, client *containerd.Client, rawRef string, ocispecPlatforms []v1.Platform, pull string, unpack *bool, quiet bool, options types.ImagePullOptions) (*imgutil.EnsuredImage, error) {
 	var ensured *imgutil.EnsuredImage
 
-	var progressHandler jobs.StatusHandler
-	if options.ProgressHandler != nil {
-		progressHandler = options.ProgressHandler
-	} else {
-		progressHandler = jobs.PrintProgress(options.Stdout)
-	}
 	pullCfg := imgutil.PullConfig{
 		Ref:             rawRef,
 		Platforms:       ocispecPlatforms,
@@ -73,7 +66,7 @@ func EnsureImage(ctx context.Context, client *containerd.Client, rawRef string,
 		Mode:            pull,
 		Unpack:          unpack,
 		Quiet:           quiet,
-		ProgressHandler: progressHandler,
+		ProgressHandler: options.ProgressHandler,
 	}
 
 	if scheme, ref, err := referenceutil.ParseIPFSRefWithScheme(rawRef); err == nil {

pkg/cmd/image/push.go

@@ -33,7 +33,6 @@ import (
 	"github.com/containerd/nerdctl/pkg/api/types"
 	"github.com/containerd/nerdctl/pkg/errutil"
 	"github.com/containerd/nerdctl/pkg/imgutil/dockerconfigresolver"
-	"github.com/containerd/nerdctl/pkg/imgutil/jobs"
 	"github.com/containerd/nerdctl/pkg/imgutil/push"
 	"github.com/containerd/nerdctl/pkg/ipfs"
 	"github.com/containerd/nerdctl/pkg/platformutil"
@@ -48,22 +47,22 @@ import (
 )
 
 // Push pushes an image specified by `rawRef`.
-func Push(ctx context.Context, client *containerd.Client, rawRef string, options types.ImagePushOptions) error {
+func Push(ctx context.Context, client *containerd.Client, rawRef string, options types.ImagePushOptions) (imageRef string, err error) {
 	if scheme, ref, err := referenceutil.ParseIPFSRefWithScheme(rawRef); err == nil {
 		if scheme != "ipfs" {
-			return fmt.Errorf("ipfs scheme is only supported but got %q", scheme)
+			return "", fmt.Errorf("ipfs scheme is only supported but got %q", scheme)
 		}
 		logrus.Infof("pushing image %q to IPFS", ref)
 
 		var ipfsPath string
 		if options.IpfsAddress != "" {
 			dir, err := os.MkdirTemp("", "apidirtmp")
 			if err != nil {
-				return err
+				return "", err
 			}
 			defer os.RemoveAll(dir)
 			if err := os.WriteFile(filepath.Join(dir, "api"), []byte(options.IpfsAddress), 0600); err != nil {
-				return err
+				return "", err
 			}
 			ipfsPath = dir
 		}
@@ -75,22 +74,21 @@ func Push(ctx context.Context, client *containerd.Client, rawRef string, options
 		c, err := ipfs.Push(ctx, client, ref, layerConvert, options.AllPlatforms, options.Platforms, options.IpfsEnsureImage, ipfsPath)
 		if err != nil {
 			logrus.WithError(err).Warnf("ipfs push failed")
-			return err
+			return "", err
 		}
-		fmt.Fprintln(options.Stdout, c)
-		return nil
+		return c, nil
 	}
 
 	named, err := refdocker.ParseDockerRef(rawRef)
 	if err != nil {
-		return err
+		return "", err
 	}
 	ref := named.String()
 	refDomain := refdocker.Domain(named)
 
 	platMC, err := platformutil.NewMatchComparer(options.AllPlatforms, options.Platforms)
 	if err != nil {
-		return err
+		return "", err
 	}
 	pushRef := ref
 	if !options.AllPlatforms {
@@ -100,9 +98,9 @@ func Push(ctx context.Context, client *containerd.Client, rawRef string, options
 		platImg, err := converter.Convert(ctx, client, pushRef, ref, converter.WithPlatform(platMC))
 		if err != nil {
 			if len(options.Platforms) == 0 {
-				return fmt.Errorf("failed to create a tmp single-platform image %q: %w", pushRef, err)
+				return "", fmt.Errorf("failed to create a tmp single-platform image %q: %w", pushRef, err)
 			}
-			return fmt.Errorf("failed to create a tmp reduced-platform image %q (platform=%v): %w", pushRef, options.Platforms, err)
+			return "", fmt.Errorf("failed to create a tmp reduced-platform image %q (platform=%v): %w", pushRef, options.Platforms, err)
 		}
 		defer client.ImageService().Delete(ctx, platImg.Name, images.SynchronousDelete())
 		logrus.Infof("pushing as a reduced-platform image (%s, %s)", platImg.Target.MediaType, platImg.Target.Digest)
@@ -112,21 +110,14 @@ func Push(ctx context.Context, client *containerd.Client, rawRef string, options
 		pushRef = ref + "-tmp-esgz"
 		esgzImg, err := converter.Convert(ctx, client, pushRef, ref, converter.WithPlatform(platMC), converter.WithLayerConvertFunc(eStargzConvertFunc()))
 		if err != nil {
-			return fmt.Errorf("failed to convert to eStargz: %v", err)
+			return "", fmt.Errorf("failed to convert to eStargz: %v", err)
 		}
 		defer client.ImageService().Delete(ctx, esgzImg.Name, images.SynchronousDelete())
 		logrus.Infof("pushing as an eStargz image (%s, %s)", esgzImg.Target.MediaType, esgzImg.Target.Digest)
 	}
 
-	var progressHandler jobs.StatusHandler
-	if options.ProgressHandler != nil {
-		progressHandler = options.ProgressHandler
-	} else {
-		progressHandler = jobs.PrintProgress(options.Stdout)
-	}
-
 	pushFunc := func(r remotes.Resolver) error {
-		return push.Push(ctx, client, r, progressHandler, pushRef, ref, platMC, options.AllowNondistributableArtifacts, options.Quiet)
+		return push.Push(ctx, client, r, options.ProgressHandler, pushRef, ref, platMC, options.AllowNondistributableArtifacts, options.Quiet)
 	}
 
 	var dOpts []dockerconfigresolver.Opt
@@ -137,45 +128,47 @@ func Push(ctx context.Context, client *containerd.Client, rawRef string, options
 	dOpts = append(dOpts, dockerconfigresolver.WithHostsDirs(options.GOptions.HostsDir))
 	resolver, err := dockerconfigresolver.New(ctx, refDomain, dOpts...)
 	if err != nil {
-		return err
+		return "", err
 	}
 	if err = pushFunc(resolver); err != nil {
 		// In some circumstance (e.g. people just use 80 port to support pure http), the error will contain message like "dial tcp <port>: connection refused"
 		if !errutil.IsErrHTTPResponseToHTTPSClient(err) && !errutil.IsErrConnectionRefused(err) {
-			return err
+			return "", err
 		}
 		if options.GOptions.InsecureRegistry {
 			logrus.WithError(err).Warnf("server %q does not seem to support HTTPS, falling back to plain HTTP", refDomain)
 			dOpts = append(dOpts, dockerconfigresolver.WithPlainHTTP(true))
 			resolver, err = dockerconfigresolver.New(ctx, refDomain, dOpts...)
 			if err != nil {
-				return err
+				return "", err
+			}
+			err = pushFunc(resolver)
+			if err != nil {
+				return "", err
+			} else {
+				return ref, err
 			}
-			return pushFunc(resolver)
 		}
 		logrus.WithError(err).Errorf("server %q does not seem to support HTTPS", refDomain)
 		logrus.Info("Hint: you may want to try --insecure-registry to allow plain HTTP (if you are in a trusted network)")
-		return err
+		return "", err
 	}
 
 	img, err := client.ImageService().Get(ctx, pushRef)
 	if err != nil {
-		return err
+		return "", err
 	}
 	refSpec, err := reference.Parse(pushRef)
 	if err != nil {
-		return err
+		return "", err
 	}
 	signRef := fmt.Sprintf("%s@%s", refSpec.String(), img.Target.Digest.String())
 	if err = signutil.Sign(signRef,
 		options.GOptions.Experimental,
 		options.SignOptions); err != nil {
-		return err
-	}
-	if options.Quiet {
-		fmt.Fprintln(options.Stdout, ref)
+		return "", err
 	}
-	return nil
+	return ref, nil
 }
 
 func eStargzConvertFunc() converter.ConvertFunc {

pkg/imgutil/pull/pull.go

@@ -92,10 +92,10 @@ func Pull(ctx context.Context, client *containerd.Client, ref string, config *Co
 		img = containerd.NewImageWithPlatform(client, imagesImg, platformMC)
 	}
 	stopProgress()
+	<-progress
 	if err != nil {
 		return nil, err
 	}
 
-	<-progress
 	return img, nil
 }