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

Improve help and positional arg enforcement in most commands #161

Merged
merged 4 commits into from
Oct 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/ocm/account/orgs/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ var Cmd = &cobra.Command{
Use: "orgs",
Short: "List organizations.",
Long: "Display a list of organizations.",
Args: cobra.NoArgs,
RunE: run,
}

Expand Down
1 change: 1 addition & 0 deletions cmd/ocm/account/quota/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var Cmd = &cobra.Command{
Use: "quota",
Short: "Retrieve cluster quota information.",
Long: "Retrieve cluster quota information of a specific organization.",
Args: cobra.NoArgs,
RunE: run,
}

Expand Down
12 changes: 9 additions & 3 deletions cmd/ocm/account/roles/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,16 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "roles [role-name]",
Use: "roles [flags] [ROLE_NAME]",
Short: "Retrieve information of the different roles",
Long: "Get description of a role or list of all roles ",
RunE: run,
Long: "Get description of a role or list of all roles",
Args: func(cmd *cobra.Command, args []string) error {
Copy link
Member

Choose a reason for hiding this comment

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

You could use cobra.MaximumNArgs(1) here.

Copy link
Contributor Author

@cben cben Oct 23, 2020

Choose a reason for hiding this comment

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

For now I figured custom validation lets me have better control of messages.
In this case, MaximumNArgs(1) would print:

Error: accepts at most 1 arg(s), received 2

which is not bad, but I felt saying "at most 1 role name" is more helpful.
I dumped my thoughts on this spf13/cobra#571 (comment)

Part of the problem using built-in validation funcs for us is that we disabled printing usage on error (SilenceUsage: true).
The default behavior of dumping full description of all args on any error is too annoying for my taste too.
I think best behavior would be to print just the Usage: ocm account roles [flags] [ROLE_NAME] line on errors (user can always --help for details); if we had that it'd be reasonable to go back to cobra.MaximumNArgs and friends.
Opened spf13/cobra#1252, currently we can't SetUsageTemplate to something briefer without breaking --help, working on upstream improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did add/leave many cobra.*Args and similar, without reviewing each message, largely because of laziness — this PR actually grew from 1 command with wrong help I wanted to fix 🤣

Copy link
Member

Choose a reason for hiding this comment

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

Read your threads upstream and I agree, if we can expose UseLine() when Arg fails validation that would get us very close to full-lazy cmd definitions :)

if len(args) > 1 {
return fmt.Errorf("Accepts at most 1 role name")
}
return nil
},
RunE: run,
}

func init() {
Expand Down
1 change: 1 addition & 0 deletions cmd/ocm/account/status/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ var Cmd = &cobra.Command{
Use: "status",
Short: "Status of current user.",
Long: "Display status of current user.",
Args: cobra.NoArgs,
RunE: run,
}

Expand Down
1 change: 1 addition & 0 deletions cmd/ocm/account/users/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ var Cmd = &cobra.Command{
Use: "users",
Short: "Retrieve users and their roles",
Long: "Retrieve information of all users/roles in the same organization",
Args: cobra.NoArgs,
RunE: run,
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/ocm/cluster/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ var args struct {

// Cmd Constant:
var Cmd = &cobra.Command{
Use: "create [flags] <cluster name>",
Use: "create [flags] CLUSTER_NAME",
Short: "Create managed clusters",
Long: "Create managed OpenShift Dedicated v4 clusters via OCM",
Deprecated: "please use `ocm create cluster` command",
Expand Down
2 changes: 1 addition & 1 deletion cmd/ocm/cluster/describe/describe.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "describe NAME|ID|EXTERNAL_ID [--output] [--short]",
Use: "describe [flags] {NAME|ID|EXTERNAL_ID}",
Short: "Describe a cluster",
Long: "Get info about a cluster identified by name, identifier or external identifier",
Deprecated: "please use `ocm describe cluster` command",
Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/cluster/list/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ var args struct {

// Cmd Constant:
var Cmd = &cobra.Command{
Use: "list [flags] [partial cluster ID or name]",
Use: "list [flags] [PARTIAL_CLUSTER_ID_OR_NAME]",
Short: "List clusters",
Long: "List clusters by ID and Name",
Long: "List clusters, optionally filtering by substring of ID or Name",
Deprecated: "please use `ocm list clusters` command",
Args: cobra.RangeArgs(0, 1),
RunE: run,
Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/cluster/login/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "login [CLUSTERID|CLUSTER_NAME|CLUSTER_NAME_SEARCH]",
Use: "login [flags] {CLUSTER_ID|CLUSTER_NAME|CLUSTER_NAME_SEARCH}",
Short: "login to a cluster",
Long: "login to a cluster by ID or Name or cluster name search string according to the api: " +
"https://api.openshift.com/#/clusters/get_api_clusters_mgmt_v1_clusters",
Example: " ocm cluster login <id>\n ocm cluster login %test%",
RunE: run,
Args: func(cmd *cobra.Command, args []string) error {
if len(args) < 1 {
if len(args) != 1 {
return fmt.Errorf("cluster name expected")
}

Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/cluster/status/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
)

var Cmd = &cobra.Command{
Use: "status CLUSTERID",
Use: "status [flags] CLUSTER_ID",
Short: "Status of a cluster",
Long: "Get the status of a cluster identified by its cluster ID",
RunE: run,
Expand All @@ -33,7 +33,7 @@ var Cmd = &cobra.Command{
func run(cmd *cobra.Command, argv []string) error {

if len(argv) != 1 {
return fmt.Errorf("Expected exactly one cluster")
return fmt.Errorf("Expected exactly one cluster id")
}

// Create the client for the OCM API:
Expand Down
6 changes: 1 addition & 5 deletions cmd/ocm/cluster/versions/versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,11 @@ var Cmd = &cobra.Command{
Short: "List available versions",
Long: "List the versions available for provisioning a cluster",
Deprecated: "please use `ocm list versions` command",
Args: cobra.NoArgs,
RunE: run,
}

func run(cmd *cobra.Command, argv []string) error {

if len(argv) != 0 {
return fmt.Errorf("Expected no arguments")
}

// Create the client for the OCM API:
connection, err := ocm.NewConnection().Build()
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions cmd/ocm/completion/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ To configure your bash shell to load completions for each session add to your ba
# ~/.bashrc or ~/.profile
. <(ocm completion)
`,
Args: cobra.NoArgs,
RunE: run,
}

Expand Down
6 changes: 5 additions & 1 deletion cmd/ocm/config/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ or ~/.ocm.json if that variable is unset. Currently using: %s

The following variables are supported:

%s`, loc, configVarDocs())
%s

Note that "ocm config get access_token" gives whatever the file contains - may be missing or expired;
you probably want "ocm token" command instead which will obtain a fresh token if needed.
`, loc, configVarDocs())
return
}

Expand Down
6 changes: 3 additions & 3 deletions cmd/ocm/config/get/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "get VARIABLE",
Use: "get [flags] VARIABLE",
Short: "Prints the value of a config variable",
Long: "Prints the value of a config variable. See 'ocm config --help' for supported config variables.",
Args: cobra.MinimumNArgs(1),
Args: cobra.ExactArgs(1),
RunE: run,
}

Expand Down Expand Up @@ -76,7 +76,7 @@ func run(cmd *cobra.Command, argv []string) error {
case "url":
fmt.Fprintf(os.Stdout, "%s\n", cfg.URL)
default:
fmt.Fprintf(os.Stderr, "Uknown setting\n")
return fmt.Errorf("Unknown setting")
}

return nil
Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/config/set/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "set VARIABLE VALUE",
Use: "set [flags] VARIABLE VALUE",
Short: "Sets the variable's value",
Long: "Sets the value of a config variable. See 'ocm config --help' for supported config variables.",
Args: cobra.MinimumNArgs(2),
Args: cobra.ExactArgs(2),
RunE: run,
}

Expand Down
9 changes: 5 additions & 4 deletions cmd/ocm/create/cluster/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,12 @@ var args struct {

// Cmd Constant:
var Cmd = &cobra.Command{
Use: "cluster",
Use: "cluster [flags] NAME",
Short: "Create managed clusters",
Long: "Create managed OpenShift Dedicated v4 clusters via OCM",
Args: cobra.ExactArgs(1),
RunE: run,
Long: "Create managed OpenShift Dedicated v4 clusters via OCM.\n" +
"\n" +
"The specified name is used as a DNS component, as well as initial display_name.",
RunE: run,
}

func init() {
Expand Down
2 changes: 1 addition & 1 deletion cmd/ocm/create/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

var Cmd = &cobra.Command{
Use: "create RESOURCE [flags]",
Use: "create [flags] RESOURCE",
Aliases: []string{"add"},
Short: "Create a resource from stdin",
Long: "Create a resource from stdin",
Expand Down
5 changes: 3 additions & 2 deletions cmd/ocm/create/idp/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,14 @@ var args struct {
var validIdps = []string{"github", "google", "ldap", "openid"}

var Cmd = &cobra.Command{
Use: "idp",
Use: "idp --cluster={NAME|ID|EXTERNAL_ID}",
Short: "Add IDP for cluster",
Long: "Add an Identity providers to determine how users log into the cluster.",
Example: ` # Add a GitHub identity provider to a cluster named "mycluster"
ocm create idp --type=github --cluster=mycluster
# Add an identity provider following interactive prompts
ocm create idp --cluster=mycluster`,
Args: cobra.NoArgs,
RunE: run,
}

Expand All @@ -81,7 +82,7 @@ func init() {
"cluster",
"c",
"",
"Name or ID of the cluster to add the IdP to (required).",
"Name or ID or external_id of the cluster to add the IdP to (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("cluster")
Expand Down
5 changes: 3 additions & 2 deletions cmd/ocm/create/ingress/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "ingress",
Use: "ingress --cluster={NAME|ID|EXTERNAL_ID}",
Aliases: []string{"route", "routes", "ingresses"},
Short: "Add Ingress to cluster",
Long: "Add an Ingress endpoint to determine API access to the cluster.",
Expand All @@ -40,6 +40,7 @@ var Cmd = &cobra.Command{
ocm create ingress --cluster=mycluster
# Add an ingress with route selector label match
ocm create ingress -c mycluster --label-match="foo=bar,bar=baz"`,
Args: cobra.NoArgs,
RunE: run,
}

Expand All @@ -51,7 +52,7 @@ func init() {
"cluster",
"c",
"",
"Name or ID of the cluster to add the ingress to (required).",
"Name or ID or external_id of the cluster to add the ingress to (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("cluster")
Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/create/machinepool/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "machinepool",
Use: "machinepool --cluster={NAME|ID|EXTERNAL_ID} --replicas=N [flags] MACHINE_POOL_ID",
Aliases: []string{"machinepools", "machine-pool", "machine-pools"},
Short: "Add machine pool to cluster",
Long: "Add a machine pool to the cluster.",
Expand All @@ -52,7 +52,7 @@ func init() {
"cluster",
"c",
"",
"Name or ID of the cluster to add the machine pool to (required).",
"Name or ID or external_id of the cluster to add the machine pool to (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("cluster")
Expand Down
10 changes: 5 additions & 5 deletions cmd/ocm/create/user/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "user",
Use: "user --cluster={NAME|ID|EXTERNAL_ID} --group=GROUP_ID [flags] USERS",
Aliases: []string{"users"},
Short: "Configure user access for cluster",
Long: "Configure user access for cluster",
Example: `# Add users to the dedicated-admins group
Long: "Add users (comma-separated) to a priviledged group on a cluster.",
Example: ` # Add users to the dedicated-admins group
ocm create user user1,user2 --cluster=mycluster --group=dedicated-admins`,
RunE: run,
}
Expand All @@ -47,7 +47,7 @@ func init() {
"cluster",
"c",
"",
"Name or ID of the cluster to add the user to (required).",
"Name or ID or external_id of the cluster to add the user to (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("cluster")
Expand All @@ -56,7 +56,7 @@ func init() {
&args.group,
"group",
"",
"Group name to add the users to.",
"Group name to add the users to (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("group")
Expand Down
2 changes: 1 addition & 1 deletion cmd/ocm/delete/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "delete PATH",
Use: "delete [flags] PATH",
Short: "Send a DELETE request",
Long: "Send a DELETE request to the given path.",
RunE: run,
Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/delete/idp/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "idp [IDP NAME]",
Use: "idp --cluster={NAME|ID|EXTERNAL_ID} [flags] IDP_NAME",
Aliases: []string{"idps"},
Short: "Delete cluster IDPs",
Long: "Delete a specific identity provider for a cluster.",
Expand All @@ -45,7 +45,7 @@ func init() {
"cluster",
"c",
"",
"Name or ID of the cluster to delete the IdP from (required).",
"Name or ID or external_id of the cluster to delete the IdP from (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("cluster")
Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/delete/ingress/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "ingress",
Use: "ingress --cluster={NAME|ID|EXTERNAL_ID} [flags] INGRESS_ID",
Aliases: []string{"ingresses", "route", "routes"},
Short: "Delete cluster ingress",
Long: "Delete the additional non-default application router for a cluster.",
Expand All @@ -49,7 +49,7 @@ func init() {
"cluster",
"c",
"",
"Name or ID of the cluster to delete the ingress from (required).",
"Name or ID or external_id of the cluster to delete the ingress from (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("cluster")
Expand Down
4 changes: 2 additions & 2 deletions cmd/ocm/delete/machinepool/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var args struct {
}

var Cmd = &cobra.Command{
Use: "machinepool",
Use: "machinepool --cluster={NAME|ID|EXTERNAL_ID} [flags] MACHINE_POOL_ID",
Aliases: []string{"machine-pool", "machinepools", "machine-pools"},
Short: "Delete cluster machine pool",
Long: "Delete the additional machine pool of a cluster.",
Expand All @@ -44,7 +44,7 @@ func init() {
"cluster",
"c",
"",
"Name or ID of the cluster to delete the machine pool from (required).",
"Name or ID or external_id of the cluster to delete the machine pool from (required).",
)
//nolint:gosec
Cmd.MarkFlagRequired("cluster")
Expand Down
Loading