From f8be8829d56bb66b43a3ec189cd6d50694a6471c Mon Sep 17 00:00:00 2001
From: Billy Zha <jinzha1@microsoft.com>
Date: Fri, 29 Dec 2023 15:01:52 +0800
Subject: [PATCH] chore(ux): improve error output for unknown spec flag (#1221)

Signed-off-by: Billy Zha <jinzha1@microsoft.com>
---
 cmd/oras/internal/option/remote.go |  5 +-
 cmd/oras/internal/option/spec.go   | 98 +++++++++++++++++++++++-------
 test/e2e/internal/utils/flags.go   | 10 ++-
 test/e2e/suite/command/attach.go   | 12 ++++
 test/e2e/suite/command/push.go     | 51 +++++++++++++++-
 5 files changed, 144 insertions(+), 32 deletions(-)

diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go
index 26c01fc90..5547569e5 100644
--- a/cmd/oras/internal/option/remote.go
+++ b/cmd/oras/internal/option/remote.go
@@ -116,10 +116,7 @@ func (opts *Remote) Parse() error {
 	if err := opts.parseCustomHeaders(); err != nil {
 		return err
 	}
-	if err := opts.readPassword(); err != nil {
-		return err
-	}
-	return opts.DistributionSpec.Parse()
+	return opts.readPassword()
 }
 
 // readPassword tries to read password with optional cmd prompt.
diff --git a/cmd/oras/internal/option/spec.go b/cmd/oras/internal/option/spec.go
index 965eb929b..4a51a820d 100644
--- a/cmd/oras/internal/option/spec.go
+++ b/cmd/oras/internal/option/spec.go
@@ -17,9 +17,11 @@ package option
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/spf13/pflag"
 	"oras.land/oras-go/v2"
+	oerrors "oras.land/oras/cmd/oras/internal/errors"
 )
 
 const (
@@ -27,59 +29,109 @@ const (
 	ImageSpecV1_0 = "v1.0"
 )
 
-// ImageSpec option struct.
+const (
+	DistributionSpecReferrersTagV1_1 = "v1.1-referrers-tag"
+	DistributionSpecReferrersAPIV1_1 = "v1.1-referrers-api"
+)
+
+// ImageSpec option struct which implements pflag.Value interface.
 type ImageSpec struct {
 	flag        string
 	PackVersion oras.PackManifestVersion
 }
 
-// Parse parses flags into the option.
-func (opts *ImageSpec) Parse() error {
-	switch opts.flag {
+// Set validates and sets the flag value from a string argument.
+func (is *ImageSpec) Set(value string) error {
+	is.flag = value
+	switch value {
 	case ImageSpecV1_1:
-		opts.PackVersion = oras.PackManifestVersion1_1_RC4
+		is.PackVersion = oras.PackManifestVersion1_1_RC4
 	case ImageSpecV1_0:
-		opts.PackVersion = oras.PackManifestVersion1_0
+		is.PackVersion = oras.PackManifestVersion1_0
 	default:
-		return fmt.Errorf("unknown image specification flag: %q", opts.flag)
+		return &oerrors.Error{
+			Err:            fmt.Errorf("unknown image specification flag: %s", value),
+			Recommendation: fmt.Sprintf("Available options: %s", is.Options()),
+		}
 	}
 	return nil
 }
 
+// Type returns the string value of the inner flag.
+func (is *ImageSpec) Type() string {
+	return "string"
+}
+
+// Options returns the string of usable options for the flag.
+func (is *ImageSpec) Options() string {
+	return strings.Join([]string{
+		ImageSpecV1_1,
+		ImageSpecV1_0,
+	}, ", ")
+}
+
+// String returns the string representation of the flag.
+func (is *ImageSpec) String() string {
+	return is.flag
+}
+
 // ApplyFlags applies flags to a command flag set.
-func (opts *ImageSpec) ApplyFlags(fs *pflag.FlagSet) {
-	fs.StringVar(&opts.flag, "image-spec", ImageSpecV1_1, fmt.Sprintf("[Experimental] specify manifest type for building artifact. options: %s, %s", ImageSpecV1_1, ImageSpecV1_0))
+func (is *ImageSpec) ApplyFlags(fs *pflag.FlagSet) {
+	// default to v1.1-rc.4
+	is.PackVersion = oras.PackManifestVersion1_1_RC4
+	defaultFlag := ImageSpecV1_1
+	fs.Var(is, "image-spec", fmt.Sprintf(`[Experimental] specify manifest type for building artifact. Options: %s (default %q)`, is.Options(), defaultFlag))
 }
 
-// DistributionSpec option struct.
+// DistributionSpec option struct which implements pflag.Value interface.
 type DistributionSpec struct {
 	// ReferrersAPI indicates the preference of the implementation of the Referrers API.
 	// Set to true for referrers API, false for referrers tag scheme, and nil for auto fallback.
 	ReferrersAPI *bool
 
 	// specFlag should be provided in form of`<version>-<api>-<option>`
-	specFlag string
+	flag string
 }
 
-// Parse parses flags into the option.
-func (opts *DistributionSpec) Parse() error {
-	switch opts.specFlag {
-	case "":
-		opts.ReferrersAPI = nil
-	case "v1.1-referrers-tag":
+// Set validates and sets the flag value from a string argument.
+func (ds *DistributionSpec) Set(value string) error {
+	ds.flag = value
+	switch ds.flag {
+	case DistributionSpecReferrersTagV1_1:
 		isApi := false
-		opts.ReferrersAPI = &isApi
-	case "v1.1-referrers-api":
+		ds.ReferrersAPI = &isApi
+	case DistributionSpecReferrersAPIV1_1:
 		isApi := true
-		opts.ReferrersAPI = &isApi
+		ds.ReferrersAPI = &isApi
 	default:
-		return fmt.Errorf("unknown distribution specification flag: %q", opts.specFlag)
+		return &oerrors.Error{
+			Err:            fmt.Errorf("unknown distribution specification flag: %s", value),
+			Recommendation: fmt.Sprintf("Available options: %s", ds.Options()),
+		}
 	}
 	return nil
 }
 
+// Type returns the string value of the inner flag.
+func (ds *DistributionSpec) Type() string {
+	return "string"
+}
+
+// Options returns the string of usable options for the flag.
+func (ds *DistributionSpec) Options() string {
+	return strings.Join([]string{
+		DistributionSpecReferrersTagV1_1,
+		DistributionSpecReferrersAPIV1_1,
+	}, ", ")
+}
+
+// String returns the string representation of the flag.
+func (ds *DistributionSpec) String() string {
+	return ds.flag
+}
+
 // ApplyFlagsWithPrefix applies flags to a command flag set with a prefix string.
-func (opts *DistributionSpec) ApplyFlagsWithPrefix(fs *pflag.FlagSet, prefix, description string) {
+func (ds *DistributionSpec) ApplyFlagsWithPrefix(fs *pflag.FlagSet, prefix, description string) {
 	flagPrefix, notePrefix := applyPrefix(prefix, description)
-	fs.StringVar(&opts.specFlag, flagPrefix+"distribution-spec", "", "[Preview] set OCI distribution spec version and API option for "+notePrefix+"target. options: v1.1-referrers-api, v1.1-referrers-tag")
+	fs.Var(ds, flagPrefix+"distribution-spec", fmt.Sprintf("[Preview] set OCI distribution spec version and API option for "+notePrefix+"target. Options: ", ds.Options()))
 }
diff --git a/test/e2e/internal/utils/flags.go b/test/e2e/internal/utils/flags.go
index 8263e49f9..893fa575d 100644
--- a/test/e2e/internal/utils/flags.go
+++ b/test/e2e/internal/utils/flags.go
@@ -16,11 +16,15 @@ limitations under the License.
 package utils
 
 var Flags = struct {
-	Layout     string
-	FromLayout string
-	ToLayout   string
+	Layout           string
+	FromLayout       string
+	ToLayout         string
+	DistributionSpec string
+	ImageSpec        string
 }{
 	"--oci-layout",
 	"--from-oci-layout",
 	"--to-oci-layout",
+	"--distribution-spec",
+	"--image-spec",
 }
diff --git a/test/e2e/suite/command/attach.go b/test/e2e/suite/command/attach.go
index 63fc1fea4..269337b04 100644
--- a/test/e2e/suite/command/attach.go
+++ b/test/e2e/suite/command/attach.go
@@ -72,6 +72,18 @@ var _ = Describe("ORAS beginners:", func() {
 			gomega.Expect(err).Should(gbytes.Say("\n"))
 			gomega.Expect(err).Should(gbytes.Say(`Run "oras attach -h"`))
 		})
+
+		It("should fail if distribution spec is not valid", func() {
+			testRepo := attachTestRepo("invalid-image-spec")
+			CopyZOTRepo(ImageRepo, testRepo)
+			subjectRef := RegistryRef(ZOTHost, testRepo, foobar.Tag)
+			invalidFlag := "???"
+			ORAS("attach", "--artifact-type", "test/attach", subjectRef, fmt.Sprintf("%s:%s", foobar.AttachFileName, foobar.AttachFileMedia), Flags.DistributionSpec, invalidFlag).
+				ExpectFailure().
+				WithWorkDir(PrepareTempFiles()).
+				MatchErrKeyWords("Error:", invalidFlag, "Available options: v1.1-referrers-tag, v1.1-referrers-api").
+				Exec()
+		})
 	})
 })
 
diff --git a/test/e2e/suite/command/push.go b/test/e2e/suite/command/push.go
index 123f6a0bd..1126c4c81 100644
--- a/test/e2e/suite/command/push.go
+++ b/test/e2e/suite/command/push.go
@@ -56,6 +56,36 @@ var _ = Describe("ORAS beginners:", func() {
 
 			ORAS("push", ref, "--config", foobar.FileConfigName, "--artifact-type", "test/artifact+json", "--image-spec", "v1.0").ExpectFailure().WithWorkDir(tempDir).Exec()
 		})
+
+		It("should fail if image spec is not valid", func() {
+			testRepo := attachTestRepo("invalid-image-spec")
+			subjectRef := RegistryRef(ZOTHost, testRepo, foobar.Tag)
+			invalidFlag := "???"
+			ORAS("push", subjectRef, Flags.ImageSpec, invalidFlag).
+				ExpectFailure().
+				MatchErrKeyWords("Error:", invalidFlag, "Available options: v1.1, v1.0").
+				Exec()
+		})
+
+		It("should fail if image spec is not valid", func() {
+			testRepo := attachTestRepo("invalid-image-spec")
+			subjectRef := RegistryRef(ZOTHost, testRepo, foobar.Tag)
+			invalidFlag := "???"
+			ORAS("push", subjectRef, Flags.ImageSpec, invalidFlag).
+				ExpectFailure().
+				MatchErrKeyWords("Error:", invalidFlag, "Available options: v1.1, v1.0").
+				Exec()
+		})
+
+		It("should fail if image spec is not valid", func() {
+			testRepo := attachTestRepo("invalid-image-spec")
+			subjectRef := RegistryRef(ZOTHost, testRepo, foobar.Tag)
+			invalidFlag := "???"
+			ORAS("push", subjectRef, Flags.ImageSpec, invalidFlag).
+				ExpectFailure().
+				MatchErrKeyWords("Error:", invalidFlag, "Available options: v1.1, v1.0").
+				Exec()
+		})
 	})
 })
 
