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

chore: Reworked CLI conventions and include more details #831

Merged
merged 5 commits into from
May 9, 2020

Conversation

rhuss
Copy link
Contributor

@rhuss rhuss commented May 7, 2020

This PR tries to add some more details about the CLI conventions that we have established for the kn client. This guideline should also apply for plugin development.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 7, 2020
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2020
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

Left some comments. Overall OK and could use some examples early on as are provided toward the end. I also think we should address:

  1. plurals of names. So kn services create ... should be OK and same kn service create ... Like kubectl
  2. putting verb before resource names. So I would also be in favor of kn create service ... to be same as kn service create ...

If there is an issue open for these, then just list the issue until resolution.

Please also address the auto markdown formatting issues. Otherwise, LGTM.

conventions/cli.md Outdated Show resolved Hide resolved
conventions/cli.md Outdated Show resolved Hide resolved
conventions/cli.md Outdated Show resolved Hide resolved
conventions/cli.md Outdated Show resolved Hide resolved
conventions/cli.md Outdated Show resolved Hide resolved
@rhuss
Copy link
Contributor Author

rhuss commented May 8, 2020

Thanks @maximilien for the feedback, highly appreciated!

  • plurals of names. So kn services create ... should be OK and same kn service create ... Like kubectl

This makes much sense imo and we should be more liberal here. Happy to open an issue for this

  • putting verb before resource names. So I would also be in favor of kn create service ... to be same as kn service create ...

Here I'm not so sure, because of the hierarchy of command we are using. If we allow a verb as the first command, we might hit some issues:

  • kn create help would need to show all creating commands for services, sources, triggers, ...
  • For creating a source like the PingSource, would it be kn create source ping or kn create ping source ? or both ?
  • What's about plugin which have a create in their somewhere in there name ? Are they considered to be shown in kn create help, too ? Should be "create" verbs on the second (aka service) or third (aka source) level be considered ?

I for myself often type kn create service as this is really one of the confusing aspects when using both kubectl and kn. But it was an early design decision to deliberately selected the "noun verb" format (this was before I joined Knative). Even when we decide we should allow this for a fixed set of verbs (describe, create, list, update, delete) this will get technically challenging (especially when dealing with plugins).

Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

conventions/cli.md Outdated Show resolved Hide resolved
conventions/cli.md Outdated Show resolved Hide resolved
conventions/cli.md Outdated Show resolved Hide resolved
Co-authored-by: Navid Shaikh <[email protected]>
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

Co-authored-by: Navid Shaikh <[email protected]>
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

Comment on lines +3 to +4
This document describes the conventions that are used for all `kn` commands and options.
It is normative in the sense that any new feature that introduces or changes user-facing commands or options needs to adhere to the given principles.
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
This document describes the conventions that are used for all `kn` commands and options.
It is normative in the sense that any new feature that introduces or changes user-facing commands or options needs to adhere to the given principles.
This document describes the conventions that are used for all `kn` commands and
options. It is normative in the sense that any new feature that introduces or
changes user-facing commands or options needs to adhere to the given principles.

