docs: kubectl command structure and generators conventions #17366

0xmichalis · 2015-11-17T10:01:04Z

@smarterclayton @fabianofranz @kubernetes/kubectl

k8s-github-robot · 2015-11-17T10:09:59Z

Labelling this PR as size/M

k8s-bot · 2015-11-17T10:30:04Z

GCE e2e test build/test passed for commit e9a2702c0efa40bc87e2167e319dd6f7d4a17c77.

k8s-bot · 2015-11-17T13:18:11Z

GCE e2e test build/test passed for commit cb5fda55b851717949c88bfaba85c8228126b8ce.

deads2k · 2015-11-17T14:21:21Z

docs/devel/kubectl-conventions.md

+)
+
+// NewCmdMine implements the kubectl mine command.
+func NewCmdMine(f *cmdutil.Factory, out io.Writer) *cobra.Command {


Is the intent to document what we have or what we want? Composition is easier if we pass the local and absolute names like we do downstream.

I agree with you though I have mixed feelings about pushing for this pattern here. Downstream we use two different parent names (oc and openshift cli) while here it's only kubectl. Is there any possibility in renaming/aliasing kubectl in the future? @bgrant0607 @brendandburns If not, I don't want to enforce an unnecessary convention.

What are we comparing with? Pointer to code examples?

Most of our commands are passed an additional name which is the name of the parent command:
https://github.com/openshift/origin/blob/08471b135e168e77de716fee9e888fe78cf5f249/pkg/cmd/cli/cmd/process.go#L57
But we are gearing towards passing both the parent and child (active command) names:
https://github.com/openshift/origin/blob/08471b135e168e77de716fee9e888fe78cf5f249/pkg/cmd/cli/cmd/logs.go#L63
Having the command name as a separate constant is helpful when we want to suggest using this command in code elsewhere, for example:
https://github.com/openshift/origin/blob/08471b135e168e77de716fee9e888fe78cf5f249/pkg/cmd/cli/cmd/newapp.go#L295

If we ever want to split kubectl into two or more commands, or make it easy to compose new commands with extension binaries, passing names makes a lot of sense.

bgrant0607 · 2015-11-17T17:41:18Z

docs/devel/kubectl-conventions.md

+
+For every command there should be a `NewCmd<CommandName>` function that creates the command and returns a pointer to a `cobra.Command`, which can later be added to other parent commands to compose the structure tree. There should also be a `<CommandName>Options` struct with a variable to every flag and argument declared by the command (and any other variable required for the command to run). This makes tests and mocking easier. The struct ideally exposes three functions:
+
+* `Complete`: Completes the struct variables with values that may not be directly provided, for example, by flags pointers. Here you will usually take the `args` slice and set the values as appropriate variables, instantiate configs or clients, etc.


This couples the business logic to the command framework, which I want to move away from -- #7311.

Complete should be separate from Validate and Run.

How is that? It's possible that I don't understand you but it seems to me that this structure respects* the separation between business logic (the functionality that the user cares about when running the command?) and the command framework (all the machinery that needs to be in place for the command to work?).

[*] this is not 100% true because sometimes completing and validating may overlap eg. in interactive commands but still this structure is way better than a plain unit.

We likely need multiple conventions:

Interactive commands

Builder-based generic commands, which should move into a more generic client library that has no dependency on cobra

Generators

bgrant0607 · 2015-11-17T17:54:59Z

Thanks. Documenting the convention we desire would be useful.

k8s-bot · 2015-11-18T10:52:28Z

GCE e2e build/test failed for commit 5aad737af61ca4a7929b55f328a6609a303c8ae7.

k8s-bot · 2015-11-20T07:43:14Z

GCE e2e test build/test passed for commit 5aad737af61ca4a7929b55f328a6609a303c8ae7.

k8s-bot · 2015-11-23T11:03:08Z

GCE e2e build/test failed for commit 8f8cae2863d5723747582974a24b81d7fd025f1e.

k8s-bot · 2015-11-23T11:04:54Z

GCE e2e build/test failed for commit d4be40f6dd2874e50141d68e9e43faa3cae72fa1.

0xmichalis · 2015-11-23T11:05:09Z

I've updated the doc to pass the names down and updated kubectl logs to follow. PTAL

k8s-github-robot · 2015-11-23T11:09:24Z

Labelling this PR as size/L

k8s-bot · 2015-11-24T00:41:13Z

GCE e2e build/test failed for commit d4be40f6dd2874e50141d68e9e43faa3cae72fa1.

0xmichalis · 2015-11-26T11:57:52Z

There are also kubectl attach, kubectl annotate, kubectl exec, and almost kubectl convert that currently follow this pattern.

k8s-bot · 2015-11-27T10:33:35Z

GCE e2e build/test failed for commit a6a81f064eb3cb821c65f0722785b66b860cbf4b.

k8s-bot · 2015-12-02T15:27:21Z

GCE e2e build/test failed for commit 2c6a415b4c3cc353d07d2c7f1cb6ab7ac1fc1c19.

k8s-bot · 2015-12-02T15:29:16Z

GCE e2e build/test failed for commit 4d123c7a22755fdb3033e872b590f91d0d81d8cb.

k8s-bot · 2015-12-02T15:35:12Z

GCE e2e build/test failed for commit 41d7b953acc62351282f9b1971bd863e2728bbd3.

0xmichalis · 2015-12-02T15:38:17Z

@bgrant0607 I am taking a first pass at documenting generators (#17833) here. Mind having a look?

bgrant0607 · 2015-12-04T08:57:32Z

Any comments from anyone else on @kubernetes/kubectl? @deads2k? @jlowdermilk?

deads2k · 2015-12-04T12:40:37Z

Any comments from anyone else on @kubernetes/kubectl? @deads2k? @jlowdermilk?

I like the Complete (fine with this not being a method on the struct), Validate, Run code structure. It's worked well in OpenShift.

I'm also ok with explaining what generators are, but I really don't want to encourage their use until we find something more structured.

j3ffml · 2015-12-04T19:43:40Z

docs/devel/kubectl-conventions.md

+
+* `Complete`: Completes the struct variables with values that may not be directly provided, for example, by flags pointers. Here you will usually take the `args` slice and set the values as appropriate variables, instantiate configs or clients, etc.
+* `Validate`: performs validation and returns errors.
+* `Run<CommandName>`: runs the actual command, taking as assumption that the struct is complete with all required values to run, and they are valid.


It's not convention for Run to be a function of the options struct. It's usually a plain function that takes an options struct, as you typically need to pass in the cobra.Command as well.

It's not convention for Run to be a function of the options struct. It's usually a plain function that takes an options struct, as you typically need to pass in the cobra.Command as well.

I think that #7311 suggests moving towards the structure laid out here. Or at least something with a similar intent.

I think #7311 is slightly different; that suggests moving all business logic out of command definitions, which sounds like a good idea. Inside the command we still need access to the cobra.Command, cmdutil.Factory, cmdIn, and cmdOut, to parse flag values, access generators/printer, etc. Current convention is to do that in Run<CommandName>, which can (and should) use a library to handle the business logic aside from parsing input and formatting output. e.g. rollingupdate does this.

The separation between Complete, Validate, and Run makes cleaner where the business logic will exist or most probably called from (Run), and where cobra, factory, etc. fall into (Complete). There are a bunch of commands in OpenShift (openshift/origin#3697) that already use this pattern and a couple of kubectl commands too

ingvagabund · 2016-01-11T14:53:46Z

docs/devel/kubectl-conventions.md

@@ -135,6 +133,151 @@ Updated: 8/27/2015
 * Use "TYPE" for the particular flavor of resource type accepted by kubectl, rather than "RESOURCE" or "KIND"
 * Use "NAME" for resource names

+## Command implementation conventions
+
+For every command there should be a `NewCmd<CommandName>` function that creates the command and returns a pointer to a `cobra.Command`, which can later be added to other parent commands to compose the structure tree. There should also be a `<CommandName>Options` struct with a variable to every flag and argument declared by the command (and any other variable required for the command to run). This makes tests and mocking easier. The struct ideally exposes three functions:


Options in <CommandName>Options struct name makes impression the struct contains only flags. I.e. all values that are passed down by cobra.Command. As the paragraph goes on it says: "with a variable and every flag ... (and any other variable required for the command to run)". Any other variable required for the command to run does not come under Options. It is more configuration related. It would make more sense to use <CommandName>Config instead. Options are part of configuration so as other variables like client, factory, mapper, etc.

I like this proposal. @deads2k ?

I like this proposal. @deads2k ?

I'm fine with pretty much any name for it. I just want to make sure I end up with a value object containing all the information required to perform the action without any direct dependency on commands or flagsets.

k8s-bot · 2016-01-27T15:53:43Z

GCE e2e test build/test passed for commit 0571a7b048017232ab792dd384cb1a3a1cbf6a6e.

k8s-bot · 2016-01-27T15:54:16Z

GCE e2e test build/test passed for commit facf120753dceb8195fdca3313429b343f6ae629.

k8s-bot · 2016-01-27T16:29:47Z

GCE e2e test build/test passed for commit ab3277ea739fc2fef14e6eb37ca58f5f40559dc2.

0xmichalis · 2016-01-27T16:31:55Z

@bgrant0607 updated this doc, ptal when you have time

smarterclayton · 2016-01-27T17:58:12Z

docs/devel/kubectl-conventions.md

-* NOUNs may be specified as TYPE name1 name2 ... or TYPE/name1 TYPE/name2; TYPE is omitted when only a single type is expected
-* Resource types are all lowercase, with no hyphens; both singular and plural forms are accepted
+* kubectl VERB NOUNs for commands that apply to multiple resource types.
+* NOUNs may be specified as TYPE name1 name2 ... or TYPE/name1 TYPE/name2; TYPE is omitted when only a single type is expected.


Or TYPE1,TYPE2,TYPE3/name1

bgrant0607 · 2016-01-29T04:32:02Z

Good enough for now, thanks.

LGTM after adding the comma-separated form pointed out by @smarterclayton

k8s-bot · 2016-01-31T16:53:53Z

GCE e2e test build/test passed for commit 426ef93.

0xmichalis · 2016-01-31T16:58:30Z

@bgrant0607 @smarterclayton comments addressed

bgrant0607 · 2016-02-03T04:48:30Z

LGTM

k8s-github-robot · 2016-02-03T04:54:10Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-02-03T05:23:38Z

GCE e2e test build/test passed for commit 426ef93.

k8s-github-robot · 2016-02-04T19:42:19Z

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

k8s-github-robot · 2016-02-05T19:49:32Z

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

k8s-bot · 2016-02-05T21:41:45Z

GCE e2e test build/test passed for commit 426ef93.

k8s-bot · 2016-02-06T00:50:23Z

GCE e2e test build/test passed for commit 426ef93.

k8s-github-robot · 2016-02-06T19:56:48Z

Automatic merge from submit-queue

Auto commit by PR queue bot

k8s-github-robot assigned thockin Nov 17, 2015

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 17, 2015

wojtek-t added the e2e-not-required label Nov 17, 2015

deads2k reviewed Nov 17, 2015
View reviewed changes

bgrant0607 assigned bgrant0607 and unassigned thockin Nov 17, 2015

bgrant0607 reviewed Nov 17, 2015
View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 23, 2015

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2015

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2015

j3ffml reviewed Dec 4, 2015
View reviewed changes

0xmichalis mentioned this pull request Dec 14, 2015

Initial node drain implementation for #3885. #16698

Merged

ingvagabund reviewed Jan 11, 2016
View reviewed changes

0xmichalis changed the title ~~kubectl: Doc conventions regarding command structure~~ docs: kubectl command structure and generators conventions Jan 27, 2016

smarterclayton reviewed Jan 27, 2016
View reviewed changes

bgrant0607 mentioned this pull request Jan 29, 2016

Add kubectl rollout undo #19835

Merged

docs: kubectl command structure and generators conventions

426ef93

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2016

wojtek-t added the cla: human-approved label Feb 6, 2016

k8s-github-robot pushed a commit that referenced this pull request Feb 6, 2016

Merge pull request #17366 from kargakis/kubectl-conventions

c6a4f84

Auto commit by PR queue bot

k8s-github-robot merged commit c6a4f84 into kubernetes:master Feb 6, 2016

0xmichalis deleted the kubectl-conventions branch February 7, 2016 01:04

xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016

Merge pull request kubernetes#17366 from kargakis/kubectl-conventions

3097eee

Auto commit by PR queue bot


		For every command there should be a `NewCmd<CommandName>` function that creates the command and returns a pointer to a `cobra.Command`, which can later be added to other parent commands to compose the structure tree. There should also be a `<CommandName>Options` struct with a variable to every flag and argument declared by the command (and any other variable required for the command to run). This makes tests and mocking easier. The struct ideally exposes three functions:

		* `Complete`: Completes the struct variables with values that may not be directly provided, for example, by flags pointers. Here you will usually take the `args` slice and set the values as appropriate variables, instantiate configs or clients, etc.

docs: kubectl command structure and generators conventions #17366

docs: kubectl command structure and generators conventions #17366

Conversation

0xmichalis commented Nov 17, 2015

k8s-github-robot commented Nov 17, 2015

k8s-bot commented Nov 17, 2015

k8s-bot commented Nov 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Nov 17, 2015

k8s-bot commented Nov 18, 2015

k8s-bot commented Nov 20, 2015

k8s-bot commented Nov 23, 2015

k8s-bot commented Nov 23, 2015

0xmichalis commented Nov 23, 2015

k8s-github-robot commented Nov 23, 2015

k8s-bot commented Nov 24, 2015

0xmichalis commented Nov 26, 2015

k8s-bot commented Nov 27, 2015

k8s-bot commented Dec 2, 2015

k8s-bot commented Dec 2, 2015

k8s-bot commented Dec 2, 2015

0xmichalis commented Dec 2, 2015

bgrant0607 commented Dec 4, 2015

deads2k commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Jan 27, 2016

k8s-bot commented Jan 27, 2016

k8s-bot commented Jan 27, 2016

0xmichalis commented Jan 27, 2016

Choose a reason for hiding this comment

bgrant0607 commented Jan 29, 2016

k8s-bot commented Jan 31, 2016

0xmichalis commented Jan 31, 2016

bgrant0607 commented Feb 3, 2016

k8s-github-robot commented Feb 3, 2016

k8s-bot commented Feb 3, 2016

k8s-github-robot commented Feb 4, 2016

k8s-github-robot commented Feb 5, 2016

k8s-bot commented Feb 5, 2016

k8s-bot commented Feb 6, 2016

k8s-github-robot commented Feb 6, 2016