Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crane: add catalog argument use annotation #1473

Merged
merged 5 commits into from Nov 3, 2022

Conversation

brandtkeller
Copy link
Contributor

Following Cobra Recommended Syntax for "Use", I added the required argument annotation to the command.

While the registry argument may be assumed, I believe this improves the user experience by explicitly defining required arguments. Runtime without a registry Error: accepts 1 arg(s), received 0 and use of the --help flag does not define what the expected argument should be.

@brandtkeller
Copy link
Contributor Author

brandtkeller commented Oct 23, 2022

also considered using the same language/syntax as auth get.

ie crane catalog {REGISTRY_ADDR} or crane catalog [REGISTRY_ADDR]

It does feel required rather than optional - but recommended by cobra doesn't mean that this project doesn't have other preferred practices.

@@ -24,7 +24,7 @@ import (
// NewCmdCatalog creates a new cobra.Command for the repos subcommand.
func NewCmdCatalog(options *[]crane.Option) *cobra.Command {
return &cobra.Command{
Use: "catalog",
Use: "catalog {REGISTRY}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Use: "catalog {REGISTRY}",
Use: "catalog [REGISTRY]",

Yeah let's use the same form we use elsewhere.

It might also help to add a usage section with some examples, so folks know what kinds of values to expect [REGISTRY] to mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to square brackets & included a standard example using some existing patterns.

crane catalog --help
List the repos in a registry

Usage:
  crane catalog [REGISTRY] [flags]

Examples:
  # list the repos for reg.example.com
  $ echo "reg.example.com" | crane catalog
  # or
  $ crane catalog reg.example.com

Flags:
  -h, --help   help for catalog

Global Flags:
      --allow-nondistributable-artifacts   Allow pushing non-distributable (foreign) layers
      --insecure                           Allow image references to be fetched without TLS
      --platform platform                  Specifies the platform in the form os/arch[/variant][:osversion] (e.g. linux/amd64). (default all)
  -v, --verbose                            Enable debug logs

cmd/crane/cmd/catalog.go Outdated Show resolved Hide resolved
cmd/crane/cmd/catalog.go Outdated Show resolved Hide resolved
Co-authored-by: Jason Hall <jason@chainguard.dev>
@codecov-commenter
Copy link

Codecov Report

Merging #1473 (ca480e8) into main (3413eb6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1473   +/-   ##
=======================================
  Coverage   73.18%   73.18%           
=======================================
  Files         115      115           
  Lines        8809     8809           
=======================================
  Hits         6447     6447           
  Misses       1712     1712           
  Partials      650      650           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@imjasonh
Copy link
Collaborator

-  $ /tmp/go-build1373274557/b001/exe/main catalog reg.example.com
+  $ crane catalog reg.example.com

lolwut

How does this work for crane auth login but not here 🤔

@jonjohnsonjr
Copy link
Collaborator

jonjohnsonjr commented Oct 24, 2022

How does this work for crane auth login but not here 🤔

Apologies in advance:

NewCmdAuth(options, "crane", "auth"),

@brandtkeller
Copy link
Contributor Author

Are there any additional steps that I need to make prior to this being clear for merge?

@imjasonh imjasonh merged commit 353a117 into google:main Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants