feat!: change describe command to info
#474
Conversation
The describe command conflicts sematically with kubectl describe. This commit changes the command name to `info`. Fixes: knative#337 Signed-off-by: Lance Ball <lball@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
approved
| @@ -31,7 +31,7 @@ func NewDescriber(namespaceOverride string) (describer *Describer, err error) { | |||
| // restricts to label-syntax, which is thus escaped. Therefore as a knative (kube) implementation | |||
| // detal proper full names have to be escaped on the way in and unescaped on the way out. ex: | |||
| // www.example-site.com -> www-example--site-com | |||
| func (d *Describer) Describe(ctx context.Context, name string) (description fn.Description, err error) { | |||
| func (d *Describer) Describe(ctx context.Context, name string) (description fn.Info, err error) { | |||
There was a problem hiding this comment.
func (d *Describer) Describe(ctx context.Context, name string) (description fn.Info, err error) {
Shouldn't this be changed as well?
There was a problem hiding this comment.
I thought about that and decided not to change it for a couple of reasons. I meant to put this in the PR description, but just forgot.
Infoeris a terrible name! 🤣- The
Describerin theknativepackage is responsible for the verb "describe" which the client uses to surface the noun "info" and they don't need to necessarily evolve 1:1 😎
But now by the light of morning, those reasons seem pretty weak. Do you have any suggestions for naming? I don't particularly like this.
type Infoer struct {
// ...
}
func (i *Infoer) Info(ctx context.Context, name string) (description fn.Info, err error)What do you think of this?
type Info struct {
// ...
}
func (i *Info) Fetch(ctx context.Context, name string) (description fn.Info, err error)
There was a problem hiding this comment.
Hm, seems like a tradeoff either way. I agree that Describer seems a lot nicer than Infoer, and that it's fine if the CLI and client library aren't exactly mapped, if it means the client library is closer to what we think is "correct". I could go either way on it, but leaving it Describe fits for me: run the info command to get a description.
There was a problem hiding this comment.
Hm, seems like a tradeoff either way. I agree that Describer seems a lot nicer than Infoer, and that it's fine if the CLI and client library aren't exactly mapped, if it means the client library is closer to what we think is "correct". I could go either way on it, but leaving it Describer making a Description still fits I think: "run the info command to get more information, including a description from the describer" seems like a well-formed sentence, which is a good litmus test. Not to mention, there is the possibility that info may display more than just the description from the describer in the future...
There was a problem hiding this comment.
Not to mention, there is the possibility that
infomay display more than just the description from the describer in the future...
Thanks for mentioning this. I avoided saying it because it's easy to fall down the flexibility hole.
|
/lgtm |
Changes
describecommand toinfo/kind enhancement
Fixes: #337
The describe command conflicts sematically with kubectl describe.
Release Note
Docs
Documentation changes are included in this PR.