@@ -270,7 +300,7 @@ var _ = Describe("Remote registry users:", func() {
 			Expect(manifest.Annotations[annotationKey]).Should(Equal(annotationValue))
 		})
 
-		It("should push artifact with blob", func() {
+		It("should push files", func() {
 			repo := pushTestRepo("artifact-with-blob")
 			tempDir := PrepareTempFiles()
 
@@ -282,7 +312,24 @@ var _ = Describe("Remote registry users:", func() {
 			fetched := ORAS("manifest", "fetch", RegistryRef(ZOTHost, repo, tag)).Exec().Out.Contents()
 			var manifest ocispec.Manifest
 			Expect(json.Unmarshal(fetched, &manifest)).ShouldNot(HaveOccurred())
-			Expect(manifest.ArtifactType).Should(Equal(artifact.DefaultArtifactType))
+			Expect(manifest.ArtifactType).Should(Equal("application/vnd.unknown.artifact.v1"))
+			Expect(manifest.Layers).Should(ContainElements(foobar.BlobBarDescriptor("application/vnd.oci.image.layer.v1.tar")))
+			Expect(manifest.Config).Should(Equal(artifact.EmptyLayerJSON))
+		})
+
+		It("should push v1.1-rc.4 artifact", func() {
+			repo := pushTestRepo("v1.1-artifact")
+			tempDir := PrepareTempFiles()
+
+			ORAS("push", RegistryRef(ZOTHost, repo, tag), foobar.FileBarName, "-v", "--image-spec", "v1.1").
+				MatchStatus([]match.StateKey{foobar.FileBarStateKey, artifact.DefaultConfigStateKey}, true, 2).
+				WithWorkDir(tempDir).Exec()
+
+			// validate
+			fetched := ORAS("manifest", "fetch", RegistryRef(ZOTHost, repo, tag)).Exec().Out.Contents()
+			var manifest ocispec.Manifest
+			Expect(json.Unmarshal(fetched, &manifest)).ShouldNot(HaveOccurred())
+			Expect(manifest.ArtifactType).Should(Equal("application/vnd.unknown.artifact.v1"))
 			Expect(manifest.Layers).Should(ContainElements(foobar.BlobBarDescriptor("application/vnd.oci.image.layer.v1.tar")))
 			Expect(manifest.Config).Should(Equal(artifact.EmptyLayerJSON))
 		})