search

knative / client-contrib

Community contributed `kn` plugins.
Apache License 2.0
10 stars 23 forks source link

[kn-admin] Improve domain set command #41

Closed chaozbj closed 4 years ago

chaozbj commented 4 years ago

When I tried to use kn-admin domain set to config a custom domain, I found the below points that we can improve to provide a better user experience:

  1. For the --selector description, we can add an example like: app=prod to help user understand how to use this flag, and also we can tell user the flag can be used multiple times with different selectors.
  2. For the below codes, I talked with @zhanggbj and she agreed that we should check the error returned from splitByEqualSign function, and tell user the detail error message if the given selector is invalid. Also @zhanggbj suggests moving the selector parsing codes into PreRunE phrase. https://github.com/knative/client-contrib/blob/f207c1f9a74266bebeb5a2a32f2a51cf8f9cd87f/plugins/admin/pkg/command/domain/set.go#L62
  3. When I give the wrong flag value or miss the value, the command will output the error message, but also the whole usage will be printed. I think printing usage is a little verbose for user, actually the user can use --help to show the usage details. I tried knative command and no such behavior, I think we'd better keep the same with kn command. Here is an example for our domain command: 
    
☕15:25 ➜ ~/Workspace/Code/github.com/client-contrib/plugins/admin/cmd [master ✘] ./kn-admin domain set -d test.com --selector
Error: flag needs an argument: --selector
Usage:
kn admin domain set [flags]

Flags: -d, --custom-domain string desired custom domain -h, --help help for set --selector strings domain selector

Global Flags: --config string config file (default is $HOME/.config/kn/plugins/admin.yaml)

zhanggbj commented 4 years ago

@chaozbj Thanks for opening this! for 2, I think we do need a precheck in PreRunE for value of --selector and give an suggestion if the input is wrong, but maybe not moving whole splitByEqualSign to PreRunE

chaozbj commented 4 years ago

@chaozbj Thanks for opening this! for 2, I think we do need a precheck in PreRunE for value of --selector and give an suggestion if the input is wrong, but maybe not moving whole splitByEqualSign to PreRunE

Ok, thanks, I will be careful when I am doing these changes.

chaozbj commented 4 years ago

@zhanggbj I made changes for this issue and created PR in my repo: https://github.com/chaozbj/client-contrib/pull/3, the changes are simple, please help me have a look when you are free. I didn't create a formal PR in this repo since I think there are conflicts between our changes and github may can not automatically merge it. So I want to wait your changes to merge first, and then submit a formal PR for my changes. Any problems, please let me know, thanks a lot!

chaozbj commented 4 years ago

/assign I would like to work on it

zhanggbj commented 4 years ago

@chaozbj would you please also handle the comments for domain subcommand and some other wording as suggested here? https://github.com/knative/client-contrib/pull/40#discussion_r444649879 https://github.com/knative/client-contrib/pull/40#discussion_r444652039 https://github.com/knative/client-contrib/pull/40#discussion_r444657724 https://github.com/knative/client-contrib/pull/40#discussion_r444660533

chaozbj commented 4 years ago

@zhanggbj ok, I will handle them.

chaozbj commented 4 years ago

The codes have been merged and would like to close. /close

knative-prow-robot commented 4 years ago

@chaozbj: Closing this issue.

In response to [this](https://github.com/knative/client-contrib/issues/41#issuecomment-656463431): >The codes have been merged and would like to close. >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.