From 904e991d8a689051cef031d9fd4bfde738376fdc Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Fri, 13 Sep 2024 15:40:41 +0800 Subject: [PATCH 01/13] moved annotation out of packer Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/option/annotation.go | 91 +++++++++++++++++++++++++ cmd/oras/internal/option/packer.go | 59 +--------------- cmd/oras/internal/option/packer_test.go | 42 +++++++++--- cmd/oras/root/attach_test.go | 6 +- 4 files changed, 130 insertions(+), 68 deletions(-) create mode 100644 cmd/oras/internal/option/annotation.go diff --git a/cmd/oras/internal/option/annotation.go b/cmd/oras/internal/option/annotation.go new file mode 100644 index 000000000..c306b09ad --- /dev/null +++ b/cmd/oras/internal/option/annotation.go @@ -0,0 +1,91 @@ +/* +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 option + +import ( + "errors" + "fmt" + "strings" + + "github.com/spf13/pflag" + oerrors "oras.land/oras/cmd/oras/internal/errors" +) + +// Pre-defined annotation keys for annotation file +const ( + AnnotationManifest = "$manifest" + AnnotationConfig = "$config" +) + +var ( + errAnnotationConflict = errors.New("`--annotation` and `--annotation-file` cannot be both specified") + errAnnotationFormat = errors.New("annotation value doesn't match the required format") + errAnnotationDuplication = errors.New("duplicate annotation key") +) + +// Packer option struct. +type Annotation struct { + AnnotationFilePath string + ManifestAnnotations []string +} + +// ApplyFlags applies flags to a command flag set. +func (opts *Annotation) ApplyFlags(fs *pflag.FlagSet) { + fs.StringArrayVarP(&opts.ManifestAnnotations, "annotation", "a", nil, "manifest annotations") + fs.StringVarP(&opts.AnnotationFilePath, "annotation-file", "", "", "path of the annotation file") +} + +// LoadManifestAnnotations loads the manifest annotation map. +func (opts *Annotation) LoadManifestAnnotations() (annotations map[string]map[string]string, err error) { + if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotations) != 0 { + return nil, errAnnotationConflict + } + if opts.AnnotationFilePath != "" { + if err = decodeJSON(opts.AnnotationFilePath, &annotations); err != nil { + return nil, &oerrors.Error{ + Err: fmt.Errorf(`invalid annotation json file: failed to load annotations from %s`, opts.AnnotationFilePath), + Recommendation: `Annotation file doesn't match the required format. Please refer to the document at https://oras.land/docs/how_to_guides/manifest_annotations`, + } + } + } + if len(opts.ManifestAnnotations) != 0 { + annotations = make(map[string]map[string]string) + if err = parseAnnotationFlags(opts.ManifestAnnotations, annotations); err != nil { + return nil, err + } + } + return +} + +// parseAnnotationFlags parses annotation flags into a map. +func parseAnnotationFlags(flags []string, annotations map[string]map[string]string) error { + manifestAnnotations := make(map[string]string) + for _, anno := range flags { + key, val, success := strings.Cut(anno, "=") + if !success { + return &oerrors.Error{ + Err: errAnnotationFormat, + Recommendation: `Please use the correct format in the flag: --annotation "key=value"`, + } + } + if _, ok := manifestAnnotations[key]; ok { + return fmt.Errorf("%w: %v, ", errAnnotationDuplication, key) + } + manifestAnnotations[key] = val + } + annotations[AnnotationManifest] = manifestAnnotations + return nil +} diff --git a/cmd/oras/internal/option/packer.go b/cmd/oras/internal/option/packer.go index 7217df5ee..a427d9b3b 100644 --- a/cmd/oras/internal/option/packer.go +++ b/cmd/oras/internal/option/packer.go @@ -28,29 +28,18 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" "oras.land/oras-go/v2/content" - oerrors "oras.land/oras/cmd/oras/internal/errors" "oras.land/oras/cmd/oras/internal/fileref" ) -// Pre-defined annotation keys for annotation file -const ( - AnnotationManifest = "$manifest" - AnnotationConfig = "$config" -) - var ( - errAnnotationConflict = errors.New("`--annotation` and `--annotation-file` cannot be both specified") - errAnnotationFormat = errors.New("annotation value doesn't match the required format") - errAnnotationDuplication = errors.New("duplicate annotation key") - errPathValidation = errors.New("absolute file path detected. If it's intentional, use --disable-path-validation flag to skip this check") + errPathValidation = errors.New("absolute file path detected. If it's intentional, use --disable-path-validation flag to skip this check") ) // Packer option struct. type Packer struct { + Annotation ManifestExportPath string PathValidationDisabled bool - AnnotationFilePath string - ManifestAnnotations []string FileRefs []string } @@ -58,8 +47,6 @@ type Packer struct { // ApplyFlags applies flags to a command flag set. func (opts *Packer) ApplyFlags(fs *pflag.FlagSet) { fs.StringVarP(&opts.ManifestExportPath, "export-manifest", "", "", "`path` of the pushed manifest") - fs.StringArrayVarP(&opts.ManifestAnnotations, "annotation", "a", nil, "manifest annotations") - fs.StringVarP(&opts.AnnotationFilePath, "annotation-file", "", "", "path of the annotation file") fs.BoolVarP(&opts.PathValidationDisabled, "disable-path-validation", "", false, "skip path validation") } @@ -94,28 +81,6 @@ func (opts *Packer) Parse(*cobra.Command) error { return nil } -// LoadManifestAnnotations loads the manifest annotation map. -func (opts *Packer) LoadManifestAnnotations() (annotations map[string]map[string]string, err error) { - if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotations) != 0 { - return nil, errAnnotationConflict - } - if opts.AnnotationFilePath != "" { - if err = decodeJSON(opts.AnnotationFilePath, &annotations); err != nil { - return nil, &oerrors.Error{ - Err: fmt.Errorf(`invalid annotation json file: failed to load annotations from %s`, opts.AnnotationFilePath), - Recommendation: `Annotation file doesn't match the required format. Please refer to the document at https://oras.land/docs/how_to_guides/manifest_annotations`, - } - } - } - if len(opts.ManifestAnnotations) != 0 { - annotations = make(map[string]map[string]string) - if err = parseAnnotationFlags(opts.ManifestAnnotations, annotations); err != nil { - return nil, err - } - } - return -} - // decodeJSON decodes a json file v to filename. func decodeJSON(filename string, v interface{}) error { file, err := os.Open(filename) @@ -125,23 +90,3 @@ func decodeJSON(filename string, v interface{}) error { defer file.Close() return json.NewDecoder(file).Decode(v) } - -// parseAnnotationFlags parses annotation flags into a map. -func parseAnnotationFlags(flags []string, annotations map[string]map[string]string) error { - manifestAnnotations := make(map[string]string) - for _, anno := range flags { - key, val, success := strings.Cut(anno, "=") - if !success { - return &oerrors.Error{ - Err: errAnnotationFormat, - Recommendation: `Please use the correct format in the flag: --annotation "key=value"`, - } - } - if _, ok := manifestAnnotations[key]; ok { - return fmt.Errorf("%w: %v, ", errAnnotationDuplication, key) - } - manifestAnnotations[key] = val - } - annotations[AnnotationManifest] = manifestAnnotations - return nil -} diff --git a/cmd/oras/internal/option/packer_test.go b/cmd/oras/internal/option/packer_test.go index e0cd51339..fd6e47f91 100644 --- a/cmd/oras/internal/option/packer_test.go +++ b/cmd/oras/internal/option/packer_test.go @@ -39,29 +39,37 @@ func TestPacker_FlagInit(t *testing.T) { func TestPacker_LoadManifestAnnotations_err(t *testing.T) { opts := Packer{ - AnnotationFilePath: "this is not a file", // testFile, - ManifestAnnotations: []string{"Key=Val"}, + Annotation: Annotation{ + AnnotationFilePath: "this is not a file", // testFile, + ManifestAnnotations: []string{"Key=Val"}, + }, } if _, err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationConflict) { t.Fatalf("unexpected error: %v", err) } opts = Packer{ - AnnotationFilePath: "this is not a file", // testFile, + Annotation: Annotation{ + AnnotationFilePath: "this is not a file", // testFile, + }, } if _, err := opts.LoadManifestAnnotations(); err == nil { t.Fatalf("unexpected error: %v", err) } opts = Packer{ - ManifestAnnotations: []string{"KeyVal"}, + Annotation: Annotation{ + ManifestAnnotations: []string{"KeyVal"}, + }, } if _, err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationFormat) { t.Fatalf("unexpected error: %v", err) } opts = Packer{ - ManifestAnnotations: []string{"Key=Val1", "Key=Val2"}, + Annotation: Annotation{ + ManifestAnnotations: []string{"Key=Val1", "Key=Val2"}, + }, } if _, err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationDuplication) { t.Fatalf("unexpected error: %v", err) @@ -74,7 +82,11 @@ func TestPacker_LoadManifestAnnotations_annotationFile(t *testing.T) { if err != nil { t.Fatalf("Error writing %s: %v", testFile, err) } - opts := Packer{AnnotationFilePath: testFile} + opts := Packer{ + Annotation: Annotation{ + AnnotationFilePath: testFile, + }, + } anno, err := opts.LoadManifestAnnotations() if err != nil { @@ -91,7 +103,11 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { "Key", } var annotations map[string]map[string]string - opts := Packer{ManifestAnnotations: invalidFlag0} + opts := Packer{ + Annotation: Annotation{ + ManifestAnnotations: invalidFlag0, + }, + } _, err := opts.LoadManifestAnnotations() if !errors.Is(err, errAnnotationFormat) { t.Fatalf("unexpected error: %v", err) @@ -102,7 +118,11 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { "Key=0", "Key=1", } - opts = Packer{ManifestAnnotations: invalidFlag1} + opts = Packer{ + Annotation: Annotation{ + ManifestAnnotations: invalidFlag1, + }, + } _, err = opts.LoadManifestAnnotations() if !errors.Is(err, errAnnotationDuplication) { t.Fatalf("unexpected error: %v", err) @@ -114,7 +134,11 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { "Key1=Val", // 2. Normal Item "Key2=${env:USERNAME}", // 3. Item contains variable eg. "${env:USERNAME}" } - opts = Packer{ManifestAnnotations: validFlag} + opts = Packer{ + Annotation: Annotation{ + ManifestAnnotations: validFlag, + }, + } annotations, err = opts.LoadManifestAnnotations() if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/cmd/oras/root/attach_test.go b/cmd/oras/root/attach_test.go index 7bc666377..8fec6325a 100644 --- a/cmd/oras/root/attach_test.go +++ b/cmd/oras/root/attach_test.go @@ -33,8 +33,10 @@ func Test_runAttach_errType(t *testing.T) { // test opts := &attachOptions{ Packer: option.Packer{ - AnnotationFilePath: "/tmp/whatever", - ManifestAnnotations: []string{"one", "two"}, + Annotation: option.Annotation{ + AnnotationFilePath: "/tmp/whatever", + ManifestAnnotations: []string{"one", "two"}, + }, }, } got := runAttach(cmd, opts).Error() From d7cb2536c117a66d4a3e3bf0453a127cfd492f34 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Fri, 13 Sep 2024 16:00:47 +0800 Subject: [PATCH 02/13] fix apply flags Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/option/packer.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/oras/internal/option/packer.go b/cmd/oras/internal/option/packer.go index a427d9b3b..3c599189a 100644 --- a/cmd/oras/internal/option/packer.go +++ b/cmd/oras/internal/option/packer.go @@ -46,6 +46,7 @@ type Packer struct { // ApplyFlags applies flags to a command flag set. func (opts *Packer) ApplyFlags(fs *pflag.FlagSet) { + opts.Annotation.ApplyFlags(fs) fs.StringVarP(&opts.ManifestExportPath, "export-manifest", "", "", "`path` of the pushed manifest") fs.BoolVarP(&opts.PathValidationDisabled, "disable-path-validation", "", false, "skip path validation") } From 87817c8a30e04c21bc8390881ef5af54a097e130 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Sat, 14 Sep 2024 15:50:43 +0800 Subject: [PATCH 03/13] implement annotations Signed-off-by: Xiaoxuan Wang --- cmd/oras/root/manifest/index/create.go | 32 ++++++++++++++++++++---- test/e2e/suite/command/manifest_index.go | 26 +++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/cmd/oras/root/manifest/index/create.go b/cmd/oras/root/manifest/index/create.go index 1d6164d5d..ee1b3e657 100644 --- a/cmd/oras/root/manifest/index/create.go +++ b/cmd/oras/root/manifest/index/create.go @@ -48,9 +48,11 @@ type createOptions struct { option.Target option.Pretty - sources []string - extraRefs []string - outputPath string + sources []string + extraRefs []string + rawAnnotations []string + indexAnnotations map[string]string + outputPath string } func createCmd() *cobra.Command { @@ -72,6 +74,9 @@ Example - Create an index from source manifests using both tags and digests, and Example - Create an index and push it with multiple tags: oras manifest index create localhost:5000/hello:tag1,tag2,tag3 linux-amd64 linux-arm64 sha256:99e4703fbf30916f549cd6bfa9cdbab614b5392fbe64fdee971359a77073cdf9 +Example - Create and push an index with annotations: + oras manifest index create localhost:5000/hello:v1 linux-amd64 --annotation "key=val" + Example - Create an index and push to an OCI image layout folder 'layout-dir' and tag with 'v1': oras manifest index create layout-dir:v1 linux-amd64 sha256:99e4703fbf30916f549cd6bfa9cdbab614b5392fbe64fdee971359a77073cdf9 @@ -87,6 +92,10 @@ Example - Create an index and output the index to stdout, auto push will be disa opts.RawReference = refs[0] opts.extraRefs = refs[1:] opts.sources = args[1:] + opts.indexAnnotations = make(map[string]string) + if err := parseAnnotations(opts.rawAnnotations, opts.indexAnnotations); err != nil { + return err + } return option.Parse(cmd, &opts) }, Aliases: []string{"pack"}, @@ -95,6 +104,7 @@ Example - Create an index and output the index to stdout, auto push will be disa }, } cmd.Flags().StringVarP(&opts.outputPath, "output", "o", "", "file `path` to write the created index to, use - for stdout") + cmd.Flags().StringArrayVarP(&opts.rawAnnotations, "annotation", "a", nil, "index annotations") option.ApplyFlags(&opts, cmd.Flags()) return oerrors.Command(cmd, &opts.Target) } @@ -113,8 +123,9 @@ func createIndex(cmd *cobra.Command, opts createOptions) error { Versioned: specs.Versioned{ SchemaVersion: 2, }, - MediaType: ocispec.MediaTypeImageIndex, - Manifests: manifests, + MediaType: ocispec.MediaTypeImageIndex, + Manifests: manifests, + Annotations: opts.indexAnnotations, } indexBytes, err := json.Marshal(index) if err != nil { @@ -204,3 +215,14 @@ func pushIndex(ctx context.Context, target oras.Target, desc ocispec.Descriptor, } return printer.Println("Digest:", desc.Digest) } + +func parseAnnotations(input []string, annotations map[string]string) error { + for _, anno := range input { + key, val, success := strings.Cut(anno, "=") + if !success { + return fmt.Errorf("annotation value doesn't match the required format of \"key=value\"") + } + annotations[key] = val + } + return nil +} diff --git a/test/e2e/suite/command/manifest_index.go b/test/e2e/suite/command/manifest_index.go index 95eee3165..336e2bae5 100644 --- a/test/e2e/suite/command/manifest_index.go +++ b/test/e2e/suite/command/manifest_index.go @@ -137,6 +137,19 @@ var _ = Describe("1.1 registry users:", func() { ValidateIndex(content, expectedManifests) }) + It("should create index with annotations", func() { + testRepo := indexTestRepo("create", "with-annotations") + key := "image-anno-key" + value := "image-anno-value" + CopyZOTRepo(ImageRepo, testRepo) + ORAS("manifest", "index", "create", RegistryRef(ZOTHost, testRepo, "v1"), "--annotation", fmt.Sprintf("%s=%s", key, value)).Exec() + // verify + content := ORAS("manifest", "fetch", RegistryRef(ZOTHost, testRepo, "v1")).Exec().Out.Contents() + var manifest ocispec.Manifest + Expect(json.Unmarshal(content, &manifest)).ShouldNot(HaveOccurred()) + Expect(manifest.Annotations[key]).To(Equal(value)) + }) + It("should output created index to file", func() { testRepo := indexTestRepo("create", "output-to-file") CopyZOTRepo(ImageRepo, testRepo) @@ -374,6 +387,19 @@ var _ = Describe("OCI image layout users:", func() { ValidateIndex(content, expectedManifests) }) + It("should create index with annotations", func() { + root := PrepareTempOCI(ImageRepo) + indexRef := LayoutRef(root, "with-annotations") + key := "image-anno-key" + value := "image-anno-value" + ORAS("manifest", "index", "create", Flags.Layout, indexRef, "--annotation", fmt.Sprintf("%s=%s", key, value)).Exec() + // verify + content := ORAS("manifest", "fetch", Flags.Layout, indexRef).Exec().Out.Contents() + var manifest ocispec.Manifest + Expect(json.Unmarshal(content, &manifest)).ShouldNot(HaveOccurred()) + Expect(manifest.Annotations[key]).To(Equal(value)) + }) + It("should output created index to file", func() { root := PrepareTempOCI(ImageRepo) indexRef := LayoutRef(root, "output-to-file") From 168a0115d25a514b5b9d8ef6d82ab480c2b0625b Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 18 Sep 2024 11:18:49 +0800 Subject: [PATCH 04/13] added tests Signed-off-by: Xiaoxuan Wang --- cmd/oras/root/manifest/index/create_test.go | 56 +++++++++++++++++++++ test/e2e/suite/command/manifest_index.go | 8 +++ 2 files changed, 64 insertions(+) create mode 100644 cmd/oras/root/manifest/index/create_test.go diff --git a/cmd/oras/root/manifest/index/create_test.go b/cmd/oras/root/manifest/index/create_test.go new file mode 100644 index 000000000..4b2d16f5d --- /dev/null +++ b/cmd/oras/root/manifest/index/create_test.go @@ -0,0 +1,56 @@ +/* +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 index + +import ( + "reflect" + "testing" +) + +func Test_parseAnnotations(t *testing.T) { + tests := []struct { + name string + input []string + annotations map[string]string + wantErr bool + wantAnnotations map[string]string + }{ + { + name: "valid input", + input: []string{"a=b", "c=d", "e=f"}, + annotations: make(map[string]string), + wantErr: false, + wantAnnotations: map[string]string{"a": "b", "c": "d", "e": "f"}, + }, + { + name: "invalid input", + input: []string{"c:d", "e=f"}, + annotations: make(map[string]string), + wantErr: true, + wantAnnotations: map[string]string{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := parseAnnotations(tt.input, tt.annotations); (err != nil) != tt.wantErr { + t.Errorf("parseAnnotations() error = %v, wantErr %v", err, tt.wantErr) + } + if !reflect.DeepEqual(tt.annotations, tt.wantAnnotations) { + t.Errorf("parseAnnotations() annotations = %v, want %v", tt.annotations, tt.wantAnnotations) + } + }) + } +} diff --git a/test/e2e/suite/command/manifest_index.go b/test/e2e/suite/command/manifest_index.go index 336e2bae5..e90a8f2ed 100644 --- a/test/e2e/suite/command/manifest_index.go +++ b/test/e2e/suite/command/manifest_index.go @@ -172,6 +172,14 @@ var _ = Describe("1.1 registry users:", func() { "does-not-exist").ExpectFailure(). MatchErrKeyWords("Error", "could not find", "does-not-exist").Exec() }) + + It("should fail if given annotation input of wrong format", func() { + testRepo := indexTestRepo("create", "bad-annotations") + CopyZOTRepo(ImageRepo, testRepo) + ORAS("manifest", "index", "create", RegistryRef(ZOTHost, testRepo, ""), + string(multi_arch.LinuxAMD64.Digest), "-a", "foo:bar").ExpectFailure(). + MatchErrKeyWords("Error", "annotation value doesn't match the required format").Exec() + }) }) When("running `manifest index update`", func() { From 9ea1f267d6b9ec27d868a68fc3f184ee6fe27ab2 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 18 Sep 2024 11:34:11 +0800 Subject: [PATCH 05/13] improved code style Signed-off-by: Xiaoxuan Wang --- cmd/oras/root/manifest/index/create.go | 12 +++++++----- cmd/oras/root/manifest/index/create_test.go | 11 +++++------ 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/cmd/oras/root/manifest/index/create.go b/cmd/oras/root/manifest/index/create.go index ee1b3e657..8c50231a0 100644 --- a/cmd/oras/root/manifest/index/create.go +++ b/cmd/oras/root/manifest/index/create.go @@ -92,8 +92,9 @@ Example - Create an index and output the index to stdout, auto push will be disa opts.RawReference = refs[0] opts.extraRefs = refs[1:] opts.sources = args[1:] - opts.indexAnnotations = make(map[string]string) - if err := parseAnnotations(opts.rawAnnotations, opts.indexAnnotations); err != nil { + var err error + opts.indexAnnotations, err = parseAnnotations(opts.rawAnnotations) + if err != nil { return err } return option.Parse(cmd, &opts) @@ -216,13 +217,14 @@ func pushIndex(ctx context.Context, target oras.Target, desc ocispec.Descriptor, return printer.Println("Digest:", desc.Digest) } -func parseAnnotations(input []string, annotations map[string]string) error { +func parseAnnotations(input []string) (map[string]string, error) { + annotations := make(map[string]string) for _, anno := range input { key, val, success := strings.Cut(anno, "=") if !success { - return fmt.Errorf("annotation value doesn't match the required format of \"key=value\"") + return nil, fmt.Errorf("annotation value doesn't match the required format of \"key=value\"") } annotations[key] = val } - return nil + return annotations, nil } diff --git a/cmd/oras/root/manifest/index/create_test.go b/cmd/oras/root/manifest/index/create_test.go index 4b2d16f5d..b08f28a25 100644 --- a/cmd/oras/root/manifest/index/create_test.go +++ b/cmd/oras/root/manifest/index/create_test.go @@ -31,24 +31,23 @@ func Test_parseAnnotations(t *testing.T) { { name: "valid input", input: []string{"a=b", "c=d", "e=f"}, - annotations: make(map[string]string), wantErr: false, wantAnnotations: map[string]string{"a": "b", "c": "d", "e": "f"}, }, { name: "invalid input", - input: []string{"c:d", "e=f"}, - annotations: make(map[string]string), + input: []string{"a=b", "c:d", "e=f"}, wantErr: true, - wantAnnotations: map[string]string{}, + wantAnnotations: nil, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := parseAnnotations(tt.input, tt.annotations); (err != nil) != tt.wantErr { + annotations, err := parseAnnotations(tt.input) + if (err != nil) != tt.wantErr { t.Errorf("parseAnnotations() error = %v, wantErr %v", err, tt.wantErr) } - if !reflect.DeepEqual(tt.annotations, tt.wantAnnotations) { + if !reflect.DeepEqual(annotations, tt.wantAnnotations) { t.Errorf("parseAnnotations() annotations = %v, want %v", tt.annotations, tt.wantAnnotations) } }) From 70fc42174f9ae16993c54fe4ccfd477b16e834a0 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 18 Sep 2024 12:02:58 +0800 Subject: [PATCH 06/13] refinement Signed-off-by: Xiaoxuan Wang --- cmd/oras/root/manifest/index/create.go | 3 +++ cmd/oras/root/manifest/index/create_test.go | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/cmd/oras/root/manifest/index/create.go b/cmd/oras/root/manifest/index/create.go index 8c50231a0..18cf241ee 100644 --- a/cmd/oras/root/manifest/index/create.go +++ b/cmd/oras/root/manifest/index/create.go @@ -224,6 +224,9 @@ func parseAnnotations(input []string) (map[string]string, error) { if !success { return nil, fmt.Errorf("annotation value doesn't match the required format of \"key=value\"") } + if _, ok := annotations[key]; ok { + return nil, fmt.Errorf("duplicate annotation key: %v", key) + } annotations[key] = val } return annotations, nil diff --git a/cmd/oras/root/manifest/index/create_test.go b/cmd/oras/root/manifest/index/create_test.go index b08f28a25..7b5bd2c5a 100644 --- a/cmd/oras/root/manifest/index/create_test.go +++ b/cmd/oras/root/manifest/index/create_test.go @@ -40,6 +40,12 @@ func Test_parseAnnotations(t *testing.T) { wantErr: true, wantAnnotations: nil, }, + { + name: "duplicate key", + input: []string{"a=b", "c=d", "a=e"}, + wantErr: true, + wantAnnotations: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 1b79560f52db48c4c04093b5ef148e438851c5b0 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 19 Sep 2024 15:08:08 +0800 Subject: [PATCH 07/13] refactor done Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/option/annotation.go | 24 ----------------------- cmd/oras/internal/option/packer.go | 27 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/cmd/oras/internal/option/annotation.go b/cmd/oras/internal/option/annotation.go index c306b09ad..9baaca603 100644 --- a/cmd/oras/internal/option/annotation.go +++ b/cmd/oras/internal/option/annotation.go @@ -38,36 +38,12 @@ var ( // Packer option struct. type Annotation struct { - AnnotationFilePath string ManifestAnnotations []string } // ApplyFlags applies flags to a command flag set. func (opts *Annotation) ApplyFlags(fs *pflag.FlagSet) { fs.StringArrayVarP(&opts.ManifestAnnotations, "annotation", "a", nil, "manifest annotations") - fs.StringVarP(&opts.AnnotationFilePath, "annotation-file", "", "", "path of the annotation file") -} - -// LoadManifestAnnotations loads the manifest annotation map. -func (opts *Annotation) LoadManifestAnnotations() (annotations map[string]map[string]string, err error) { - if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotations) != 0 { - return nil, errAnnotationConflict - } - if opts.AnnotationFilePath != "" { - if err = decodeJSON(opts.AnnotationFilePath, &annotations); err != nil { - return nil, &oerrors.Error{ - Err: fmt.Errorf(`invalid annotation json file: failed to load annotations from %s`, opts.AnnotationFilePath), - Recommendation: `Annotation file doesn't match the required format. Please refer to the document at https://oras.land/docs/how_to_guides/manifest_annotations`, - } - } - } - if len(opts.ManifestAnnotations) != 0 { - annotations = make(map[string]map[string]string) - if err = parseAnnotationFlags(opts.ManifestAnnotations, annotations); err != nil { - return nil, err - } - } - return } // parseAnnotationFlags parses annotation flags into a map. diff --git a/cmd/oras/internal/option/packer.go b/cmd/oras/internal/option/packer.go index 3c599189a..78caa241b 100644 --- a/cmd/oras/internal/option/packer.go +++ b/cmd/oras/internal/option/packer.go @@ -28,6 +28,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" "oras.land/oras-go/v2/content" + oerrors "oras.land/oras/cmd/oras/internal/errors" "oras.land/oras/cmd/oras/internal/fileref" ) @@ -38,8 +39,10 @@ var ( // Packer option struct. type Packer struct { Annotation + ManifestExportPath string PathValidationDisabled bool + AnnotationFilePath string FileRefs []string } @@ -47,7 +50,9 @@ type Packer struct { // ApplyFlags applies flags to a command flag set. func (opts *Packer) ApplyFlags(fs *pflag.FlagSet) { opts.Annotation.ApplyFlags(fs) + fs.StringVarP(&opts.ManifestExportPath, "export-manifest", "", "", "`path` of the pushed manifest") + fs.StringVarP(&opts.AnnotationFilePath, "annotation-file", "", "", "path of the annotation file") fs.BoolVarP(&opts.PathValidationDisabled, "disable-path-validation", "", false, "skip path validation") } @@ -82,6 +87,28 @@ func (opts *Packer) Parse(*cobra.Command) error { return nil } +// LoadManifestAnnotations loads the manifest annotation map. +func (opts *Packer) LoadManifestAnnotations() (annotations map[string]map[string]string, err error) { + if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotations) != 0 { + return nil, errAnnotationConflict + } + if opts.AnnotationFilePath != "" { + if err = decodeJSON(opts.AnnotationFilePath, &annotations); err != nil { + return nil, &oerrors.Error{ + Err: fmt.Errorf(`invalid annotation json file: failed to load annotations from %s`, opts.AnnotationFilePath), + Recommendation: `Annotation file doesn't match the required format. Please refer to the document at https://oras.land/docs/how_to_guides/manifest_annotations`, + } + } + } + if len(opts.ManifestAnnotations) != 0 { + annotations = make(map[string]map[string]string) + if err = parseAnnotationFlags(opts.ManifestAnnotations, annotations); err != nil { + return nil, err + } + } + return +} + // decodeJSON decodes a json file v to filename. func decodeJSON(filename string, v interface{}) error { file, err := os.Open(filename) From 6217c17a91a871e6cb5322f8ac4e5bafb57aaea6 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 19 Sep 2024 15:57:35 +0800 Subject: [PATCH 08/13] unit test works Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/option/annotation.go | 12 +++------ cmd/oras/internal/option/packer.go | 25 +++++++++++------- cmd/oras/internal/option/packer_test.go | 35 +++++++++++-------------- cmd/oras/root/attach.go | 10 +++---- cmd/oras/root/attach_test.go | 6 ++--- cmd/oras/root/push.go | 10 +++---- 6 files changed, 43 insertions(+), 55 deletions(-) diff --git a/cmd/oras/internal/option/annotation.go b/cmd/oras/internal/option/annotation.go index 9baaca603..c7bdf53c1 100644 --- a/cmd/oras/internal/option/annotation.go +++ b/cmd/oras/internal/option/annotation.go @@ -24,14 +24,7 @@ import ( oerrors "oras.land/oras/cmd/oras/internal/errors" ) -// Pre-defined annotation keys for annotation file -const ( - AnnotationManifest = "$manifest" - AnnotationConfig = "$config" -) - var ( - errAnnotationConflict = errors.New("`--annotation` and `--annotation-file` cannot be both specified") errAnnotationFormat = errors.New("annotation value doesn't match the required format") errAnnotationDuplication = errors.New("duplicate annotation key") ) @@ -39,6 +32,7 @@ var ( // Packer option struct. type Annotation struct { ManifestAnnotations []string + Annotations map[string]map[string]string } // ApplyFlags applies flags to a command flag set. @@ -47,7 +41,7 @@ func (opts *Annotation) ApplyFlags(fs *pflag.FlagSet) { } // parseAnnotationFlags parses annotation flags into a map. -func parseAnnotationFlags(flags []string, annotations map[string]map[string]string) error { +func (opts *Annotation) parseAnnotationFlags(flags []string) error { manifestAnnotations := make(map[string]string) for _, anno := range flags { key, val, success := strings.Cut(anno, "=") @@ -62,6 +56,6 @@ func parseAnnotationFlags(flags []string, annotations map[string]map[string]stri } manifestAnnotations[key] = val } - annotations[AnnotationManifest] = manifestAnnotations + opts.Annotations[AnnotationManifest] = manifestAnnotations return nil } diff --git a/cmd/oras/internal/option/packer.go b/cmd/oras/internal/option/packer.go index 78caa241b..5b345b100 100644 --- a/cmd/oras/internal/option/packer.go +++ b/cmd/oras/internal/option/packer.go @@ -32,8 +32,15 @@ import ( "oras.land/oras/cmd/oras/internal/fileref" ) +// Pre-defined annotation keys for annotation file +const ( + AnnotationManifest = "$manifest" + AnnotationConfig = "$config" +) + var ( - errPathValidation = errors.New("absolute file path detected. If it's intentional, use --disable-path-validation flag to skip this check") + errAnnotationConflict = errors.New("`--annotation` and `--annotation-file` cannot be both specified") + errPathValidation = errors.New("absolute file path detected. If it's intentional, use --disable-path-validation flag to skip this check") ) // Packer option struct. @@ -84,26 +91,26 @@ func (opts *Packer) Parse(*cobra.Command) error { return fmt.Errorf("%w: %v", errPathValidation, strings.Join(failedPaths, ", ")) } } - return nil + return opts.LoadManifestAnnotations() } // LoadManifestAnnotations loads the manifest annotation map. -func (opts *Packer) LoadManifestAnnotations() (annotations map[string]map[string]string, err error) { +func (opts *Packer) LoadManifestAnnotations() (err error) { if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotations) != 0 { - return nil, errAnnotationConflict + return errAnnotationConflict } if opts.AnnotationFilePath != "" { - if err = decodeJSON(opts.AnnotationFilePath, &annotations); err != nil { - return nil, &oerrors.Error{ + if err = decodeJSON(opts.AnnotationFilePath, &opts.Annotations); err != nil { + return &oerrors.Error{ Err: fmt.Errorf(`invalid annotation json file: failed to load annotations from %s`, opts.AnnotationFilePath), Recommendation: `Annotation file doesn't match the required format. Please refer to the document at https://oras.land/docs/how_to_guides/manifest_annotations`, } } } if len(opts.ManifestAnnotations) != 0 { - annotations = make(map[string]map[string]string) - if err = parseAnnotationFlags(opts.ManifestAnnotations, annotations); err != nil { - return nil, err + opts.Annotations = make(map[string]map[string]string) + if err = opts.parseAnnotationFlags(opts.ManifestAnnotations); err != nil { + return err } } return diff --git a/cmd/oras/internal/option/packer_test.go b/cmd/oras/internal/option/packer_test.go index fd6e47f91..852336838 100644 --- a/cmd/oras/internal/option/packer_test.go +++ b/cmd/oras/internal/option/packer_test.go @@ -40,20 +40,18 @@ func TestPacker_FlagInit(t *testing.T) { func TestPacker_LoadManifestAnnotations_err(t *testing.T) { opts := Packer{ Annotation: Annotation{ - AnnotationFilePath: "this is not a file", // testFile, ManifestAnnotations: []string{"Key=Val"}, }, + AnnotationFilePath: "this is not a file", // testFile, } - if _, err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationConflict) { + if err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationConflict) { t.Fatalf("unexpected error: %v", err) } opts = Packer{ - Annotation: Annotation{ - AnnotationFilePath: "this is not a file", // testFile, - }, + AnnotationFilePath: "this is not a file", // testFile, } - if _, err := opts.LoadManifestAnnotations(); err == nil { + if err := opts.LoadManifestAnnotations(); err == nil { t.Fatalf("unexpected error: %v", err) } @@ -62,7 +60,7 @@ func TestPacker_LoadManifestAnnotations_err(t *testing.T) { ManifestAnnotations: []string{"KeyVal"}, }, } - if _, err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationFormat) { + if err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationFormat) { t.Fatalf("unexpected error: %v", err) } @@ -71,7 +69,7 @@ func TestPacker_LoadManifestAnnotations_err(t *testing.T) { ManifestAnnotations: []string{"Key=Val1", "Key=Val2"}, }, } - if _, err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationDuplication) { + if err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationDuplication) { t.Fatalf("unexpected error: %v", err) } } @@ -83,17 +81,15 @@ func TestPacker_LoadManifestAnnotations_annotationFile(t *testing.T) { t.Fatalf("Error writing %s: %v", testFile, err) } opts := Packer{ - Annotation: Annotation{ - AnnotationFilePath: testFile, - }, + AnnotationFilePath: testFile, } - anno, err := opts.LoadManifestAnnotations() + err = opts.LoadManifestAnnotations() if err != nil { t.Fatalf("unexpected error: %v", err) } - if !reflect.DeepEqual(anno, expectedResult) { - t.Fatalf("unexpected error: %v", anno) + if !reflect.DeepEqual(opts.Annotations, expectedResult) { + t.Fatalf("unexpected error: %v", opts.Annotations) } } @@ -102,13 +98,12 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { invalidFlag0 := []string{ "Key", } - var annotations map[string]map[string]string opts := Packer{ Annotation: Annotation{ ManifestAnnotations: invalidFlag0, }, } - _, err := opts.LoadManifestAnnotations() + err := opts.LoadManifestAnnotations() if !errors.Is(err, errAnnotationFormat) { t.Fatalf("unexpected error: %v", err) } @@ -123,7 +118,7 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { ManifestAnnotations: invalidFlag1, }, } - _, err = opts.LoadManifestAnnotations() + err = opts.LoadManifestAnnotations() if !errors.Is(err, errAnnotationDuplication) { t.Fatalf("unexpected error: %v", err) } @@ -139,14 +134,14 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { ManifestAnnotations: validFlag, }, } - annotations, err = opts.LoadManifestAnnotations() + err = opts.LoadManifestAnnotations() if err != nil { t.Fatalf("unexpected error: %v", err) } - if _, ok := annotations["$manifest"]; !ok { + if _, ok := opts.Annotations["$manifest"]; !ok { t.Fatalf("unexpected error: failed when looking for '$manifest' in annotations") } - if !reflect.DeepEqual(annotations, + if !reflect.DeepEqual(opts.Annotations, map[string]map[string]string{ "$manifest": { "Key0": "", diff --git a/cmd/oras/root/attach.go b/cmd/oras/root/attach.go index 37d2ddf98..1531927d5 100644 --- a/cmd/oras/root/attach.go +++ b/cmd/oras/root/attach.go @@ -125,11 +125,7 @@ Example - Attach file to the manifest tagged 'v1' in an OCI image layout folder func runAttach(cmd *cobra.Command, opts *attachOptions) error { ctx, logger := command.GetLogger(cmd, &opts.Common) - annotations, err := opts.LoadManifestAnnotations() - if err != nil { - return err - } - if len(opts.FileRefs) == 0 && len(annotations[option.AnnotationManifest]) == 0 { + if len(opts.FileRefs) == 0 && len(opts.Annotations[option.AnnotationManifest]) == 0 { return &oerrors.Error{ Err: errors.New(`neither file nor annotation provided in the command`), Usage: fmt.Sprintf("%s %s", cmd.Parent().CommandPath(), cmd.Use), @@ -161,7 +157,7 @@ func runAttach(cmd *cobra.Command, opts *attachOptions) error { if err != nil { return err } - descs, err := loadFiles(ctx, store, annotations, opts.FileRefs, displayStatus) + descs, err := loadFiles(ctx, store, opts.Annotations, opts.FileRefs, displayStatus) if err != nil { return err } @@ -179,7 +175,7 @@ func runAttach(cmd *cobra.Command, opts *attachOptions) error { packOpts := oras.PackManifestOptions{ Subject: &subject, - ManifestAnnotations: annotations[option.AnnotationManifest], + ManifestAnnotations: opts.Annotations[option.AnnotationManifest], Layers: descs, } pack := func() (ocispec.Descriptor, error) { diff --git a/cmd/oras/root/attach_test.go b/cmd/oras/root/attach_test.go index 8fec6325a..895bb30be 100644 --- a/cmd/oras/root/attach_test.go +++ b/cmd/oras/root/attach_test.go @@ -26,7 +26,7 @@ import ( ) func Test_runAttach_errType(t *testing.T) { - // prpare + // prepare cmd := &cobra.Command{} cmd.SetContext(context.Background()) @@ -34,12 +34,12 @@ func Test_runAttach_errType(t *testing.T) { opts := &attachOptions{ Packer: option.Packer{ Annotation: option.Annotation{ - AnnotationFilePath: "/tmp/whatever", ManifestAnnotations: []string{"one", "two"}, }, + AnnotationFilePath: "/tmp/whatever", }, } - got := runAttach(cmd, opts).Error() + got := opts.Packer.Parse(cmd).Error() want := errors.New("`--annotation` and `--annotation-file` cannot be both specified").Error() if got != want { t.Fatalf("got %v, want %v", got, want) diff --git a/cmd/oras/root/push.go b/cmd/oras/root/push.go index f8afa6a3c..92ad30e5e 100644 --- a/cmd/oras/root/push.go +++ b/cmd/oras/root/push.go @@ -157,15 +157,11 @@ Example - Push file "hi.txt" into an OCI image layout folder 'layout-dir' with t func runPush(cmd *cobra.Command, opts *pushOptions) error { ctx, logger := command.GetLogger(cmd, &opts.Common) - annotations, err := opts.LoadManifestAnnotations() - if err != nil { - return err - } // prepare pack packOpts := oras.PackManifestOptions{ - ConfigAnnotations: annotations[option.AnnotationConfig], - ManifestAnnotations: annotations[option.AnnotationManifest], + ConfigAnnotations: opts.Annotations[option.AnnotationConfig], + ManifestAnnotations: opts.Annotations[option.AnnotationManifest], } store, err := file.New("") if err != nil { @@ -190,7 +186,7 @@ func runPush(cmd *cobra.Command, opts *pushOptions) error { if err != nil { return err } - descs, err := loadFiles(ctx, store, annotations, opts.FileRefs, displayStatus) + descs, err := loadFiles(ctx, store, opts.Annotations, opts.FileRefs, displayStatus) if err != nil { return err } From fa2a2e7293c8ff7d86f7e8b5f8f8b8fe9d5d96cb Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 19 Sep 2024 16:20:20 +0800 Subject: [PATCH 09/13] done refactoring Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/option/annotation.go | 13 +++++----- cmd/oras/internal/option/packer.go | 17 +++++-------- cmd/oras/internal/option/packer_test.go | 34 ++++++++++++------------- cmd/oras/root/attach_test.go | 2 +- 4 files changed, 32 insertions(+), 34 deletions(-) diff --git a/cmd/oras/internal/option/annotation.go b/cmd/oras/internal/option/annotation.go index c7bdf53c1..61b35384e 100644 --- a/cmd/oras/internal/option/annotation.go +++ b/cmd/oras/internal/option/annotation.go @@ -20,6 +20,7 @@ import ( "fmt" "strings" + "github.com/spf13/cobra" "github.com/spf13/pflag" oerrors "oras.land/oras/cmd/oras/internal/errors" ) @@ -31,19 +32,19 @@ var ( // Packer option struct. type Annotation struct { - ManifestAnnotations []string - Annotations map[string]map[string]string + ManifestAnnotationFlags []string // raw input of manifest annotation flags + Annotations map[string]map[string]string // parsed manifest and config annotations } // ApplyFlags applies flags to a command flag set. func (opts *Annotation) ApplyFlags(fs *pflag.FlagSet) { - fs.StringArrayVarP(&opts.ManifestAnnotations, "annotation", "a", nil, "manifest annotations") + fs.StringArrayVarP(&opts.ManifestAnnotationFlags, "annotation", "a", nil, "manifest annotations") } -// parseAnnotationFlags parses annotation flags into a map. -func (opts *Annotation) parseAnnotationFlags(flags []string) error { +func (opts *Annotation) Parse(*cobra.Command) error { + opts.Annotations = make(map[string]map[string]string) manifestAnnotations := make(map[string]string) - for _, anno := range flags { + for _, anno := range opts.ManifestAnnotationFlags { key, val, success := strings.Cut(anno, "=") if !success { return &oerrors.Error{ diff --git a/cmd/oras/internal/option/packer.go b/cmd/oras/internal/option/packer.go index 5b345b100..f134043f9 100644 --- a/cmd/oras/internal/option/packer.go +++ b/cmd/oras/internal/option/packer.go @@ -74,7 +74,7 @@ func (opts *Packer) ExportManifest(ctx context.Context, fetcher content.Fetcher, } return os.WriteFile(opts.ManifestExportPath, manifestBytes, 0666) } -func (opts *Packer) Parse(*cobra.Command) error { +func (opts *Packer) Parse(cmd *cobra.Command) error { if !opts.PathValidationDisabled { var failedPaths []string for _, path := range opts.FileRefs { @@ -91,12 +91,12 @@ func (opts *Packer) Parse(*cobra.Command) error { return fmt.Errorf("%w: %v", errPathValidation, strings.Join(failedPaths, ", ")) } } - return opts.LoadManifestAnnotations() + return opts.parseAnnotations(cmd) } -// LoadManifestAnnotations loads the manifest annotation map. -func (opts *Packer) LoadManifestAnnotations() (err error) { - if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotations) != 0 { +// parseAnnotations loads the manifest annotation map. +func (opts *Packer) parseAnnotations(cmd *cobra.Command) (err error) { + if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotationFlags) != 0 { return errAnnotationConflict } if opts.AnnotationFilePath != "" { @@ -107,11 +107,8 @@ func (opts *Packer) LoadManifestAnnotations() (err error) { } } } - if len(opts.ManifestAnnotations) != 0 { - opts.Annotations = make(map[string]map[string]string) - if err = opts.parseAnnotationFlags(opts.ManifestAnnotations); err != nil { - return err - } + if len(opts.ManifestAnnotationFlags) != 0 { + return opts.Annotation.Parse(cmd) } return } diff --git a/cmd/oras/internal/option/packer_test.go b/cmd/oras/internal/option/packer_test.go index 852336838..8b5258782 100644 --- a/cmd/oras/internal/option/packer_test.go +++ b/cmd/oras/internal/option/packer_test.go @@ -37,44 +37,44 @@ func TestPacker_FlagInit(t *testing.T) { ApplyFlags(&test, pflag.NewFlagSet("oras-test", pflag.ExitOnError)) } -func TestPacker_LoadManifestAnnotations_err(t *testing.T) { +func TestPacker_parseAnnotations_err(t *testing.T) { opts := Packer{ Annotation: Annotation{ - ManifestAnnotations: []string{"Key=Val"}, + ManifestAnnotationFlags: []string{"Key=Val"}, }, AnnotationFilePath: "this is not a file", // testFile, } - if err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationConflict) { + if err := opts.parseAnnotations(nil); !errors.Is(err, errAnnotationConflict) { t.Fatalf("unexpected error: %v", err) } opts = Packer{ AnnotationFilePath: "this is not a file", // testFile, } - if err := opts.LoadManifestAnnotations(); err == nil { + if err := opts.parseAnnotations(nil); err == nil { t.Fatalf("unexpected error: %v", err) } opts = Packer{ Annotation: Annotation{ - ManifestAnnotations: []string{"KeyVal"}, + ManifestAnnotationFlags: []string{"KeyVal"}, }, } - if err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationFormat) { + if err := opts.parseAnnotations(nil); !errors.Is(err, errAnnotationFormat) { t.Fatalf("unexpected error: %v", err) } opts = Packer{ Annotation: Annotation{ - ManifestAnnotations: []string{"Key=Val1", "Key=Val2"}, + ManifestAnnotationFlags: []string{"Key=Val1", "Key=Val2"}, }, } - if err := opts.LoadManifestAnnotations(); !errors.Is(err, errAnnotationDuplication) { + if err := opts.parseAnnotations(nil); !errors.Is(err, errAnnotationDuplication) { t.Fatalf("unexpected error: %v", err) } } -func TestPacker_LoadManifestAnnotations_annotationFile(t *testing.T) { +func TestPacker_parseAnnotations_annotationFile(t *testing.T) { testFile := filepath.Join(t.TempDir(), "testAnnotationFile") err := os.WriteFile(testFile, []byte(testContent), fs.ModePerm) if err != nil { @@ -84,7 +84,7 @@ func TestPacker_LoadManifestAnnotations_annotationFile(t *testing.T) { AnnotationFilePath: testFile, } - err = opts.LoadManifestAnnotations() + err = opts.parseAnnotations(nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -93,17 +93,17 @@ func TestPacker_LoadManifestAnnotations_annotationFile(t *testing.T) { } } -func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { +func TestPacker_parseAnnotations_annotationFlag(t *testing.T) { // Item do not contains '=' invalidFlag0 := []string{ "Key", } opts := Packer{ Annotation: Annotation{ - ManifestAnnotations: invalidFlag0, + ManifestAnnotationFlags: invalidFlag0, }, } - err := opts.LoadManifestAnnotations() + err := opts.parseAnnotations(nil) if !errors.Is(err, errAnnotationFormat) { t.Fatalf("unexpected error: %v", err) } @@ -115,10 +115,10 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { } opts = Packer{ Annotation: Annotation{ - ManifestAnnotations: invalidFlag1, + ManifestAnnotationFlags: invalidFlag1, }, } - err = opts.LoadManifestAnnotations() + err = opts.parseAnnotations(nil) if !errors.Is(err, errAnnotationDuplication) { t.Fatalf("unexpected error: %v", err) } @@ -131,10 +131,10 @@ func TestPacker_LoadManifestAnnotations_annotationFlag(t *testing.T) { } opts = Packer{ Annotation: Annotation{ - ManifestAnnotations: validFlag, + ManifestAnnotationFlags: validFlag, }, } - err = opts.LoadManifestAnnotations() + err = opts.parseAnnotations(nil) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cmd/oras/root/attach_test.go b/cmd/oras/root/attach_test.go index 895bb30be..00daaa67d 100644 --- a/cmd/oras/root/attach_test.go +++ b/cmd/oras/root/attach_test.go @@ -34,7 +34,7 @@ func Test_runAttach_errType(t *testing.T) { opts := &attachOptions{ Packer: option.Packer{ Annotation: option.Annotation{ - ManifestAnnotations: []string{"one", "two"}, + ManifestAnnotationFlags: []string{"one", "two"}, }, AnnotationFilePath: "/tmp/whatever", }, From a1fbe4703e8afd8de07a9a125e11b2601a60aabf Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 19 Sep 2024 16:26:21 +0800 Subject: [PATCH 10/13] refactored packer Signed-off-by: Xiaoxuan Wang --- cmd/oras/root/manifest/index/create.go | 32 ++------- cmd/oras/root/manifest/index/create_test.go | 75 ++++++++++----------- 2 files changed, 42 insertions(+), 65 deletions(-) diff --git a/cmd/oras/root/manifest/index/create.go b/cmd/oras/root/manifest/index/create.go index 18cf241ee..12b51e95d 100644 --- a/cmd/oras/root/manifest/index/create.go +++ b/cmd/oras/root/manifest/index/create.go @@ -47,12 +47,11 @@ type createOptions struct { option.Common option.Target option.Pretty + option.Annotation - sources []string - extraRefs []string - rawAnnotations []string - indexAnnotations map[string]string - outputPath string + sources []string + extraRefs []string + outputPath string } func createCmd() *cobra.Command { @@ -92,11 +91,6 @@ Example - Create an index and output the index to stdout, auto push will be disa opts.RawReference = refs[0] opts.extraRefs = refs[1:] opts.sources = args[1:] - var err error - opts.indexAnnotations, err = parseAnnotations(opts.rawAnnotations) - if err != nil { - return err - } return option.Parse(cmd, &opts) }, Aliases: []string{"pack"}, @@ -105,7 +99,6 @@ Example - Create an index and output the index to stdout, auto push will be disa }, } cmd.Flags().StringVarP(&opts.outputPath, "output", "o", "", "file `path` to write the created index to, use - for stdout") - cmd.Flags().StringArrayVarP(&opts.rawAnnotations, "annotation", "a", nil, "index annotations") option.ApplyFlags(&opts, cmd.Flags()) return oerrors.Command(cmd, &opts.Target) } @@ -126,7 +119,7 @@ func createIndex(cmd *cobra.Command, opts createOptions) error { }, MediaType: ocispec.MediaTypeImageIndex, Manifests: manifests, - Annotations: opts.indexAnnotations, + Annotations: opts.Annotations[option.AnnotationManifest], } indexBytes, err := json.Marshal(index) if err != nil { @@ -216,18 +209,3 @@ func pushIndex(ctx context.Context, target oras.Target, desc ocispec.Descriptor, } return printer.Println("Digest:", desc.Digest) } - -func parseAnnotations(input []string) (map[string]string, error) { - annotations := make(map[string]string) - for _, anno := range input { - key, val, success := strings.Cut(anno, "=") - if !success { - return nil, fmt.Errorf("annotation value doesn't match the required format of \"key=value\"") - } - if _, ok := annotations[key]; ok { - return nil, fmt.Errorf("duplicate annotation key: %v", key) - } - annotations[key] = val - } - return annotations, nil -} diff --git a/cmd/oras/root/manifest/index/create_test.go b/cmd/oras/root/manifest/index/create_test.go index 7b5bd2c5a..e7d2b90a1 100644 --- a/cmd/oras/root/manifest/index/create_test.go +++ b/cmd/oras/root/manifest/index/create_test.go @@ -16,46 +16,45 @@ limitations under the License. package index import ( - "reflect" "testing" ) func Test_parseAnnotations(t *testing.T) { - tests := []struct { - name string - input []string - annotations map[string]string - wantErr bool - wantAnnotations map[string]string - }{ - { - name: "valid input", - input: []string{"a=b", "c=d", "e=f"}, - wantErr: false, - wantAnnotations: map[string]string{"a": "b", "c": "d", "e": "f"}, - }, - { - name: "invalid input", - input: []string{"a=b", "c:d", "e=f"}, - wantErr: true, - wantAnnotations: nil, - }, - { - name: "duplicate key", - input: []string{"a=b", "c=d", "a=e"}, - wantErr: true, - wantAnnotations: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - annotations, err := parseAnnotations(tt.input) - if (err != nil) != tt.wantErr { - t.Errorf("parseAnnotations() error = %v, wantErr %v", err, tt.wantErr) - } - if !reflect.DeepEqual(annotations, tt.wantAnnotations) { - t.Errorf("parseAnnotations() annotations = %v, want %v", tt.annotations, tt.wantAnnotations) - } - }) - } + // tests := []struct { + // name string + // input []string + // annotations map[string]string + // wantErr bool + // wantAnnotations map[string]string + // }{ + // { + // name: "valid input", + // input: []string{"a=b", "c=d", "e=f"}, + // wantErr: false, + // wantAnnotations: map[string]string{"a": "b", "c": "d", "e": "f"}, + // }, + // { + // name: "invalid input", + // input: []string{"a=b", "c:d", "e=f"}, + // wantErr: true, + // wantAnnotations: nil, + // }, + // { + // name: "duplicate key", + // input: []string{"a=b", "c=d", "a=e"}, + // wantErr: true, + // wantAnnotations: nil, + // }, + // } + // for _, tt := range tests { + // t.Run(tt.name, func(t *testing.T) { + // annotations, err := parseAnnotations(tt.input) + // if (err != nil) != tt.wantErr { + // t.Errorf("parseAnnotations() error = %v, wantErr %v", err, tt.wantErr) + // } + // if !reflect.DeepEqual(annotations, tt.wantAnnotations) { + // t.Errorf("parseAnnotations() annotations = %v, want %v", tt.annotations, tt.wantAnnotations) + // } + // }) + // } } From 119aceef3842041b95cddf7a01bfc6a3bf7ed565 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 19 Sep 2024 16:35:49 +0800 Subject: [PATCH 11/13] removed redundant file Signed-off-by: Xiaoxuan Wang --- cmd/oras/root/manifest/index/create_test.go | 60 --------------------- 1 file changed, 60 deletions(-) delete mode 100644 cmd/oras/root/manifest/index/create_test.go diff --git a/cmd/oras/root/manifest/index/create_test.go b/cmd/oras/root/manifest/index/create_test.go deleted file mode 100644 index e7d2b90a1..000000000 --- a/cmd/oras/root/manifest/index/create_test.go +++ /dev/null @@ -1,60 +0,0 @@ -/* -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 index - -import ( - "testing" -) - -func Test_parseAnnotations(t *testing.T) { - // tests := []struct { - // name string - // input []string - // annotations map[string]string - // wantErr bool - // wantAnnotations map[string]string - // }{ - // { - // name: "valid input", - // input: []string{"a=b", "c=d", "e=f"}, - // wantErr: false, - // wantAnnotations: map[string]string{"a": "b", "c": "d", "e": "f"}, - // }, - // { - // name: "invalid input", - // input: []string{"a=b", "c:d", "e=f"}, - // wantErr: true, - // wantAnnotations: nil, - // }, - // { - // name: "duplicate key", - // input: []string{"a=b", "c=d", "a=e"}, - // wantErr: true, - // wantAnnotations: nil, - // }, - // } - // for _, tt := range tests { - // t.Run(tt.name, func(t *testing.T) { - // annotations, err := parseAnnotations(tt.input) - // if (err != nil) != tt.wantErr { - // t.Errorf("parseAnnotations() error = %v, wantErr %v", err, tt.wantErr) - // } - // if !reflect.DeepEqual(annotations, tt.wantAnnotations) { - // t.Errorf("parseAnnotations() annotations = %v, want %v", tt.annotations, tt.wantAnnotations) - // } - // }) - // } -} From 6b0349b71db4efdc2252b31535a7c09419397e4e Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Fri, 20 Sep 2024 17:51:43 +0800 Subject: [PATCH 12/13] refined the code Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/option/annotation.go | 16 ++++++++++------ cmd/oras/internal/option/packer.go | 11 ++++++----- cmd/oras/internal/option/packer_test.go | 12 ++++++------ cmd/oras/root/attach_test.go | 2 +- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/cmd/oras/internal/option/annotation.go b/cmd/oras/internal/option/annotation.go index 61b35384e..5588b4b53 100644 --- a/cmd/oras/internal/option/annotation.go +++ b/cmd/oras/internal/option/annotation.go @@ -32,19 +32,21 @@ var ( // Packer option struct. type Annotation struct { - ManifestAnnotationFlags []string // raw input of manifest annotation flags - Annotations map[string]map[string]string // parsed manifest and config annotations + // ManifestAnnotations contains raw input of manifest annotation "key=value" pairs + ManifestAnnotations []string + + // Annotations contains parsed manifest and config annotations + Annotations map[string]map[string]string } // ApplyFlags applies flags to a command flag set. func (opts *Annotation) ApplyFlags(fs *pflag.FlagSet) { - fs.StringArrayVarP(&opts.ManifestAnnotationFlags, "annotation", "a", nil, "manifest annotations") + fs.StringArrayVarP(&opts.ManifestAnnotations, "annotation", "a", nil, "manifest annotations") } func (opts *Annotation) Parse(*cobra.Command) error { - opts.Annotations = make(map[string]map[string]string) manifestAnnotations := make(map[string]string) - for _, anno := range opts.ManifestAnnotationFlags { + for _, anno := range opts.ManifestAnnotations { key, val, success := strings.Cut(anno, "=") if !success { return &oerrors.Error{ @@ -57,6 +59,8 @@ func (opts *Annotation) Parse(*cobra.Command) error { } manifestAnnotations[key] = val } - opts.Annotations[AnnotationManifest] = manifestAnnotations + opts.Annotations = map[string]map[string]string{ + AnnotationManifest: manifestAnnotations, + } return nil } diff --git a/cmd/oras/internal/option/packer.go b/cmd/oras/internal/option/packer.go index f134043f9..a69b12f3a 100644 --- a/cmd/oras/internal/option/packer.go +++ b/cmd/oras/internal/option/packer.go @@ -74,6 +74,7 @@ func (opts *Packer) ExportManifest(ctx context.Context, fetcher content.Fetcher, } return os.WriteFile(opts.ManifestExportPath, manifestBytes, 0666) } + func (opts *Packer) Parse(cmd *cobra.Command) error { if !opts.PathValidationDisabled { var failedPaths []string @@ -95,22 +96,22 @@ func (opts *Packer) Parse(cmd *cobra.Command) error { } // parseAnnotations loads the manifest annotation map. -func (opts *Packer) parseAnnotations(cmd *cobra.Command) (err error) { - if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotationFlags) != 0 { +func (opts *Packer) parseAnnotations(cmd *cobra.Command) error { + if opts.AnnotationFilePath != "" && len(opts.ManifestAnnotations) != 0 { return errAnnotationConflict } if opts.AnnotationFilePath != "" { - if err = decodeJSON(opts.AnnotationFilePath, &opts.Annotations); err != nil { + if err := decodeJSON(opts.AnnotationFilePath, &opts.Annotations); err != nil { return &oerrors.Error{ Err: fmt.Errorf(`invalid annotation json file: failed to load annotations from %s`, opts.AnnotationFilePath), Recommendation: `Annotation file doesn't match the required format. Please refer to the document at https://oras.land/docs/how_to_guides/manifest_annotations`, } } } - if len(opts.ManifestAnnotationFlags) != 0 { + if len(opts.ManifestAnnotations) != 0 { return opts.Annotation.Parse(cmd) } - return + return nil } // decodeJSON decodes a json file v to filename. diff --git a/cmd/oras/internal/option/packer_test.go b/cmd/oras/internal/option/packer_test.go index 8b5258782..35248ccd7 100644 --- a/cmd/oras/internal/option/packer_test.go +++ b/cmd/oras/internal/option/packer_test.go @@ -40,7 +40,7 @@ func TestPacker_FlagInit(t *testing.T) { func TestPacker_parseAnnotations_err(t *testing.T) { opts := Packer{ Annotation: Annotation{ - ManifestAnnotationFlags: []string{"Key=Val"}, + ManifestAnnotations: []string{"Key=Val"}, }, AnnotationFilePath: "this is not a file", // testFile, } @@ -57,7 +57,7 @@ func TestPacker_parseAnnotations_err(t *testing.T) { opts = Packer{ Annotation: Annotation{ - ManifestAnnotationFlags: []string{"KeyVal"}, + ManifestAnnotations: []string{"KeyVal"}, }, } if err := opts.parseAnnotations(nil); !errors.Is(err, errAnnotationFormat) { @@ -66,7 +66,7 @@ func TestPacker_parseAnnotations_err(t *testing.T) { opts = Packer{ Annotation: Annotation{ - ManifestAnnotationFlags: []string{"Key=Val1", "Key=Val2"}, + ManifestAnnotations: []string{"Key=Val1", "Key=Val2"}, }, } if err := opts.parseAnnotations(nil); !errors.Is(err, errAnnotationDuplication) { @@ -100,7 +100,7 @@ func TestPacker_parseAnnotations_annotationFlag(t *testing.T) { } opts := Packer{ Annotation: Annotation{ - ManifestAnnotationFlags: invalidFlag0, + ManifestAnnotations: invalidFlag0, }, } err := opts.parseAnnotations(nil) @@ -115,7 +115,7 @@ func TestPacker_parseAnnotations_annotationFlag(t *testing.T) { } opts = Packer{ Annotation: Annotation{ - ManifestAnnotationFlags: invalidFlag1, + ManifestAnnotations: invalidFlag1, }, } err = opts.parseAnnotations(nil) @@ -131,7 +131,7 @@ func TestPacker_parseAnnotations_annotationFlag(t *testing.T) { } opts = Packer{ Annotation: Annotation{ - ManifestAnnotationFlags: validFlag, + ManifestAnnotations: validFlag, }, } err = opts.parseAnnotations(nil) diff --git a/cmd/oras/root/attach_test.go b/cmd/oras/root/attach_test.go index 00daaa67d..895bb30be 100644 --- a/cmd/oras/root/attach_test.go +++ b/cmd/oras/root/attach_test.go @@ -34,7 +34,7 @@ func Test_runAttach_errType(t *testing.T) { opts := &attachOptions{ Packer: option.Packer{ Annotation: option.Annotation{ - ManifestAnnotationFlags: []string{"one", "two"}, + ManifestAnnotations: []string{"one", "two"}, }, AnnotationFilePath: "/tmp/whatever", }, From 1b039e4bf9a01f0bb2dc0dea7347116b247a7c67 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 23 Sep 2024 09:51:26 +0800 Subject: [PATCH 13/13] fix Signed-off-by: Xiaoxuan Wang --- cmd/oras/internal/option/annotation.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/annotation.go b/cmd/oras/internal/option/annotation.go index 5588b4b53..035307a40 100644 --- a/cmd/oras/internal/option/annotation.go +++ b/cmd/oras/internal/option/annotation.go @@ -30,7 +30,7 @@ var ( errAnnotationDuplication = errors.New("duplicate annotation key") ) -// Packer option struct. +// Annotation option struct. type Annotation struct { // ManifestAnnotations contains raw input of manifest annotation "key=value" pairs ManifestAnnotations []string @@ -44,6 +44,7 @@ func (opts *Annotation) ApplyFlags(fs *pflag.FlagSet) { fs.StringArrayVarP(&opts.ManifestAnnotations, "annotation", "a", nil, "manifest annotations") } +// Parse parses the input annotation flags. func (opts *Annotation) Parse(*cobra.Command) error { manifestAnnotations := make(map[string]string) for _, anno := range opts.ManifestAnnotations {