From a4a4b331f903efa89772e1ac5d45bec9b07a4b4f Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 5 Jul 2024 00:13:29 +0800 Subject: [PATCH] chore(ux): improve error message when attaching without subject artifact (#1430) Signed-off-by: Billy Zha --- cmd/oras/internal/errors/errors.go | 11 +++++++++++ cmd/oras/internal/option/target.go | 9 ++++++--- cmd/oras/root/attach.go | 21 ++++++++++++++------- test/e2e/suite/command/attach.go | 21 +++++++++++++++++---- 4 files changed, 48 insertions(+), 14 deletions(-) diff --git a/cmd/oras/internal/errors/errors.go b/cmd/oras/internal/errors/errors.go index 6e48556ef..a19d2459b 100644 --- a/cmd/oras/internal/errors/errors.go +++ b/cmd/oras/internal/errors/errors.go @@ -26,6 +26,15 @@ import ( "oras.land/oras-go/v2/registry/remote/errcode" ) +// OperationType stands for certain type of operations. +type OperationType int + +const ( + // OperationTypeParseArtifactReference represents parsing artifact + // reference operation. + OperationTypeParseArtifactReference OperationType = iota + 1 +) + // RegistryErrorPrefix is the commandline prefix for errors from registry. const RegistryErrorPrefix = "Error response from registry:" @@ -39,6 +48,7 @@ func (e UnsupportedFormatTypeError) Error() string { // Error is the error type for CLI error messaging. type Error struct { + OperationType OperationType Err error Usage string Recommendation string @@ -160,6 +170,7 @@ func NewErrEmptyTagOrDigest(ref string, cmd *cobra.Command, needsTag bool) error errMsg = "no tag or digest specified" } return &Error{ + OperationType: OperationTypeParseArtifactReference, Err: fmt.Errorf(`"%s": %s`, ref, errMsg), Usage: fmt.Sprintf("%s %s", cmd.Parent().CommandPath(), cmd.Use), Recommendation: fmt.Sprintf(`Please specify a reference in the form of %s. Run "%s -h" for more options and examples`, form, cmd.CommandPath()), diff --git a/cmd/oras/internal/option/target.go b/cmd/oras/internal/option/target.go index b15613b13..b81c52b26 100644 --- a/cmd/oras/internal/option/target.go +++ b/cmd/oras/internal/option/target.go @@ -105,11 +105,14 @@ func (opts *Target) Parse(cmd *cobra.Command) error { return opts.parseOCILayoutReference() default: opts.Type = TargetTypeRemote - if _, err := registry.ParseReference(opts.RawReference); err != nil { + if ref, err := registry.ParseReference(opts.RawReference); err != nil { return &oerrors.Error{ + OperationType: oerrors.OperationTypeParseArtifactReference, Err: fmt.Errorf("%q: %w", opts.RawReference, err), Recommendation: "Please make sure the provided reference is in the form of /[:tag|@digest]", } + } else { + opts.Reference = ref.Reference } return opts.Remote.Parse(cmd) } @@ -243,9 +246,9 @@ func (opts *Target) NewReadonlyTarget(ctx context.Context, common Common, logger } // EnsureReferenceNotEmpty returns formalized error when the reference is empty. -func (opts *Target) EnsureReferenceNotEmpty(cmd *cobra.Command, needsTag bool) error { +func (opts *Target) EnsureReferenceNotEmpty(cmd *cobra.Command, allowTag bool) error { if opts.Reference == "" { - return oerrors.NewErrEmptyTagOrDigest(opts.RawReference, cmd, needsTag) + return oerrors.NewErrEmptyTagOrDigest(opts.RawReference, cmd, allowTag) } return nil } diff --git a/cmd/oras/root/attach.go b/cmd/oras/root/attach.go index 48d819689..88f829779 100644 --- a/cmd/oras/root/attach.go +++ b/cmd/oras/root/attach.go @@ -49,7 +49,7 @@ type attachOptions struct { func attachCmd() *cobra.Command { var opts attachOptions cmd := &cobra.Command{ - Use: "attach [flags] --artifact-type= {:|@} [:] [...]", + Use: "attach [flags] --artifact-type= {:|@} {[:]|--annotation =} [...]", Short: "[Preview] Attach files to an existing artifact", Long: `[Preview] Attach files to an existing artifact @@ -87,10 +87,20 @@ Example - Attach file to the manifest tagged 'v1' in an OCI image layout folder PreRunE: func(cmd *cobra.Command, args []string) error { opts.RawReference = args[0] opts.FileRefs = args[1:] - if err := option.Parse(cmd, &opts); err != nil { - return err + err := option.Parse(cmd, &opts) + if err == nil { + if err = opts.EnsureReferenceNotEmpty(cmd, true); err == nil { + return nil + } + } + if len(opts.FileRefs) == 0 { + // no file argument provided + if err, ok := err.(*oerrors.Error); ok && err.OperationType == oerrors.OperationTypeParseArtifactReference { + // invalid reference + err.Recommendation = fmt.Sprintf("Are you missing an artifact reference to attach to? %s", err.Recommendation) + } } - return nil + return err }, RunE: func(cmd *cobra.Command, args []string) error { return runAttach(cmd, &opts) @@ -137,9 +147,6 @@ func runAttach(cmd *cobra.Command, opts *attachOptions) error { if err != nil { return err } - if err := opts.EnsureReferenceNotEmpty(cmd, true); err != nil { - return err - } // add both pull and push scope hints for dst repository // to save potential push-scope token requests during copy ctx = registryutil.WithScopeHint(ctx, dst, auth.ActionPull, auth.ActionPush) diff --git a/test/e2e/suite/command/attach.go b/test/e2e/suite/command/attach.go index 026d11575..f0c6ada5b 100644 --- a/test/e2e/suite/command/attach.go +++ b/test/e2e/suite/command/attach.go @@ -69,12 +69,25 @@ var _ = Describe("ORAS beginners:", func() { ExpectFailure().MatchErrKeyWords("unknown distribution specification flag").Exec() }) + It("should fail with error suggesting subject missed", func() { + err := ORAS("attach", "--artifact-type", "oras/test", RegistryRef(ZOTHost, ImageRepo, "")).ExpectFailure().Exec().Err + Expect(err).Should(gbytes.Say("Error")) + Expect(err).Should(gbytes.Say("\nAre you missing an artifact reference to attach to?")) + }) + + It("should fail with error suggesting right form", func() { + err := ORAS("attach", "--artifact-type", "oras/test", RegistryRef(ZOTHost, ImageRepo, ""), "./test.json").ExpectFailure().Exec().Err + Expect(err).Should(gbytes.Say("Error")) + Expect(err).Should(gbytes.Say("no tag or digest specified")) + Expect(err).ShouldNot(gbytes.Say("\nAre you missing an artifact reference to attach to?")) + }) + It("should fail and show detailed error description if no argument provided", func() { err := ORAS("attach").ExpectFailure().Exec().Err - gomega.Expect(err).Should(gbytes.Say("Error")) - gomega.Expect(err).Should(gbytes.Say("\nUsage: oras attach")) - gomega.Expect(err).Should(gbytes.Say("\n")) - gomega.Expect(err).Should(gbytes.Say(`Run "oras attach -h"`)) + Expect(err).Should(gbytes.Say("Error")) + Expect(err).Should(gbytes.Say("\nUsage: oras attach")) + Expect(err).Should(gbytes.Say("\n")) + Expect(err).Should(gbytes.Say(`Run "oras attach -h"`)) }) It("should fail if distribution spec is not valid", func() {