From 20099b6ad0d8043ef2315ad77e769674ba42099d Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 16:39:30 +0800 Subject: [PATCH 01/14] refactor: use Parser to process option input Signed-off-by: Billy Zha --- cmd/oras/attach.go | 2 +- cmd/oras/blob/delete.go | 2 +- cmd/oras/blob/fetch.go | 2 +- cmd/oras/blob/push.go | 2 +- cmd/oras/cp.go | 11 +++--- cmd/oras/discover.go | 8 ++--- cmd/oras/internal/option/parser.go | 41 +++++++++++++++++++++++ cmd/oras/internal/option/platform.go | 24 ++++++------- cmd/oras/internal/option/platform_test.go | 37 ++++++++++---------- cmd/oras/internal/option/remote.go | 4 +-- cmd/oras/login.go | 8 ++--- cmd/oras/manifest/delete.go | 2 +- cmd/oras/manifest/fetch.go | 12 ++----- cmd/oras/manifest/fetch_config.go | 10 ++---- cmd/oras/manifest/push.go | 2 +- cmd/oras/pull.go | 10 ++---- cmd/oras/push.go | 2 +- cmd/oras/repository/ls.go | 2 +- cmd/oras/repository/tags.go | 2 +- cmd/oras/tag/tag.go | 2 +- 20 files changed, 99 insertions(+), 86 deletions(-) create mode 100644 cmd/oras/internal/option/parser.go diff --git a/cmd/oras/attach.go b/cmd/oras/attach.go index c188a01ab..ed032ef6b 100644 --- a/cmd/oras/attach.go +++ b/cmd/oras/attach.go @@ -65,7 +65,7 @@ Example - Attach file 'hi.txt' and export the pushed manifest to 'manifest.json' `, Args: cobra.MinimumNArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/blob/delete.go b/cmd/oras/blob/delete.go index 4f0f64118..88bf88c2d 100644 --- a/cmd/oras/blob/delete.go +++ b/cmd/oras/blob/delete.go @@ -59,7 +59,7 @@ Example - Delete a blob and print its descriptor: if opts.OutputDescriptor && !opts.Confirmed { return errors.New("must apply --force to confirm the deletion if the descriptor is outputted") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/blob/fetch.go b/cmd/oras/blob/fetch.go index 8e501c7e2..71dee5e1b 100644 --- a/cmd/oras/blob/fetch.go +++ b/cmd/oras/blob/fetch.go @@ -70,7 +70,7 @@ Example - Fetch the blob, save it to a local file and print the descriptor: return errors.New("`--output -` cannot be used with `--descriptor` at the same time") } - return opts.ReadPassword() + return option.Parse(&opts) }, Aliases: []string{"get"}, RunE: func(cmd *cobra.Command, args []string) error { diff --git a/cmd/oras/blob/push.go b/cmd/oras/blob/push.go index f0f9537d4..0ae50e950 100644 --- a/cmd/oras/blob/push.go +++ b/cmd/oras/blob/push.go @@ -81,7 +81,7 @@ Example - Push blob without TLS: return errors.New("`--size` must be provided if the blob is read from stdin") } } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { return pushBlob(opts) diff --git a/cmd/oras/cp.go b/cmd/oras/cp.go index 59efbaaf9..960954921 100644 --- a/cmd/oras/cp.go +++ b/cmd/oras/cp.go @@ -68,6 +68,9 @@ Example - Copy the artifact tagged with 'v1' from repository 'localhost:5000/net oras cp --concurrency 6 localhost:5000/net-monitor:v1 localhost:5000/net-monitor-copy:v1,tag2,tag3 `, Args: cobra.ExactArgs(2), + PreRunE: func(cmd *cobra.Command, args []string) error { + return option.Parse(&opts) + }, RunE: func(cmd *cobra.Command, args []string) error { opts.srcRef = args[0] refs := strings.Split(args[1], ",") @@ -88,10 +91,6 @@ Example - Copy the artifact tagged with 'v1' from repository 'localhost:5000/net func runCopy(opts copyOptions) error { ctx, _ := opts.SetLoggerLevel() - targetPlatform, err := opts.Parse() - if err != nil { - return err - } // Prepare source src, err := opts.src.NewRepository(opts.srcRef, opts.Common) @@ -145,9 +144,7 @@ func runCopy(opts copyOptions) error { copyOptions := oras.CopyOptions{ CopyGraphOptions: extendedCopyOptions.CopyGraphOptions, } - if targetPlatform != nil { - copyOptions.WithTargetPlatform(targetPlatform) - } + copyOptions.WithTargetPlatform(opts.OCIPlatform) desc, err = oras.Copy(ctx, src, opts.srcRef, dst, opts.dstRef, copyOptions) } } diff --git a/cmd/oras/discover.go b/cmd/oras/discover.go index 6a561ba72..aad54bb52 100644 --- a/cmd/oras/discover.go +++ b/cmd/oras/discover.go @@ -63,7 +63,7 @@ Example - Discover referrers with type 'test-artifact' of manifest 'hello:latest `, Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] @@ -86,14 +86,10 @@ func runDiscover(opts discoverOptions) error { if repo.Reference.Reference == "" { return errors.NewErrInvalidReference(repo.Reference) } - targetPlatform, err := opts.Parse() - if err != nil { - return err - } // discover artifacts resolveOpts := oras.DefaultResolveOptions - resolveOpts.TargetPlatform = targetPlatform + resolveOpts.TargetPlatform = opts.OCIPlatform desc, err := oras.Resolve(ctx, repo, repo.Reference.Reference, resolveOpts) if err != nil { return err diff --git a/cmd/oras/internal/option/parser.go b/cmd/oras/internal/option/parser.go new file mode 100644 index 000000000..70bc5e91b --- /dev/null +++ b/cmd/oras/internal/option/parser.go @@ -0,0 +1,41 @@ +/* +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 ( + "reflect" +) + +// FlagParser parses flags in an option. +type FlagParser interface { + Parse() error +} + +// Parse parses applicable fields of the passed-in option pointer and returns +// error during parsing. +func Parse(optsPtr interface{}) error { + v := reflect.ValueOf(optsPtr).Elem() + for i := 0; i < v.NumField(); i++ { + f := v.Field(i) + if f.CanSet() { + iface := f.Addr().Interface() + if a, ok := iface.(FlagParser); ok { + if err := a.Parse(); err != nil { + return err + } + } + } + } + return nil +} diff --git a/cmd/oras/internal/option/platform.go b/cmd/oras/internal/option/platform.go index 64736487f..bbad6f84b 100644 --- a/cmd/oras/internal/option/platform.go +++ b/cmd/oras/internal/option/platform.go @@ -3,9 +3,7 @@ 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. @@ -26,25 +24,26 @@ import ( // Platform option struct. type Platform struct { - Platform string + platform string + OCIPlatform *ocispec.Platform } // ApplyFlags applies flags to a command flag set. func (opts *Platform) ApplyFlags(fs *pflag.FlagSet) { - fs.StringVarP(&opts.Platform, "platform", "", "", "request platform in the form of `os[/arch][/variant][:os_version]`") + fs.StringVarP(&opts.platform, "platform", "", "", "request platform in the form of `os[/arch][/variant][:os_version]`") } // parse parses the input platform flag to an oci platform type. -func (opts *Platform) Parse() (*ocispec.Platform, error) { - if opts.Platform == "" { - return nil, nil +func (opts *Platform) Parse() error { + if opts.platform == "" { + return nil } // OS[/Arch[/Variant]][:OSVersion] // If Arch is not provided, will use GOARCH instead var platformStr string var p ocispec.Platform - platformStr, p.OSVersion, _ = strings.Cut(opts.Platform, ":") + platformStr, p.OSVersion, _ = strings.Cut(opts.platform, ":") parts := strings.Split(platformStr, "/") switch len(parts) { case 3: @@ -55,14 +54,15 @@ func (opts *Platform) Parse() (*ocispec.Platform, error) { case 1: p.Architecture = runtime.GOARCH default: - return nil, fmt.Errorf("failed to parse platform %q: expected format os[/arch[/variant]]", opts.Platform) + return fmt.Errorf("failed to parse platform %q: expected format os[/arch[/variant]]", opts.platform) } p.OS = parts[0] if p.OS == "" { - return nil, fmt.Errorf("invalid platform: OS cannot be empty") + return fmt.Errorf("invalid platform: OS cannot be empty") } if p.Architecture == "" { - return nil, fmt.Errorf("invalid platform: Architecture cannot be empty") + return fmt.Errorf("invalid platform: Architecture cannot be empty") } - return &p, nil + opts.OCIPlatform = &p + return nil } diff --git a/cmd/oras/internal/option/platform_test.go b/cmd/oras/internal/option/platform_test.go index 246997654..3e01ec01a 100644 --- a/cmd/oras/internal/option/platform_test.go +++ b/cmd/oras/internal/option/platform_test.go @@ -3,9 +3,7 @@ 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. @@ -27,8 +25,8 @@ import ( func TestPlatform_ApplyFlags(t *testing.T) { var test struct{ Platform } ApplyFlags(&test, pflag.NewFlagSet("oras-test", pflag.ExitOnError)) - if test.Platform.Platform != "" { - t.Fatalf("expecting platform to be empty but got: %v", test.Platform.Platform) + if test.Platform.platform != "" { + t.Fatalf("expecting platform to be empty but got: %v", test.Platform.platform) } } @@ -37,15 +35,15 @@ func TestPlatform_Parse_err(t *testing.T) { name string opts *Platform }{ - {name: "empty arch 1", opts: &Platform{"os/"}}, - {name: "empty arch 2", opts: &Platform{"os//variant"}}, - {name: "empty os", opts: &Platform{"/arch"}}, - {name: "empty os with variant", opts: &Platform{"/arch/variant"}}, - {name: "trailing slash", opts: &Platform{"os/arch/variant/llama"}}, + {name: "empty arch 1", opts: &Platform{"os/", nil}}, + {name: "empty arch 2", opts: &Platform{"os//variant", nil}}, + {name: "empty os", opts: &Platform{"/arch", nil}}, + {name: "empty os with variant", opts: &Platform{"/arch/variant", nil}}, + {name: "trailing slash", opts: &Platform{"os/arch/variant/llama", nil}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := tt.opts.Parse() + err := tt.opts.Parse() if err == nil { t.Errorf("Platform.Parse() error = %v, wantErr %v", err, true) return @@ -60,21 +58,20 @@ func TestPlatform_Parse(t *testing.T) { opts *Platform want *ocispec.Platform }{ - {name: "empty", opts: &Platform{""}, want: nil}, - {name: "default arch", opts: &Platform{"os"}, want: &ocispec.Platform{OS: "os", Architecture: runtime.GOARCH}}, - {name: "os&arch", opts: &Platform{"os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, - {name: "empty variant", opts: &Platform{"os/aRcH/"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: ""}}, - {name: "os&arch&variant", opts: &Platform{"os/aRcH/vAriAnt"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt"}}, - {name: "os version", opts: &Platform{"os/aRcH/vAriAnt:osversion"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt", OSVersion: "osversion"}}, - {name: "long os version", opts: &Platform{"os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, + {name: "empty", opts: &Platform{platform: ""}, want: nil}, + {name: "default arch", opts: &Platform{platform: "os"}, want: &ocispec.Platform{OS: "os", Architecture: runtime.GOARCH}}, + {name: "os&arch", opts: &Platform{platform: "os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, + {name: "empty variant", opts: &Platform{platform: "os/aRcH/"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: ""}}, + {name: "os&arch&variant", opts: &Platform{platform: "os/aRcH/vAriAnt"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt"}}, + {name: "os version", opts: &Platform{platform: "os/aRcH/vAriAnt:osversion"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt", OSVersion: "osversion"}}, + {name: "long os version", opts: &Platform{platform: "os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := tt.opts.Parse() - if err != nil { + if err := tt.opts.Parse(); err != nil { t.Errorf("Platform.Parse() error = %v", err) - return } + got := tt.opts.OCIPlatform if !reflect.DeepEqual(got, tt.want) { t.Errorf("Platform.Parse() = %v, want %v", got, tt.want) } diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 118d75559..4e05f5aeb 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -87,8 +87,8 @@ func (opts *Remote) ApplyFlagsWithPrefix(fs *pflag.FlagSet, prefix, description } } -// ReadPassword tries to read password with optional cmd prompt. -func (opts *Remote) ReadPassword() (err error) { +// Parse tries to read password with optional cmd prompt. +func (opts *Remote) Parse() error { if opts.Password != "" { fmt.Fprintln(os.Stderr, "WARNING! Using --password via the CLI is insecure. Use --password-stdin.") } else if opts.PasswordFromStdin { diff --git a/cmd/oras/login.go b/cmd/oras/login.go index e896d2252..b102444a8 100644 --- a/cmd/oras/login.go +++ b/cmd/oras/login.go @@ -61,7 +61,7 @@ Example - Log in with username and password in an interactive terminal and no TL `, Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.Hostname = args[0] @@ -130,9 +130,9 @@ func runLogin(opts loginOptions) (err error) { return nil } -func readLine(prompt string, slient bool) (string, error) { +func readLine(prompt string, silent bool) (string, error) { fmt.Print(prompt) - if slient { + if silent { fd := os.Stdin.Fd() state, err := term.SaveState(fd) if err != nil { @@ -147,7 +147,7 @@ func readLine(prompt string, slient bool) (string, error) { if err != nil { return "", err } - if slient { + if silent { fmt.Println() } diff --git a/cmd/oras/manifest/delete.go b/cmd/oras/manifest/delete.go index 568eefd91..09751eb2e 100644 --- a/cmd/oras/manifest/delete.go +++ b/cmd/oras/manifest/delete.go @@ -63,7 +63,7 @@ Example - Delete a manifest by digest 'sha256:99e4703fbf30916f549cd6bfa9cdbab614 if opts.OutputDescriptor && !opts.Confirmed { return errors.New("must apply --force to confirm the deletion if the descriptor is outputted") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(_ *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/manifest/fetch.go b/cmd/oras/manifest/fetch.go index 7173a5c42..e4c911f21 100644 --- a/cmd/oras/manifest/fetch.go +++ b/cmd/oras/manifest/fetch.go @@ -69,8 +69,7 @@ Example - Fetch manifest with prettified json result: if opts.outputPath == "-" && opts.OutputDescriptor { return errors.New("`--output -` cannot be used with `--descriptor` at the same time") } - - return opts.ReadPassword() + return option.Parse(&opts) }, Aliases: []string{"get"}, RunE: func(cmd *cobra.Command, args []string) error { @@ -98,11 +97,6 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { } repo.ManifestMediaTypes = opts.mediaTypes - targetPlatform, err := opts.Parse() - if err != nil { - return err - } - src, err := opts.CachedTarget(repo) if err != nil { return err @@ -112,7 +106,7 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { if opts.OutputDescriptor && opts.outputPath == "" { // fetch manifest descriptor only fetchOpts := oras.DefaultResolveOptions - fetchOpts.TargetPlatform = targetPlatform + fetchOpts.TargetPlatform = opts.OCIPlatform desc, err = oras.Resolve(ctx, src, opts.targetRef, fetchOpts) if err != nil { return err @@ -121,7 +115,7 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { // fetch manifest content var content []byte fetchOpts := oras.DefaultFetchBytesOptions - fetchOpts.TargetPlatform = targetPlatform + fetchOpts.TargetPlatform = opts.OCIPlatform desc, content, err = oras.FetchBytes(ctx, src, opts.targetRef, fetchOpts) if err != nil { return err diff --git a/cmd/oras/manifest/fetch_config.go b/cmd/oras/manifest/fetch_config.go index d57fc62ff..2e54eb478 100644 --- a/cmd/oras/manifest/fetch_config.go +++ b/cmd/oras/manifest/fetch_config.go @@ -76,8 +76,7 @@ Example - Fetch and print the prettified descriptor of the config: if opts.outputPath == "-" && opts.OutputDescriptor { return errors.New("`--output -` cannot be used with `--descriptor` at the same time") } - - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] @@ -102,18 +101,13 @@ func fetchConfig(opts fetchConfigOptions) (fetchErr error) { return oerrors.NewErrInvalidReference(repo.Reference) } - targetPlatform, err := opts.Parse() - if err != nil { - return err - } - src, err := opts.CachedTarget(repo) if err != nil { return err } // fetch config descriptor - configDesc, err := fetchConfigDesc(ctx, src, opts.targetRef, targetPlatform) + configDesc, err := fetchConfigDesc(ctx, src, opts.targetRef, opts.OCIPlatform) if err != nil { return err } diff --git a/cmd/oras/manifest/push.go b/cmd/oras/manifest/push.go index 6249cef30..0c172ade5 100644 --- a/cmd/oras/manifest/push.go +++ b/cmd/oras/manifest/push.go @@ -81,7 +81,7 @@ Example - Push a manifest to repository 'locahost:5000/hello' and tag with 'tag1 if opts.fileRef == "-" && opts.PasswordFromStdin { return errors.New("`-` read file from input and `--password-stdin` read password from input cannot be both used") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(_ *cobra.Command, args []string) error { refs := strings.Split(args[0], ",") diff --git a/cmd/oras/pull.go b/cmd/oras/pull.go index c57cc6eb2..630392e07 100644 --- a/cmd/oras/pull.go +++ b/cmd/oras/pull.go @@ -79,7 +79,7 @@ Example - Pull all files with concurrency level tuned: `, Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] @@ -99,10 +99,6 @@ Example - Pull all files with concurrency level tuned: func runPull(opts pullOptions) error { var printed sync.Map - targetPlatform, err := opts.Parse() - if err != nil { - return err - } repo, err := opts.NewRepository(opts.targetRef, opts.Common) if err != nil { return err @@ -125,9 +121,7 @@ func runPull(opts pullOptions) error { return err } } - if targetPlatform != nil { - copyOptions.WithTargetPlatform(targetPlatform) - } + copyOptions.WithTargetPlatform(opts.OCIPlatform) var getConfigOnce sync.Once copyOptions.FindSuccessors = func(ctx context.Context, fetcher content.Fetcher, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) { statusFetcher := content.FetcherFunc(func(ctx context.Context, target ocispec.Descriptor) (fetched io.ReadCloser, fetchErr error) { diff --git a/cmd/oras/push.go b/cmd/oras/push.go index e43c28d07..7a9f15a02 100644 --- a/cmd/oras/push.go +++ b/cmd/oras/push.go @@ -96,7 +96,7 @@ Example - Push file "hi.txt" with multiple tags and concurrency level tuned: if opts.artifactType != "" && opts.manifestConfigRef != "" { return errors.New("--artifact-type and --config cannot both be provided") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { refs := strings.Split(args[0], ",") diff --git a/cmd/oras/repository/ls.go b/cmd/oras/repository/ls.go index 40963b404..4355ba307 100644 --- a/cmd/oras/repository/ls.go +++ b/cmd/oras/repository/ls.go @@ -47,7 +47,7 @@ Example - List the repositories under the registry that include values lexically Args: cobra.ExactArgs(1), Aliases: []string{"list"}, PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.hostname = args[0] diff --git a/cmd/oras/repository/tags.go b/cmd/oras/repository/tags.go index abc6cc202..4cbf7287d 100644 --- a/cmd/oras/repository/tags.go +++ b/cmd/oras/repository/tags.go @@ -53,7 +53,7 @@ Example - Show tags of the target repository that include values lexically after Args: cobra.ExactArgs(1), Aliases: []string{"show-tags"}, PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/tag/tag.go b/cmd/oras/tag/tag.go index c4fa8a336..bcad5f796 100644 --- a/cmd/oras/tag/tag.go +++ b/cmd/oras/tag/tag.go @@ -55,7 +55,7 @@ Example - Tag the manifest 'v1.0.1' in 'localhost:5000/hello' to 'v1.0.1', 'v1.0 `, Args: cobra.MinimumNArgs(2), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(_ *cobra.Command, args []string) error { opts.srcRef = args[0] From 93aa4e1284850ad6108cf8043c2a4f1c068eeb0e Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 17:12:25 +0800 Subject: [PATCH 02/14] add unit test Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser_test.go | 52 +++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 cmd/oras/internal/option/parser_test.go diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go new file mode 100644 index 000000000..98d6f0948 --- /dev/null +++ b/cmd/oras/internal/option/parser_test.go @@ -0,0 +1,52 @@ +/* +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_test + +import ( + "testing" + + "oras.land/oras/cmd/oras/internal/option" +) + +type Test struct { + Cnt int +} + +func (t *Test) Parse() { + t.Cnt++ +} + +func TestParse(t *testing.T) { + type args struct { + Test + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"parse should be called", args{Test{Cnt: 0}}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := option.Parse(&tt.args); (err != nil) != tt.wantErr { + t.Errorf("Parse() error = %v, wantErr %v", err, tt.wantErr) + } + + if tt.args.Cnt != 1 { + t.Errorf("Expect Parse() to be called once but got %v", tt.args.Cnt) + } + }) + } +} From 7b1bd594695d4dd2a719932b1bcd6417a895d7e6 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 17:13:55 +0800 Subject: [PATCH 03/14] bug fix Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go index 98d6f0948..3eab59cbc 100644 --- a/cmd/oras/internal/option/parser_test.go +++ b/cmd/oras/internal/option/parser_test.go @@ -24,7 +24,7 @@ type Test struct { } func (t *Test) Parse() { - t.Cnt++ + t.Cnt += 1 } func TestParse(t *testing.T) { From 1d5657c7fbe762ac3b0d1a35a8c865fac031f195 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 17:21:43 +0800 Subject: [PATCH 04/14] make runnable Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go index 3eab59cbc..67bf17658 100644 --- a/cmd/oras/internal/option/parser_test.go +++ b/cmd/oras/internal/option/parser_test.go @@ -23,8 +23,9 @@ type Test struct { Cnt int } -func (t *Test) Parse() { +func (t *Test) Parse() error { t.Cnt += 1 + return nil } func TestParse(t *testing.T) { From 64f93949d576c433a150a45eafd100018ee5a89d Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 16:39:30 +0800 Subject: [PATCH 05/14] refactor: use Parser to process option input Signed-off-by: Billy Zha --- cmd/oras/attach.go | 2 +- cmd/oras/blob/delete.go | 2 +- cmd/oras/blob/fetch.go | 2 +- cmd/oras/blob/push.go | 2 +- cmd/oras/cp.go | 11 +++--- cmd/oras/discover.go | 8 ++--- cmd/oras/internal/option/parser.go | 41 +++++++++++++++++++++++ cmd/oras/internal/option/platform.go | 24 ++++++------- cmd/oras/internal/option/platform_test.go | 37 ++++++++++---------- cmd/oras/internal/option/remote.go | 4 +-- cmd/oras/login.go | 8 ++--- cmd/oras/manifest/delete.go | 2 +- cmd/oras/manifest/fetch.go | 12 ++----- cmd/oras/manifest/fetch_config.go | 10 ++---- cmd/oras/manifest/push.go | 2 +- cmd/oras/pull.go | 10 ++---- cmd/oras/push.go | 2 +- cmd/oras/repository/ls.go | 2 +- cmd/oras/repository/tags.go | 2 +- cmd/oras/tag/tag.go | 2 +- 20 files changed, 99 insertions(+), 86 deletions(-) create mode 100644 cmd/oras/internal/option/parser.go diff --git a/cmd/oras/attach.go b/cmd/oras/attach.go index c188a01ab..ed032ef6b 100644 --- a/cmd/oras/attach.go +++ b/cmd/oras/attach.go @@ -65,7 +65,7 @@ Example - Attach file 'hi.txt' and export the pushed manifest to 'manifest.json' `, Args: cobra.MinimumNArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/blob/delete.go b/cmd/oras/blob/delete.go index 4f0f64118..88bf88c2d 100644 --- a/cmd/oras/blob/delete.go +++ b/cmd/oras/blob/delete.go @@ -59,7 +59,7 @@ Example - Delete a blob and print its descriptor: if opts.OutputDescriptor && !opts.Confirmed { return errors.New("must apply --force to confirm the deletion if the descriptor is outputted") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/blob/fetch.go b/cmd/oras/blob/fetch.go index 8e501c7e2..71dee5e1b 100644 --- a/cmd/oras/blob/fetch.go +++ b/cmd/oras/blob/fetch.go @@ -70,7 +70,7 @@ Example - Fetch the blob, save it to a local file and print the descriptor: return errors.New("`--output -` cannot be used with `--descriptor` at the same time") } - return opts.ReadPassword() + return option.Parse(&opts) }, Aliases: []string{"get"}, RunE: func(cmd *cobra.Command, args []string) error { diff --git a/cmd/oras/blob/push.go b/cmd/oras/blob/push.go index f0f9537d4..0ae50e950 100644 --- a/cmd/oras/blob/push.go +++ b/cmd/oras/blob/push.go @@ -81,7 +81,7 @@ Example - Push blob without TLS: return errors.New("`--size` must be provided if the blob is read from stdin") } } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { return pushBlob(opts) diff --git a/cmd/oras/cp.go b/cmd/oras/cp.go index 59efbaaf9..960954921 100644 --- a/cmd/oras/cp.go +++ b/cmd/oras/cp.go @@ -68,6 +68,9 @@ Example - Copy the artifact tagged with 'v1' from repository 'localhost:5000/net oras cp --concurrency 6 localhost:5000/net-monitor:v1 localhost:5000/net-monitor-copy:v1,tag2,tag3 `, Args: cobra.ExactArgs(2), + PreRunE: func(cmd *cobra.Command, args []string) error { + return option.Parse(&opts) + }, RunE: func(cmd *cobra.Command, args []string) error { opts.srcRef = args[0] refs := strings.Split(args[1], ",") @@ -88,10 +91,6 @@ Example - Copy the artifact tagged with 'v1' from repository 'localhost:5000/net func runCopy(opts copyOptions) error { ctx, _ := opts.SetLoggerLevel() - targetPlatform, err := opts.Parse() - if err != nil { - return err - } // Prepare source src, err := opts.src.NewRepository(opts.srcRef, opts.Common) @@ -145,9 +144,7 @@ func runCopy(opts copyOptions) error { copyOptions := oras.CopyOptions{ CopyGraphOptions: extendedCopyOptions.CopyGraphOptions, } - if targetPlatform != nil { - copyOptions.WithTargetPlatform(targetPlatform) - } + copyOptions.WithTargetPlatform(opts.OCIPlatform) desc, err = oras.Copy(ctx, src, opts.srcRef, dst, opts.dstRef, copyOptions) } } diff --git a/cmd/oras/discover.go b/cmd/oras/discover.go index 5401520b9..7b61f8739 100644 --- a/cmd/oras/discover.go +++ b/cmd/oras/discover.go @@ -64,7 +64,7 @@ Example - Discover referrers with type 'test-artifact' of manifest 'hello:latest `, Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] @@ -87,14 +87,10 @@ func runDiscover(opts discoverOptions) error { if repo.Reference.Reference == "" { return errors.NewErrInvalidReference(repo.Reference) } - targetPlatform, err := opts.Parse() - if err != nil { - return err - } // discover artifacts resolveOpts := oras.DefaultResolveOptions - resolveOpts.TargetPlatform = targetPlatform + resolveOpts.TargetPlatform = opts.OCIPlatform desc, err := oras.Resolve(ctx, repo, repo.Reference.Reference, resolveOpts) if err != nil { return err diff --git a/cmd/oras/internal/option/parser.go b/cmd/oras/internal/option/parser.go new file mode 100644 index 000000000..70bc5e91b --- /dev/null +++ b/cmd/oras/internal/option/parser.go @@ -0,0 +1,41 @@ +/* +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 ( + "reflect" +) + +// FlagParser parses flags in an option. +type FlagParser interface { + Parse() error +} + +// Parse parses applicable fields of the passed-in option pointer and returns +// error during parsing. +func Parse(optsPtr interface{}) error { + v := reflect.ValueOf(optsPtr).Elem() + for i := 0; i < v.NumField(); i++ { + f := v.Field(i) + if f.CanSet() { + iface := f.Addr().Interface() + if a, ok := iface.(FlagParser); ok { + if err := a.Parse(); err != nil { + return err + } + } + } + } + return nil +} diff --git a/cmd/oras/internal/option/platform.go b/cmd/oras/internal/option/platform.go index 64736487f..bbad6f84b 100644 --- a/cmd/oras/internal/option/platform.go +++ b/cmd/oras/internal/option/platform.go @@ -3,9 +3,7 @@ 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. @@ -26,25 +24,26 @@ import ( // Platform option struct. type Platform struct { - Platform string + platform string + OCIPlatform *ocispec.Platform } // ApplyFlags applies flags to a command flag set. func (opts *Platform) ApplyFlags(fs *pflag.FlagSet) { - fs.StringVarP(&opts.Platform, "platform", "", "", "request platform in the form of `os[/arch][/variant][:os_version]`") + fs.StringVarP(&opts.platform, "platform", "", "", "request platform in the form of `os[/arch][/variant][:os_version]`") } // parse parses the input platform flag to an oci platform type. -func (opts *Platform) Parse() (*ocispec.Platform, error) { - if opts.Platform == "" { - return nil, nil +func (opts *Platform) Parse() error { + if opts.platform == "" { + return nil } // OS[/Arch[/Variant]][:OSVersion] // If Arch is not provided, will use GOARCH instead var platformStr string var p ocispec.Platform - platformStr, p.OSVersion, _ = strings.Cut(opts.Platform, ":") + platformStr, p.OSVersion, _ = strings.Cut(opts.platform, ":") parts := strings.Split(platformStr, "/") switch len(parts) { case 3: @@ -55,14 +54,15 @@ func (opts *Platform) Parse() (*ocispec.Platform, error) { case 1: p.Architecture = runtime.GOARCH default: - return nil, fmt.Errorf("failed to parse platform %q: expected format os[/arch[/variant]]", opts.Platform) + return fmt.Errorf("failed to parse platform %q: expected format os[/arch[/variant]]", opts.platform) } p.OS = parts[0] if p.OS == "" { - return nil, fmt.Errorf("invalid platform: OS cannot be empty") + return fmt.Errorf("invalid platform: OS cannot be empty") } if p.Architecture == "" { - return nil, fmt.Errorf("invalid platform: Architecture cannot be empty") + return fmt.Errorf("invalid platform: Architecture cannot be empty") } - return &p, nil + opts.OCIPlatform = &p + return nil } diff --git a/cmd/oras/internal/option/platform_test.go b/cmd/oras/internal/option/platform_test.go index 246997654..3e01ec01a 100644 --- a/cmd/oras/internal/option/platform_test.go +++ b/cmd/oras/internal/option/platform_test.go @@ -3,9 +3,7 @@ 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. @@ -27,8 +25,8 @@ import ( func TestPlatform_ApplyFlags(t *testing.T) { var test struct{ Platform } ApplyFlags(&test, pflag.NewFlagSet("oras-test", pflag.ExitOnError)) - if test.Platform.Platform != "" { - t.Fatalf("expecting platform to be empty but got: %v", test.Platform.Platform) + if test.Platform.platform != "" { + t.Fatalf("expecting platform to be empty but got: %v", test.Platform.platform) } } @@ -37,15 +35,15 @@ func TestPlatform_Parse_err(t *testing.T) { name string opts *Platform }{ - {name: "empty arch 1", opts: &Platform{"os/"}}, - {name: "empty arch 2", opts: &Platform{"os//variant"}}, - {name: "empty os", opts: &Platform{"/arch"}}, - {name: "empty os with variant", opts: &Platform{"/arch/variant"}}, - {name: "trailing slash", opts: &Platform{"os/arch/variant/llama"}}, + {name: "empty arch 1", opts: &Platform{"os/", nil}}, + {name: "empty arch 2", opts: &Platform{"os//variant", nil}}, + {name: "empty os", opts: &Platform{"/arch", nil}}, + {name: "empty os with variant", opts: &Platform{"/arch/variant", nil}}, + {name: "trailing slash", opts: &Platform{"os/arch/variant/llama", nil}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := tt.opts.Parse() + err := tt.opts.Parse() if err == nil { t.Errorf("Platform.Parse() error = %v, wantErr %v", err, true) return @@ -60,21 +58,20 @@ func TestPlatform_Parse(t *testing.T) { opts *Platform want *ocispec.Platform }{ - {name: "empty", opts: &Platform{""}, want: nil}, - {name: "default arch", opts: &Platform{"os"}, want: &ocispec.Platform{OS: "os", Architecture: runtime.GOARCH}}, - {name: "os&arch", opts: &Platform{"os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, - {name: "empty variant", opts: &Platform{"os/aRcH/"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: ""}}, - {name: "os&arch&variant", opts: &Platform{"os/aRcH/vAriAnt"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt"}}, - {name: "os version", opts: &Platform{"os/aRcH/vAriAnt:osversion"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt", OSVersion: "osversion"}}, - {name: "long os version", opts: &Platform{"os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, + {name: "empty", opts: &Platform{platform: ""}, want: nil}, + {name: "default arch", opts: &Platform{platform: "os"}, want: &ocispec.Platform{OS: "os", Architecture: runtime.GOARCH}}, + {name: "os&arch", opts: &Platform{platform: "os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, + {name: "empty variant", opts: &Platform{platform: "os/aRcH/"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: ""}}, + {name: "os&arch&variant", opts: &Platform{platform: "os/aRcH/vAriAnt"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt"}}, + {name: "os version", opts: &Platform{platform: "os/aRcH/vAriAnt:osversion"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH", Variant: "vAriAnt", OSVersion: "osversion"}}, + {name: "long os version", opts: &Platform{platform: "os/aRcH"}, want: &ocispec.Platform{OS: "os", Architecture: "aRcH"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := tt.opts.Parse() - if err != nil { + if err := tt.opts.Parse(); err != nil { t.Errorf("Platform.Parse() error = %v", err) - return } + got := tt.opts.OCIPlatform if !reflect.DeepEqual(got, tt.want) { t.Errorf("Platform.Parse() = %v, want %v", got, tt.want) } diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 118d75559..4e05f5aeb 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -87,8 +87,8 @@ func (opts *Remote) ApplyFlagsWithPrefix(fs *pflag.FlagSet, prefix, description } } -// ReadPassword tries to read password with optional cmd prompt. -func (opts *Remote) ReadPassword() (err error) { +// Parse tries to read password with optional cmd prompt. +func (opts *Remote) Parse() error { if opts.Password != "" { fmt.Fprintln(os.Stderr, "WARNING! Using --password via the CLI is insecure. Use --password-stdin.") } else if opts.PasswordFromStdin { diff --git a/cmd/oras/login.go b/cmd/oras/login.go index e896d2252..b102444a8 100644 --- a/cmd/oras/login.go +++ b/cmd/oras/login.go @@ -61,7 +61,7 @@ Example - Log in with username and password in an interactive terminal and no TL `, Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.Hostname = args[0] @@ -130,9 +130,9 @@ func runLogin(opts loginOptions) (err error) { return nil } -func readLine(prompt string, slient bool) (string, error) { +func readLine(prompt string, silent bool) (string, error) { fmt.Print(prompt) - if slient { + if silent { fd := os.Stdin.Fd() state, err := term.SaveState(fd) if err != nil { @@ -147,7 +147,7 @@ func readLine(prompt string, slient bool) (string, error) { if err != nil { return "", err } - if slient { + if silent { fmt.Println() } diff --git a/cmd/oras/manifest/delete.go b/cmd/oras/manifest/delete.go index 568eefd91..09751eb2e 100644 --- a/cmd/oras/manifest/delete.go +++ b/cmd/oras/manifest/delete.go @@ -63,7 +63,7 @@ Example - Delete a manifest by digest 'sha256:99e4703fbf30916f549cd6bfa9cdbab614 if opts.OutputDescriptor && !opts.Confirmed { return errors.New("must apply --force to confirm the deletion if the descriptor is outputted") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(_ *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/manifest/fetch.go b/cmd/oras/manifest/fetch.go index 7173a5c42..e4c911f21 100644 --- a/cmd/oras/manifest/fetch.go +++ b/cmd/oras/manifest/fetch.go @@ -69,8 +69,7 @@ Example - Fetch manifest with prettified json result: if opts.outputPath == "-" && opts.OutputDescriptor { return errors.New("`--output -` cannot be used with `--descriptor` at the same time") } - - return opts.ReadPassword() + return option.Parse(&opts) }, Aliases: []string{"get"}, RunE: func(cmd *cobra.Command, args []string) error { @@ -98,11 +97,6 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { } repo.ManifestMediaTypes = opts.mediaTypes - targetPlatform, err := opts.Parse() - if err != nil { - return err - } - src, err := opts.CachedTarget(repo) if err != nil { return err @@ -112,7 +106,7 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { if opts.OutputDescriptor && opts.outputPath == "" { // fetch manifest descriptor only fetchOpts := oras.DefaultResolveOptions - fetchOpts.TargetPlatform = targetPlatform + fetchOpts.TargetPlatform = opts.OCIPlatform desc, err = oras.Resolve(ctx, src, opts.targetRef, fetchOpts) if err != nil { return err @@ -121,7 +115,7 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { // fetch manifest content var content []byte fetchOpts := oras.DefaultFetchBytesOptions - fetchOpts.TargetPlatform = targetPlatform + fetchOpts.TargetPlatform = opts.OCIPlatform desc, content, err = oras.FetchBytes(ctx, src, opts.targetRef, fetchOpts) if err != nil { return err diff --git a/cmd/oras/manifest/fetch_config.go b/cmd/oras/manifest/fetch_config.go index d57fc62ff..2e54eb478 100644 --- a/cmd/oras/manifest/fetch_config.go +++ b/cmd/oras/manifest/fetch_config.go @@ -76,8 +76,7 @@ Example - Fetch and print the prettified descriptor of the config: if opts.outputPath == "-" && opts.OutputDescriptor { return errors.New("`--output -` cannot be used with `--descriptor` at the same time") } - - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] @@ -102,18 +101,13 @@ func fetchConfig(opts fetchConfigOptions) (fetchErr error) { return oerrors.NewErrInvalidReference(repo.Reference) } - targetPlatform, err := opts.Parse() - if err != nil { - return err - } - src, err := opts.CachedTarget(repo) if err != nil { return err } // fetch config descriptor - configDesc, err := fetchConfigDesc(ctx, src, opts.targetRef, targetPlatform) + configDesc, err := fetchConfigDesc(ctx, src, opts.targetRef, opts.OCIPlatform) if err != nil { return err } diff --git a/cmd/oras/manifest/push.go b/cmd/oras/manifest/push.go index 6249cef30..0c172ade5 100644 --- a/cmd/oras/manifest/push.go +++ b/cmd/oras/manifest/push.go @@ -81,7 +81,7 @@ Example - Push a manifest to repository 'locahost:5000/hello' and tag with 'tag1 if opts.fileRef == "-" && opts.PasswordFromStdin { return errors.New("`-` read file from input and `--password-stdin` read password from input cannot be both used") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(_ *cobra.Command, args []string) error { refs := strings.Split(args[0], ",") diff --git a/cmd/oras/pull.go b/cmd/oras/pull.go index c57cc6eb2..630392e07 100644 --- a/cmd/oras/pull.go +++ b/cmd/oras/pull.go @@ -79,7 +79,7 @@ Example - Pull all files with concurrency level tuned: `, Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] @@ -99,10 +99,6 @@ Example - Pull all files with concurrency level tuned: func runPull(opts pullOptions) error { var printed sync.Map - targetPlatform, err := opts.Parse() - if err != nil { - return err - } repo, err := opts.NewRepository(opts.targetRef, opts.Common) if err != nil { return err @@ -125,9 +121,7 @@ func runPull(opts pullOptions) error { return err } } - if targetPlatform != nil { - copyOptions.WithTargetPlatform(targetPlatform) - } + copyOptions.WithTargetPlatform(opts.OCIPlatform) var getConfigOnce sync.Once copyOptions.FindSuccessors = func(ctx context.Context, fetcher content.Fetcher, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) { statusFetcher := content.FetcherFunc(func(ctx context.Context, target ocispec.Descriptor) (fetched io.ReadCloser, fetchErr error) { diff --git a/cmd/oras/push.go b/cmd/oras/push.go index e43c28d07..7a9f15a02 100644 --- a/cmd/oras/push.go +++ b/cmd/oras/push.go @@ -96,7 +96,7 @@ Example - Push file "hi.txt" with multiple tags and concurrency level tuned: if opts.artifactType != "" && opts.manifestConfigRef != "" { return errors.New("--artifact-type and --config cannot both be provided") } - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { refs := strings.Split(args[0], ",") diff --git a/cmd/oras/repository/ls.go b/cmd/oras/repository/ls.go index 40963b404..4355ba307 100644 --- a/cmd/oras/repository/ls.go +++ b/cmd/oras/repository/ls.go @@ -47,7 +47,7 @@ Example - List the repositories under the registry that include values lexically Args: cobra.ExactArgs(1), Aliases: []string{"list"}, PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.hostname = args[0] diff --git a/cmd/oras/repository/tags.go b/cmd/oras/repository/tags.go index abc6cc202..4cbf7287d 100644 --- a/cmd/oras/repository/tags.go +++ b/cmd/oras/repository/tags.go @@ -53,7 +53,7 @@ Example - Show tags of the target repository that include values lexically after Args: cobra.ExactArgs(1), Aliases: []string{"show-tags"}, PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(cmd *cobra.Command, args []string) error { opts.targetRef = args[0] diff --git a/cmd/oras/tag/tag.go b/cmd/oras/tag/tag.go index c4fa8a336..bcad5f796 100644 --- a/cmd/oras/tag/tag.go +++ b/cmd/oras/tag/tag.go @@ -55,7 +55,7 @@ Example - Tag the manifest 'v1.0.1' in 'localhost:5000/hello' to 'v1.0.1', 'v1.0 `, Args: cobra.MinimumNArgs(2), PreRunE: func(cmd *cobra.Command, args []string) error { - return opts.ReadPassword() + return option.Parse(&opts) }, RunE: func(_ *cobra.Command, args []string) error { opts.srcRef = args[0] From aed4e115c87f723822b41cc964adec3df5aad01a Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 17:12:25 +0800 Subject: [PATCH 06/14] add unit test Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser_test.go | 52 +++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 cmd/oras/internal/option/parser_test.go diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go new file mode 100644 index 000000000..98d6f0948 --- /dev/null +++ b/cmd/oras/internal/option/parser_test.go @@ -0,0 +1,52 @@ +/* +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_test + +import ( + "testing" + + "oras.land/oras/cmd/oras/internal/option" +) + +type Test struct { + Cnt int +} + +func (t *Test) Parse() { + t.Cnt++ +} + +func TestParse(t *testing.T) { + type args struct { + Test + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"parse should be called", args{Test{Cnt: 0}}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := option.Parse(&tt.args); (err != nil) != tt.wantErr { + t.Errorf("Parse() error = %v, wantErr %v", err, tt.wantErr) + } + + if tt.args.Cnt != 1 { + t.Errorf("Expect Parse() to be called once but got %v", tt.args.Cnt) + } + }) + } +} From de1002e51690f31b1926f2b050eb987cb01ad8d7 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 17:13:55 +0800 Subject: [PATCH 07/14] bug fix Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go index 98d6f0948..3eab59cbc 100644 --- a/cmd/oras/internal/option/parser_test.go +++ b/cmd/oras/internal/option/parser_test.go @@ -24,7 +24,7 @@ type Test struct { } func (t *Test) Parse() { - t.Cnt++ + t.Cnt += 1 } func TestParse(t *testing.T) { From 1f94c4f0b618635a269062e67ecf5a45caeda838 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Fri, 6 Jan 2023 17:21:43 +0800 Subject: [PATCH 08/14] make runnable Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go index 3eab59cbc..67bf17658 100644 --- a/cmd/oras/internal/option/parser_test.go +++ b/cmd/oras/internal/option/parser_test.go @@ -23,8 +23,9 @@ type Test struct { Cnt int } -func (t *Test) Parse() { +func (t *Test) Parse() error { t.Cnt += 1 + return nil } func TestParse(t *testing.T) { From e7ee62685a8c1f13e8d6fc6312844e13dda48af8 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 10 Jan 2023 09:54:51 +0800 Subject: [PATCH 09/14] resolve comments Signed-off-by: Billy Zha --- cmd/oras/cp.go | 4 +- cmd/oras/discover.go | 2 +- cmd/oras/internal/option/applier.go | 15 ++----- cmd/oras/internal/option/applier_test.go | 49 +++++++++++++++++++++++ cmd/oras/internal/option/parser.go | 18 +++++++-- cmd/oras/internal/option/platform.go | 6 +-- cmd/oras/internal/option/platform_test.go | 2 +- cmd/oras/internal/option/remote.go | 5 +++ cmd/oras/manifest/fetch.go | 4 +- cmd/oras/manifest/fetch_config.go | 2 +- cmd/oras/pull.go | 4 +- 11 files changed, 85 insertions(+), 26 deletions(-) create mode 100644 cmd/oras/internal/option/applier_test.go diff --git a/cmd/oras/cp.go b/cmd/oras/cp.go index 960954921..7fc485d02 100644 --- a/cmd/oras/cp.go +++ b/cmd/oras/cp.go @@ -144,7 +144,9 @@ func runCopy(opts copyOptions) error { copyOptions := oras.CopyOptions{ CopyGraphOptions: extendedCopyOptions.CopyGraphOptions, } - copyOptions.WithTargetPlatform(opts.OCIPlatform) + if opts.Platform.Platform != nil { + copyOptions.WithTargetPlatform(opts.Platform.Platform) + } desc, err = oras.Copy(ctx, src, opts.srcRef, dst, opts.dstRef, copyOptions) } } diff --git a/cmd/oras/discover.go b/cmd/oras/discover.go index 7b61f8739..3063c1a83 100644 --- a/cmd/oras/discover.go +++ b/cmd/oras/discover.go @@ -90,7 +90,7 @@ func runDiscover(opts discoverOptions) error { // discover artifacts resolveOpts := oras.DefaultResolveOptions - resolveOpts.TargetPlatform = opts.OCIPlatform + resolveOpts.TargetPlatform = opts.Platform.Platform desc, err := oras.Resolve(ctx, repo, repo.Reference.Reference, resolveOpts) if err != nil { return err diff --git a/cmd/oras/internal/option/applier.go b/cmd/oras/internal/option/applier.go index 5803a5f76..d8f52e7ea 100644 --- a/cmd/oras/internal/option/applier.go +++ b/cmd/oras/internal/option/applier.go @@ -16,8 +16,6 @@ limitations under the License. package option import ( - "reflect" - "github.com/spf13/pflag" ) @@ -31,14 +29,7 @@ type FlagApplier interface { // NOTE: The option argument need to be a pointer to the options, so its value // becomes addressable. func ApplyFlags(optsPtr interface{}, target *pflag.FlagSet) { - v := reflect.ValueOf(optsPtr).Elem() - for i := 0; i < v.NumField(); i++ { - f := v.Field(i) - if f.CanSet() { - iface := f.Addr().Interface() - if a, ok := iface.(FlagApplier); ok { - a.ApplyFlags(target) - } - } - } + rangeFields(optsPtr, func(fa FlagApplier, err *error) { + fa.ApplyFlags(target) + }) } diff --git a/cmd/oras/internal/option/applier_test.go b/cmd/oras/internal/option/applier_test.go new file mode 100644 index 000000000..b20505559 --- /dev/null +++ b/cmd/oras/internal/option/applier_test.go @@ -0,0 +1,49 @@ +/* +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_test + +import ( + "testing" + + "github.com/spf13/pflag" + "oras.land/oras/cmd/oras/internal/option" +) + +func (t *Test) ApplyFlags(fs *pflag.FlagSet) { + t.Cnt += 1 +} + +func TestApplyFlags(t *testing.T) { + type args struct { + Test + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"flags should be applied once", args{Test{Cnt: 0}}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + option.ApplyFlags(&tt.args, nil) + + if tt.args.Cnt != 1 { + t.Errorf("Expect ApplyFlags() to be called once but got %v", tt.args.Cnt) + } + }) + } +} diff --git a/cmd/oras/internal/option/parser.go b/cmd/oras/internal/option/parser.go index 70bc5e91b..bcba169db 100644 --- a/cmd/oras/internal/option/parser.go +++ b/cmd/oras/internal/option/parser.go @@ -25,14 +25,24 @@ type FlagParser interface { // Parse parses applicable fields of the passed-in option pointer and returns // error during parsing. func Parse(optsPtr interface{}) error { - v := reflect.ValueOf(optsPtr).Elem() + return rangeFields(optsPtr, func(fp FlagParser, err *error) { + *err = fp.Parse() + }) +} + +// rangeFields goes through all fields of ptr, optionally run fn if a field is +// public AND typed T. +func rangeFields[T any](ptr any, fn func(T, *error)) error { + v := reflect.ValueOf(ptr).Elem() for i := 0; i < v.NumField(); i++ { f := v.Field(i) if f.CanSet() { iface := f.Addr().Interface() - if a, ok := iface.(FlagParser); ok { - if err := a.Parse(); err != nil { - return err + if opts, ok := iface.(T); ok { + var err *error + fn(opts, err) + if err != nil && *err != nil { + return *err } } } diff --git a/cmd/oras/internal/option/platform.go b/cmd/oras/internal/option/platform.go index bbad6f84b..dbae84af6 100644 --- a/cmd/oras/internal/option/platform.go +++ b/cmd/oras/internal/option/platform.go @@ -24,8 +24,8 @@ import ( // Platform option struct. type Platform struct { - platform string - OCIPlatform *ocispec.Platform + platform string + Platform *ocispec.Platform } // ApplyFlags applies flags to a command flag set. @@ -63,6 +63,6 @@ func (opts *Platform) Parse() error { if p.Architecture == "" { return fmt.Errorf("invalid platform: Architecture cannot be empty") } - opts.OCIPlatform = &p + opts.Platform = &p return nil } diff --git a/cmd/oras/internal/option/platform_test.go b/cmd/oras/internal/option/platform_test.go index 3e01ec01a..8864bbede 100644 --- a/cmd/oras/internal/option/platform_test.go +++ b/cmd/oras/internal/option/platform_test.go @@ -71,7 +71,7 @@ func TestPlatform_Parse(t *testing.T) { if err := tt.opts.Parse(); err != nil { t.Errorf("Platform.Parse() error = %v", err) } - got := tt.opts.OCIPlatform + got := tt.opts.Platform if !reflect.DeepEqual(got, tt.want) { t.Errorf("Platform.Parse() = %v, want %v", got, tt.want) } diff --git a/cmd/oras/internal/option/remote.go b/cmd/oras/internal/option/remote.go index 4e05f5aeb..0391cb3fb 100644 --- a/cmd/oras/internal/option/remote.go +++ b/cmd/oras/internal/option/remote.go @@ -89,6 +89,11 @@ func (opts *Remote) ApplyFlagsWithPrefix(fs *pflag.FlagSet, prefix, description // Parse tries to read password with optional cmd prompt. func (opts *Remote) Parse() error { + return opts.readPassword() +} + +// readPassword tries to read password with optional cmd prompt. +func (opts *Remote) readPassword() (err error) { if opts.Password != "" { fmt.Fprintln(os.Stderr, "WARNING! Using --password via the CLI is insecure. Use --password-stdin.") } else if opts.PasswordFromStdin { diff --git a/cmd/oras/manifest/fetch.go b/cmd/oras/manifest/fetch.go index e4c911f21..5715141aa 100644 --- a/cmd/oras/manifest/fetch.go +++ b/cmd/oras/manifest/fetch.go @@ -106,7 +106,7 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { if opts.OutputDescriptor && opts.outputPath == "" { // fetch manifest descriptor only fetchOpts := oras.DefaultResolveOptions - fetchOpts.TargetPlatform = opts.OCIPlatform + fetchOpts.TargetPlatform = opts.Platform.Platform desc, err = oras.Resolve(ctx, src, opts.targetRef, fetchOpts) if err != nil { return err @@ -115,7 +115,7 @@ func fetchManifest(opts fetchOptions) (fetchErr error) { // fetch manifest content var content []byte fetchOpts := oras.DefaultFetchBytesOptions - fetchOpts.TargetPlatform = opts.OCIPlatform + fetchOpts.TargetPlatform = opts.Platform.Platform desc, content, err = oras.FetchBytes(ctx, src, opts.targetRef, fetchOpts) if err != nil { return err diff --git a/cmd/oras/manifest/fetch_config.go b/cmd/oras/manifest/fetch_config.go index 2e54eb478..542fa81e7 100644 --- a/cmd/oras/manifest/fetch_config.go +++ b/cmd/oras/manifest/fetch_config.go @@ -107,7 +107,7 @@ func fetchConfig(opts fetchConfigOptions) (fetchErr error) { } // fetch config descriptor - configDesc, err := fetchConfigDesc(ctx, src, opts.targetRef, opts.OCIPlatform) + configDesc, err := fetchConfigDesc(ctx, src, opts.targetRef, opts.Platform.Platform) if err != nil { return err } diff --git a/cmd/oras/pull.go b/cmd/oras/pull.go index 630392e07..0288b28af 100644 --- a/cmd/oras/pull.go +++ b/cmd/oras/pull.go @@ -121,7 +121,9 @@ func runPull(opts pullOptions) error { return err } } - copyOptions.WithTargetPlatform(opts.OCIPlatform) + if opts.Platform.Platform != nil { + copyOptions.WithTargetPlatform(opts.Platform.Platform) + } var getConfigOnce sync.Once copyOptions.FindSuccessors = func(ctx context.Context, fetcher content.Fetcher, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) { statusFetcher := content.FetcherFunc(func(ctx context.Context, target ocispec.Descriptor) (fetched io.ReadCloser, fetchErr error) { From 2f9aff434a9cc8e282fb34df47438cb385e90de3 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 10 Jan 2023 10:04:20 +0800 Subject: [PATCH 10/14] fix error Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/oras/internal/option/parser.go b/cmd/oras/internal/option/parser.go index bcba169db..abf075412 100644 --- a/cmd/oras/internal/option/parser.go +++ b/cmd/oras/internal/option/parser.go @@ -39,9 +39,9 @@ func rangeFields[T any](ptr any, fn func(T, *error)) error { if f.CanSet() { iface := f.Addr().Interface() if opts, ok := iface.(T); ok { - var err *error + err := new(error) fn(opts, err) - if err != nil && *err != nil { + if *err != nil { return *err } } From d915ce611b02e313e71d4739967c61d40fe2c252 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 10 Jan 2023 11:27:16 +0800 Subject: [PATCH 11/14] add test coverage Signed-off-by: Billy Zha --- cmd/oras/internal/option/applier_test.go | 10 +++--- cmd/oras/internal/option/parser_test.go | 45 ++++++++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/cmd/oras/internal/option/applier_test.go b/cmd/oras/internal/option/applier_test.go index b20505559..322287fb9 100644 --- a/cmd/oras/internal/option/applier_test.go +++ b/cmd/oras/internal/option/applier_test.go @@ -23,10 +23,11 @@ import ( ) func (t *Test) ApplyFlags(fs *pflag.FlagSet) { - t.Cnt += 1 + *t.CntPtr += 1 } func TestApplyFlags(t *testing.T) { + cnt := 0 type args struct { Test } @@ -35,14 +36,13 @@ func TestApplyFlags(t *testing.T) { args args wantErr bool }{ - {"flags should be applied once", args{Test{Cnt: 0}}, false}, + {"flags should be applied once", args{Test{CntPtr: &cnt}}, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { option.ApplyFlags(&tt.args, nil) - - if tt.args.Cnt != 1 { - t.Errorf("Expect ApplyFlags() to be called once but got %v", tt.args.Cnt) + if cnt != 1 { + t.Errorf("Expect ApplyFlags() to be called once but got %v", cnt) } }) } diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go index 67bf17658..39f77e492 100644 --- a/cmd/oras/internal/option/parser_test.go +++ b/cmd/oras/internal/option/parser_test.go @@ -14,21 +14,26 @@ limitations under the License. package option_test import ( + "errors" "testing" "oras.land/oras/cmd/oras/internal/option" ) type Test struct { - Cnt int + CntPtr *int } func (t *Test) Parse() error { - t.Cnt += 1 + *t.CntPtr += 1 + if *t.CntPtr == 2 { + return errors.New("should not be tried twice") + } return nil } -func TestParse(t *testing.T) { +func TestParse_once(t *testing.T) { + cnt := 0 type args struct { Test } @@ -37,7 +42,35 @@ func TestParse(t *testing.T) { args args wantErr bool }{ - {"parse should be called", args{Test{Cnt: 0}}, false}, + {"parse should be called once", args{Test{CntPtr: &cnt}}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := option.Parse(&tt.args); (err != nil) != tt.wantErr { + t.Errorf("Parse() error = %v, wantErr %v", err, tt.wantErr) + } + + if cnt != 1 { + t.Errorf("Expect Parse() to be called once but got %v", cnt) + } + }) + } +} + +func TestParse_err(t *testing.T) { + cnt := 0 + type args struct { + Test1 Test + Test2 Test + Test3 Test + Test4 Test + } + tests := []struct { + name string + args args + wantErr bool + }{ + {"parse should be called twice and aborted with error", args{Test{CntPtr: &cnt}, Test{CntPtr: &cnt}, Test{CntPtr: &cnt}, Test{CntPtr: &cnt}}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -45,8 +78,8 @@ func TestParse(t *testing.T) { t.Errorf("Parse() error = %v, wantErr %v", err, tt.wantErr) } - if tt.args.Cnt != 1 { - t.Errorf("Expect Parse() to be called once but got %v", tt.args.Cnt) + if cnt != 2 { + t.Errorf("Expect Parse() to be called twice but got %v", cnt) } }) } From 3370c112ebfd59b138a2e02d5795817b932448f2 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 10 Jan 2023 20:26:29 +0800 Subject: [PATCH 12/14] resolve comments Signed-off-by: Billy Zha --- cmd/oras/internal/option/applier.go | 3 ++- cmd/oras/internal/option/parser.go | 16 +++++++++------- cmd/oras/internal/option/parser_test.go | 2 ++ cmd/oras/internal/option/platform.go | 2 ++ 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/cmd/oras/internal/option/applier.go b/cmd/oras/internal/option/applier.go index d8f52e7ea..f47aba14b 100644 --- a/cmd/oras/internal/option/applier.go +++ b/cmd/oras/internal/option/applier.go @@ -29,7 +29,8 @@ type FlagApplier interface { // NOTE: The option argument need to be a pointer to the options, so its value // becomes addressable. func ApplyFlags(optsPtr interface{}, target *pflag.FlagSet) { - rangeFields(optsPtr, func(fa FlagApplier, err *error) { + rangeFields(optsPtr, func(fa FlagApplier) error { fa.ApplyFlags(target) + return nil }) } diff --git a/cmd/oras/internal/option/parser.go b/cmd/oras/internal/option/parser.go index abf075412..719601859 100644 --- a/cmd/oras/internal/option/parser.go +++ b/cmd/oras/internal/option/parser.go @@ -3,7 +3,9 @@ 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. @@ -25,24 +27,24 @@ type FlagParser interface { // Parse parses applicable fields of the passed-in option pointer and returns // error during parsing. func Parse(optsPtr interface{}) error { - return rangeFields(optsPtr, func(fp FlagParser, err *error) { - *err = fp.Parse() + return rangeFields(optsPtr, func(fp FlagParser) error { + fp.Parse() + return nil }) } // rangeFields goes through all fields of ptr, optionally run fn if a field is // public AND typed T. -func rangeFields[T any](ptr any, fn func(T, *error)) error { +func rangeFields[T any](ptr any, fn func(T) error) error { v := reflect.ValueOf(ptr).Elem() for i := 0; i < v.NumField(); i++ { f := v.Field(i) if f.CanSet() { iface := f.Addr().Interface() if opts, ok := iface.(T); ok { - err := new(error) - fn(opts, err) - if *err != nil { - return *err + fn(opts) + if err := fn(opts); err != nil { + return err } } } diff --git a/cmd/oras/internal/option/parser_test.go b/cmd/oras/internal/option/parser_test.go index 39f77e492..f54a232e5 100644 --- a/cmd/oras/internal/option/parser_test.go +++ b/cmd/oras/internal/option/parser_test.go @@ -3,7 +3,9 @@ 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. diff --git a/cmd/oras/internal/option/platform.go b/cmd/oras/internal/option/platform.go index dbae84af6..fa0fdf363 100644 --- a/cmd/oras/internal/option/platform.go +++ b/cmd/oras/internal/option/platform.go @@ -3,7 +3,9 @@ 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. From abc5ee9fd27a9684f2ed8cdd71408336f0c20986 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 10 Jan 2023 20:35:06 +0800 Subject: [PATCH 13/14] remove duplicated fn Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/oras/internal/option/parser.go b/cmd/oras/internal/option/parser.go index 719601859..21c31eef4 100644 --- a/cmd/oras/internal/option/parser.go +++ b/cmd/oras/internal/option/parser.go @@ -42,7 +42,6 @@ func rangeFields[T any](ptr any, fn func(T) error) error { if f.CanSet() { iface := f.Addr().Interface() if opts, ok := iface.(T); ok { - fn(opts) if err := fn(opts); err != nil { return err } From 3e2d4aedd99cab6487840c8cb0242d45184bf342 Mon Sep 17 00:00:00 2001 From: Billy Zha Date: Tue, 10 Jan 2023 20:37:55 +0800 Subject: [PATCH 14/14] return error when parsing Signed-off-by: Billy Zha --- cmd/oras/internal/option/parser.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/oras/internal/option/parser.go b/cmd/oras/internal/option/parser.go index 21c31eef4..43d85dd0e 100644 --- a/cmd/oras/internal/option/parser.go +++ b/cmd/oras/internal/option/parser.go @@ -28,8 +28,7 @@ type FlagParser interface { // error during parsing. func Parse(optsPtr interface{}) error { return rangeFields(optsPtr, func(fp FlagParser) error { - fp.Parse() - return nil + return fp.Parse() }) }