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

silence usage when kudoctl run commands fail #900

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

a-hilaly
Copy link
Contributor

@a-hilaly a-hilaly commented Oct 7, 2019

What this PR does / why we need it:

Fixes #896
This will prevent kudoctl from printing usage when it subcommands RunE functions fails

@@ -29,6 +29,7 @@ func NewKudoctlCmd() *cobra.Command {
Long: `KUDO CLI and future sub-commands can be used to manipulate, inspect and troubleshoot KUDO-specific CRDs
and serves as an API aggregation layer.
`,
SilenceUsage: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means we'll silence it for all errors, not just syntax errors. I wonder if there's a way to selectively prevent from printing out usage just for some errors 🤔

Copy link
Contributor Author

@a-hilaly a-hilaly Oct 7, 2019

Choose a reason for hiding this comment

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

FWIK cobra will not silence usage for all errors, only those related to RunE functions (this could be a validation error), for example this command
Cobra will still print usage if requested (-h flag is passed) or there is a syntax error.
I don't know/think it is possible to configure Cobra to print usage for only a certain type of errors, i'll try to look for other possible ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see, I think we're still returning error through runE for the non-syntax related errors, should we maybe stop doing that? 🤔

I need to think about this more, I think your solution is correct, I am just wondering if we're not suppressing the usage "too much" now as I think printing it automatically after syntax error is very useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but I'd be ok with that. I like my "usage help" when I ask for it ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zen-dog yep "usage help" will be printed only if you pass and unknown flag or call it using "-h"

@alenkacz exactly ! actually in kudoctl, RunE functions return errors for syntax and non-syntax related errors.

should we maybe stop doing that? 🤔

In my opinion the best thing to do here is use RunE for handling execution errors and Args for handling syntax/args validation errors. command.Args is used to customise argument validation in cobra ( docs here )

for example kubectl init command can be rewritten like this

	cmd := &cobra.Command{
		Use:     "init",
		Short:   "Initialize KUDO on both the client and server",
		Long:    initDesc,
		Example: initExample,
		Args: func(cmd *cobra.Command, args []string) error {
			if len(args) != 0 {
				return errors.New("this command does not accept arguments")
			}
			return i.validate(cmd.Flags());
		},
		RunE: func(cmd *cobra.Command, args []string) error {
			i.home = Settings.Home
			clog.V(8).Printf("init cmd %v", i)
			return i.run()
		},
	}

But i'm still not sure if flags validation should be handled in the Args function in this example

Copy link
Member

Choose a reason for hiding this comment

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

It's worth experimenting with flags validation in the Args fn! TIL. Let's merge this and we can open an issue to try this out on one command and then issues to expand this out to every other command, wdyt @a-hilaly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gerred LGTM 👍 i would be glad to help in this !

@alenkacz
Copy link
Contributor

alenkacz commented Oct 7, 2019

Thanks for the PR @a-hilaly !

Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

🚢

@gerred
Copy link
Member

gerred commented Oct 8, 2019

merging, tyvm @a-hilaly, looks like there's some follow-up work to do here as well!

@gerred gerred merged commit 85d4506 into kudobuilder:master Oct 8, 2019
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.

kubectl kudo displays the help when there is an error
4 participants