On error in RunE, do not display usage #340
Comments
|
sadly this is expected because any change might break people. It probably about time someone with time called this v1 of cobra and we start cleaning up mess like this in a v2.... |
|
Yeah, |
|
Thanks for the tips, both |
|
@vincentbernat I am sorry that I am late to the conversation but I agree, I never liked that you had to put When a subcommand is missing and you have |
|
I'd be fine with a change that made it respect SilenceError on a missing submodule... |
|
Okay. I will work on a fix for that tonight. 👍 |
|
@vincentbernat I am unable to recreate the issue. When I use |
|
I was unclear. My initial report was about the fact that you have to use So far, there is no bug for me anymore. I would prefer to not have to use |
|
Sorry to hijack, but I believe this is related (enough) :) -- As best I can tell, Perhaps this could be controlled explicitly on a per-error basis by optionally wrapping errors in an error type that is checked alongside the |
|
Hi all, The thing is that in Hyperledger Fabric as part of the command execution we perform additional checks on the command arguments and if those checks fail we do need the usage message to be displayed to the user. So, simply silencing usage before calling RunE wouldn't work for us, even if it were based on a different flag/setting. Instead I have found that explicitly setting the SilenceUsage to true from within RunE (well, the associated function) when we are done with checking the arguments provides the level of control we need to dictate whether or not the usage message should be displayed on error. So our RunE functions look something like this: do some additional checks on args - in case of error return, usage gets displayed Hope this helps. |
|
@lehors thanks, that was helpful. in my case I didn't need to do any extra checking on inputs so I was able to just use |
Signed-off-by: Pierre-Emmanuel Jacquier <pierre-emmanuel.jacquier@epitech.eu>
Signed-off-by: Pierre-Emmanuel Jacquier <pierre-emmanuel.jacquier@epitech.eu>
Terraform can die part-way through launching a cluster for many reasons, and we want it to be easy for users to clean up. destroy-cluster gets its teardown information from metadata.json, but before this commit we were only writing the file after a successful Terraform run. That left users with failed Terraform runs scrambling to delete their cluster on their own (e.g. with virsh-cleanup.sh or other external tools). One solution to this problem would be to move the Metadata asset before the Cluster asset in targetAssets. But Abhinav wants: * To provide metadata about the cluster (e.g. bootstrap and master IPs) that is only available after Terraform wraps up. * To minimize the number of assets, so no pre-terraform-metadata.json and post-terraform-metadata.json as separate assets. This commit squashes the metadata file into the Cluster asset, so we can fill it in as we go. I've also updated openshift-install to write any files from failed assets before exiting, so we can land the metadata and recovered Terraform state where the user can find them before we die. To avoid nil-dereference panics after this change, I've also updated a number of Files() implementations to avoid returning a [<nil>] slice. The SilenceError and SilenceUsage additions work around [1]. [1]: spf13/cobra#340
Terraform can die part-way through launching a cluster for many reasons, and we want it to be easy for users to clean up. destroy-cluster gets its teardown information from metadata.json, but before this commit we were only writing the file after a successful Terraform run. That left users with failed Terraform runs scrambling to delete their cluster on their own (e.g. with virsh-cleanup.sh or other external tools). One solution to this problem would be to move the Metadata asset before the Cluster asset in targetAssets. But Abhinav wants: * To provide metadata about the cluster (e.g. bootstrap and master IPs) that is only available after Terraform wraps up. * To minimize the number of assets, so no pre-terraform-metadata.json and post-terraform-metadata.json as separate assets. This commit squashes the metadata file into the Cluster asset, so we can fill it in as we go. I've also updated openshift-install to write any files from failed assets before exiting, so we can land the metadata and recovered Terraform state where the user can find them before we die. To avoid nil-dereference panics after this change, I've also updated a number of Files() implementations to avoid returning a [<nil>] slice. The SilenceError and SilenceUsage additions work around [1]. [1]: spf13/cobra#340
Without SilenceUsage, errors in RunE also produce usage, so we have to make our own wrapper that prints errors without usage. See also: spf13/cobra#340 spf13/cobra#914 Closes #159
Before this patch, when the install fail (e.g. some validation failed)
the usage would be displayed. For example, when the minikube minium
version was not met:
% go run ./cmd/cilium install
🔮 Auto-detected Kubernetes kind: minikube
✨ Running "minikube" validation checks
❌ Validation test minimum-version failed: minimum version is ">=2.5.2", found version "1.16.0"
ℹ️ You can disable the test with --disable-check=minimum-version
Error: validation check for kind "minikube" failed: minimum version is ">=2.5.2", found version "1.16.0"
Usage:
cilium install [flags]
Flags:
--cluster-name string Name of the cluster
--datapath-mode string Cilium version to install
--disable-check strings Disable a particular validation check
-h, --help help for install
--namespace string Namespace to install Cilium into (default "kube-system")
--version string Cilium version to install
This patch make it so an install step error would not make cobra show
the usage.
See also spf13/cobra#340
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
Before this patch, when the install fail (e.g. some validation failed)
the usage would be displayed. For example, when the minikube minium
version was not met:
% go run ./cmd/cilium install
🔮 Auto-detected Kubernetes kind: minikube
✨ Running "minikube" validation checks
❌ Validation test minimum-version failed: minimum version is ">=2.5.2", found version "1.16.0"
ℹ️ You can disable the test with --disable-check=minimum-version
Error: validation check for kind "minikube" failed: minimum version is ">=2.5.2", found version "1.16.0"
Usage:
cilium install [flags]
Flags:
--cluster-name string Name of the cluster
--datapath-mode string Cilium version to install
--disable-check strings Disable a particular validation check
-h, --help help for install
--namespace string Namespace to install Cilium into (default "kube-system")
--version string Cilium version to install
This patch make it so an install step error would not make cobra show
the usage.
See also spf13/cobra#340
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
- Added 'cmd.SilenceUsage = true' consistently so that the usage is output if the args are incorrect, but not if there are other problems, e.g. communication error. See spf13/cobra#340 for background on why it's done this way. - Added a few missing argument validations. - Nit in terminology: changed "get" to "list" and "API" to "server"
- Added 'cmd.SilenceUsage = true' consistently so that the usage is output if the args are incorrect, but not if there are other problems, e.g. communication error. See spf13/cobra#340 for background on why it's done this way. - Added a few missing argument validations. - Nit in terminology: changed "get" to "list" and "API" to "server"
|
Seems like most people were actually happy in the end with the state of the silenceUsage/etc. Often cited since people worked around the "issue" but nothing here to fix at this point. Closing. |
Showing the usage implies that the parameters were invalid hence it's very confusing. (From spf13/cobra#340 it appears that this is a popular opinion.) JIRA: HACBS-837
Enabling dependency injection using a cobra.Command factory ----------------------------------------------------------- While searching for ideas on how to make Cobra commands testable, I stumbled upon: - spf13/cobra#1041 (comment) Basically, the idea is to create a factory that can close values around the cobra.Command structure and its functions. init() can call it with sensible defaults, and tests can inject what they want. Injecting factories into the factory ------------------------------------ The problem remained of how to instantiate an AWS config once, on demand, and then use it to create other AWS clients as needed. Eventually I realized that we can inject other factory functions, hence AwsConfigFactoryFunc and DynamoDbFactoryFunc. Though I haven't written any tests, in this way, I can inject factories that return a null AWS config and a DynamoDb with a TestDynamoDbClient. (After I move that lattermost class to testutils, of course.) AWS related interfaces and helpers in cmd/aws.go ------------------------------------------------ I also realized that every command that relies on AWS will have to load the config and check its err value. Hence the NewAwsCommandFunc for wrapping the run function, which takes an additional aws.Config argument. Adding these AWS related interfaces and helpers to cmd/aws.go seemed to sense, as opposed to creating another package. But that's always an option if I change my mind later. Disabling usage on error via Command.SilenceUsage ------------------------------------------------- Finally, it was annoying that whenever the run function returned an error, Cobra would also print the usage message. A quick search turned up the recommendation to set cmd.SilenceUsage = true in the run function, which did the trick: - spf13/cobra#340 (comment)
Before this patch, when the install fail (e.g. some validation failed)
the usage would be displayed. For example, when the minikube minium
version was not met:
% go run ./cmd/cilium install
🔮 Auto-detected Kubernetes kind: minikube
✨ Running "minikube" validation checks
❌ Validation test minimum-version failed: minimum version is ">=2.5.2", found version "1.16.0"
ℹ️ You can disable the test with --disable-check=minimum-version
Error: validation check for kind "minikube" failed: minimum version is ">=2.5.2", found version "1.16.0"
Usage:
cilium install [flags]
Flags:
--cluster-name string Name of the cluster
--datapath-mode string Cilium version to install
--disable-check strings Disable a particular validation check
-h, --help help for install
--namespace string Namespace to install Cilium into (default "kube-system")
--version string Cilium version to install
This patch make it so an install step error would not make cobra show
the usage.
See also spf13/cobra#340
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
Hey!
When an error is triggered by
RunE, usage is displayed. This is a bit odd at this point, the usage is likely to be correct. I am usingRunEto be able to get the errors fromExecute()to display a detailed report. I have to setSilenceUsagein eachRunEcommand. Is that the expected workflow?The text was updated successfully, but these errors were encountered: