From d21c31f4c0a2b995df03beb2f022f38e467bf462 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang <103478229+wangxiaoxuan273@users.noreply.github.com> Date: Fri, 19 Jan 2024 09:51:17 +0800 Subject: [PATCH] chore(ux): improve error message for oras manifest push (#1241) Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/manifest/manifest.go | 42 +++++++++++++ cmd/oras/internal/manifest/manifest_test.go | 65 +++++++++++++++++++++ cmd/oras/root/manifest/push.go | 16 +++-- internal/file/file.go | 15 ----- internal/file/file_test.go | 40 ------------- test/e2e/suite/command/manifest.go | 8 +++ 6 files changed, 127 insertions(+), 59 deletions(-) create mode 100644 cmd/oras/internal/manifest/manifest.go create mode 100644 cmd/oras/internal/manifest/manifest_test.go diff --git a/cmd/oras/internal/manifest/manifest.go b/cmd/oras/internal/manifest/manifest.go new file mode 100644 index 000000000..426e8f8ef --- /dev/null +++ b/cmd/oras/internal/manifest/manifest.go @@ -0,0 +1,42 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifest + +import ( + "encoding/json" + "errors" +) + +var ( + // ErrMediaTypeNotFound is returned when the media type is not specified. + ErrMediaTypeNotFound = errors.New(`media type is not specified`) + // ErrInvalidJSON is returned when the json file is malformed. + ErrInvalidJSON = errors.New("not a valid json file") +) + +// ExtractMediaType parses the media type field of bytes content in json format. +func ExtractMediaType(content []byte) (string, error) { + var manifest struct { + MediaType string `json:"mediaType"` + } + if err := json.Unmarshal(content, &manifest); err != nil { + return "", ErrInvalidJSON + } + if manifest.MediaType == "" { + return "", ErrMediaTypeNotFound + } + return manifest.MediaType, nil +} diff --git a/cmd/oras/internal/manifest/manifest_test.go b/cmd/oras/internal/manifest/manifest_test.go new file mode 100644 index 000000000..a74054951 --- /dev/null +++ b/cmd/oras/internal/manifest/manifest_test.go @@ -0,0 +1,65 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifest + +import ( + "errors" + "reflect" + "testing" +) + +const ( + manifest = `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json","config":{"mediaType":"application/vnd.unknown.config.v1+json","digest":"sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a","size":2},"layers":[{"mediaType":"application/vnd.oci.image.layer.v1.tar","digest":"sha256:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03","size":6,"annotations":{"org.opencontainers.image.title":"hello.txt"}}]}` + manifestMediaType = "application/vnd.oci.image.manifest.v1+json" +) + +func Test_ExtractMediaType(t *testing.T) { + // generate test content + content := []byte(manifest) + + // test ExtractMediaType + want := manifestMediaType + got, err := ExtractMediaType(content) + if err != nil { + t.Fatal("ExtractMediaType() error=", err) + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ExtractMediaType() = %v, want %v", got, want) + } +} + +func Test_ExtractMediaType_invalidContent_notAJson(t *testing.T) { + // generate test content + content := []byte("manifest") + + // test ExtractMediaType + _, err := ExtractMediaType(content) + expected := "not a valid json file" + if err.Error() != expected { + t.Fatalf("ExtractMediaType() error = %v, wantErr %v", err, expected) + } +} + +func Test_ExtractMediaType_invalidContent_missingMediaType(t *testing.T) { + // generate test content + content := []byte(`{"schemaVersion":2}`) + + // test ExtractMediaType + _, err := ExtractMediaType(content) + if !errors.Is(err, ErrMediaTypeNotFound) { + t.Fatalf("ExtractMediaType() error = %v, wantErr %v", err, ErrMediaTypeNotFound) + } +} diff --git a/cmd/oras/root/manifest/push.go b/cmd/oras/root/manifest/push.go index 2043b3625..74e8135b5 100644 --- a/cmd/oras/root/manifest/push.go +++ b/cmd/oras/root/manifest/push.go @@ -31,6 +31,7 @@ import ( "oras.land/oras/cmd/oras/internal/argument" "oras.land/oras/cmd/oras/internal/display" oerrors "oras.land/oras/cmd/oras/internal/errors" + "oras.land/oras/cmd/oras/internal/manifest" "oras.land/oras/cmd/oras/internal/option" "oras.land/oras/internal/file" ) @@ -94,7 +95,7 @@ Example - Push a manifest to an OCI image layout folder 'layout-dir' and tag wit return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { - return pushManifest(cmd.Context(), opts) + return pushManifest(cmd, opts) }, } @@ -105,8 +106,8 @@ Example - Push a manifest to an OCI image layout folder 'layout-dir' and tag wit return oerrors.Command(cmd, &opts.Target) } -func pushManifest(ctx context.Context, opts pushOptions) error { - ctx, logger := opts.WithContext(ctx) +func pushManifest(cmd *cobra.Command, opts pushOptions) error { + ctx, logger := opts.WithContext(cmd.Context()) var target oras.Target var err error target, err = opts.NewTarget(opts.Common, logger) @@ -126,8 +127,15 @@ func pushManifest(ctx context.Context, opts pushOptions) error { // get manifest media type mediaType := opts.mediaType if opts.mediaType == "" { - mediaType, err = file.ParseMediaType(contentBytes) + mediaType, err = manifest.ExtractMediaType(contentBytes) if err != nil { + if errors.Is(err, manifest.ErrMediaTypeNotFound) { + return &oerrors.Error{ + Err: fmt.Errorf(`%w via the flag "--media-type" nor in %q`, err, opts.fileRef), + Usage: fmt.Sprintf("%s %s", cmd.Parent().CommandPath(), cmd.Use), + Recommendation: `Please specify a valid media type in the manifest JSON or via the "--media-type" flag`, + } + } return err } } diff --git a/internal/file/file.go b/internal/file/file.go index e1e9cb37e..6386fde1d 100644 --- a/internal/file/file.go +++ b/internal/file/file.go @@ -16,7 +16,6 @@ limitations under the License. package file import ( - "encoding/json" "errors" "fmt" "io" @@ -117,17 +116,3 @@ func PrepareBlobContent(path string, mediaType string, dgstStr string, size int6 Size: actualSize, }, file, nil } - -// ParseMediaType parses the media type field of bytes content in json format. -func ParseMediaType(content []byte) (string, error) { - var manifest struct { - MediaType string `json:"mediaType"` - } - if err := json.Unmarshal(content, &manifest); err != nil { - return "", errors.New("not a valid json file") - } - if manifest.MediaType == "" { - return "", errors.New("media type is not recognized") - } - return manifest.MediaType, nil -} diff --git a/internal/file/file_test.go b/internal/file/file_test.go index 42697fa3e..cb3c64e54 100644 --- a/internal/file/file_test.go +++ b/internal/file/file_test.go @@ -30,7 +30,6 @@ import ( "oras.land/oras/internal/file" ) -const manifestMediaType = "application/vnd.oci.image.manifest.v1+json" const blobMediaType = "application/mock-octet-stream" const manifest = `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json","config":{"mediaType":"application/vnd.unknown.config.v1+json","digest":"sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a","size":2},"layers":[{"mediaType":"application/vnd.oci.image.layer.v1.tar","digest":"sha256:5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03","size":6,"annotations":{"org.opencontainers.image.title":"hello.txt"}}]}` @@ -267,42 +266,3 @@ func TestFile_PrepareBlobContent_errOpenFile(t *testing.T) { t.Fatalf("PrepareBlobContent() error = %v, wantErr %v", err, expected) } } - -func TestFile_ParseMediaType(t *testing.T) { - // generate test content - content := []byte(manifest) - - // test ParseMediaType - want := manifestMediaType - got, err := file.ParseMediaType(content) - if err != nil { - t.Fatal("ParseMediaType() error=", err) - } - if !reflect.DeepEqual(got, want) { - t.Errorf("ParseMediaType() = %v, want %v", got, want) - } -} - -func TestFile_ParseMediaType_invalidContent_notAJson(t *testing.T) { - // generate test content - content := []byte("manifest") - - // test ParseMediaType - _, err := file.ParseMediaType(content) - expected := "not a valid json file" - if err.Error() != expected { - t.Fatalf("ParseMediaType() error = %v, wantErr %v", err, expected) - } -} - -func TestFile_ParseMediaType_invalidContent_missingMediaType(t *testing.T) { - // generate test content - content := []byte(`{"schemaVersion":2}`) - - // test ParseMediaType - _, err := file.ParseMediaType(content) - expected := "media type is not recognized" - if err.Error() != expected { - t.Fatalf("ParseMediaType() error = %v, wantErr %v", err, expected) - } -} diff --git a/test/e2e/suite/command/manifest.go b/test/e2e/suite/command/manifest.go index 442394a27..f7e52ad01 100644 --- a/test/e2e/suite/command/manifest.go +++ b/test/e2e/suite/command/manifest.go @@ -295,6 +295,7 @@ var _ = Describe("1.1 registry users:", func() { When("running `manifest push`", func() { manifest := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json","config":{"mediaType":"application/vnd.oci.image.config.v1+json","digest":"sha256:fe9dbc99451d0517d65e048c309f0b5afb2cc513b7a3d456b6cc29fe641386c5","size":53},"layers":[]}` + manifestWithoutMediaType := `{"schemaVersion":2,"mediaType":"","config":{"mediaType":"application/vnd.oci.image.config.v1+json","digest":"sha256:fe9dbc99451d0517d65e048c309f0b5afb2cc513b7a3d456b6cc29fe641386c5","size":53},"layers":[]}` digest := "sha256:bc1a59d49fc7c7b0a31f22ca0c743ecdabdb736777e3d9672fa9d97b4fe323f4" descriptor := "{\"mediaType\":\"application/vnd.oci.image.manifest.v1+json\",\"digest\":\"sha256:bc1a59d49fc7c7b0a31f22ca0c743ecdabdb736777e3d9672fa9d97b4fe323f4\",\"size\":247}" @@ -319,6 +320,13 @@ var _ = Describe("1.1 registry users:", func() { MatchKeyWords("Pushed", RegistryRef(ZOTHost, ImageRepo, tag), "Digest:", digest). WithInput(strings.NewReader(manifest)).Exec() }) + + It("should fail to push manifest without media type with suggestion", func() { + manifestPath := WriteTempFile("manifest.json", manifestWithoutMediaType) + tag := "from-file" + ORAS("manifest", "push", RegistryRef(ZOTHost, ImageRepo, tag), manifestPath). + WithInput(strings.NewReader(manifest)).ExpectFailure().MatchErrKeyWords("Error:", " media type is not specified", "oras manifest push").Exec() + }) }) When("running `manifest fetch-config`", func() {