Comment on lines +6 to 8
Also, the given rules apply to plugins as well.
Especially any plugin that wants to be promoted to the [knative/client-contrib](https://github.com/knative/client-contrib) plugin repository has to adhere to these rules.

Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
Also, the given rules apply to plugins as well.
Especially any plugin that wants to be promoted to the [knative/client-contrib](https://github.com/knative/client-contrib) plugin repository has to adhere to these rules.
Also, the given rules apply to plugins as well. Especially any plugin that wants
to be promoted to the
[knative/client-contrib](https://github.com/knative/client-contrib) plugin
repository has to adhere to these rules.

Comment on lines +18 to +22
So, commands are generally of the form `kn <noun> <verb>` <sup>[1](#foot-1)</sup>, where [<noun>](#noun) is often the name of a resource (e.g. `service`) but can also refer to other concepts (e.g. `plugin` or `config`).
This first noun forms a command group for all the operations you might want to do with that kind
of resource.
Sometimes there can be deeper hierarchies with multiple nouns (`kn <noun-1> <noun-2> .... <verb>`) when it makes sense to structure complex concepts.
A good example is `kn source <source-type> <verb>` which is used like in `kn source ping create`.
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
So, commands are generally of the form `kn <noun> <verb>` <sup>[1](#foot-1)</sup>, where [<noun>](#noun) is often the name of a resource (e.g. `service`) but can also refer to other concepts (e.g. `plugin` or `config`).
This first noun forms a command group for all the operations you might want to do with that kind
of resource.
Sometimes there can be deeper hierarchies with multiple nouns (`kn <noun-1> <noun-2> .... <verb>`) when it makes sense to structure complex concepts.
A good example is `kn source <source-type> <verb>` which is used like in `kn source ping create`.
So, commands are generally of the form `kn <noun> <verb>`
<sup>[1](#foot-1)</sup>, where [<noun>](#noun) is often the name of a resource
(e.g. `service`) but can also refer to other concepts (e.g. `plugin` or
`config`). This first noun forms a command group for all the operations you
might want to do with that kind of resource. Sometimes there can be deeper
hierarchies with multiple nouns (`kn <noun-1> <noun-2> .... <verb>`) when it
makes sense to structure complex concepts. A good example is
`kn source <source-type> <verb>` which is used like in `kn source ping create`.

Comment on lines +24 to +25
`kn` commands take only positional arguments that are used as [identifiers](#identifier).
This identifier is often the name of a resource which identifies the resource uniquely for the current or given namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
`kn` commands take only positional arguments that are used as [identifiers](#identifier).
This identifier is often the name of a resource which identifies the resource uniquely for the current or given namespace.
`kn` commands take only positional arguments that are used as
[identifiers](#identifier). This identifier is often the name of a resource
which identifies the resource uniquely for the current or given namespace.


## Verb
Top-level commands concerning the operation of `kn` itself, like `help` and `version` are also okay.
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
Top-level commands concerning the operation of `kn` itself, like `help` and `version` are also okay.
Top-level commands concerning the operation of `kn` itself, like `help` and
`version` are also okay.


If the same key is given multiple times on the command line, the latter definition overwrites the previous one.

Example:
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
Example:
Example:


# Same as above, but the last HOME "/home" flag overwrites the previous one
kn service update hello-world --env HOME=/root --env USER- --env HOME=/home
```
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
```

Comment on lines +168 to +170
Certain functionality is the same across command groups.
For example, specifying resource requests and limits via flags can be done when managing services but also for sources.
Those common functionalities should share the same conventions, syntax.
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
Certain functionality is the same across command groups.
For example, specifying resource requests and limits via flags can be done when managing services but also for sources.
Those common functionalities should share the same conventions, syntax.
Certain functionality is the same across command groups. For example, specifying
resource requests and limits via flags can be done when managing services but
also for sources. Those common functionalities should share the same
conventions, syntax.

Comment on lines +174 to +177
* Resource limits
* Output formats, i.e. the data formats supported by the `--output` option (which is reused from k8s' _cli-runtime_)
* Sinks
* ....
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
* Resource limits
* Output formats, i.e. the data formats supported by the `--output` option (which is reused from k8s' _cli-runtime_)
* Sinks
* ....
- Resource limits
- Output formats, i.e. the data formats supported by the `--output` option
(which is reused from k8s' _cli-runtime_)
- Sinks
- ....

Commands that output information should support `--output` with a shorthand of
`-o` to choose how to frame their output, and `--template` for supplying
templates to output styles that use them.
<a name="foot-1">1</a>: Note that this differs from the `kubectl` model where this order is vice versa (`kubectl <verb> <noun>`)
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
<a name="foot-1">1</a>: Note that this differs from the `kubectl` model where this order is vice versa (`kubectl <verb> <noun>`)
<a name="foot-1">1</a>: Note that this differs from the `kubectl` model where
this order is vice versa (`kubectl <verb> <noun>`)

Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, rhuss

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 36716c3 into knative:master May 9, 2020
@navidshaikh navidshaikh added this to the v0.15.0 milestone May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants