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

create command guidance #25130

Merged
merged 1 commit into from May 9, 2016

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented May 4, 2016

kubectl create <resource> commands are multiplying (configmap, namespace, secret (2 flavors and a pull for a third), and serviceaccounts so far) and I think we should agree on the goal of those subcommands.

Right now, kubectl run is an easy entrypoint for new users or users who don't really care which resources are created. It is possible to get stable output from kubectl run, but it really is intent-based: make this image go in some reasonable way. At the other end of the spectrum, you can craft yaml files by hand get exactly the object you want, but that requires pretty deep API knowledge. I think that kubectl create <resource> should be the starting point of the middle ground.

kubectl create <resource> commands should have just enough arguments to create valid objects and the expectation should be that users will use those objects as skeletons to tweak using kubectl edit or the future kubectl set to modify them after the fact. Editing an already existing and valid object is a lot easier than creating one from scratch, so it reduces the barrier to entry.

@kubernetes/kubectl @smarterclayton @bgrant0607 @liggitt

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 4, 2016

`kubectl create <resource>` commands fill the gap between "I want to try kube, but I don't know or care what gets created" (`kubectl run`) and "I want to create exactly this" (author yaml and run `kubectl create -f`).
They provide an easy way to create a valid object without having to know the vagaries of particular kinds, nested fields, and object key typos that are ignored by the yaml/json parser.
Because editing an already created object is easier than authoring on from scratch, these commands only need to have enough parameters to create a valid object and should default as much as is reasonably possible.
Copy link
Member

Choose a reason for hiding this comment

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

s/on/one

Copy link
Member

Choose a reason for hiding this comment

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

If a field is not mutable post creation, it's reasonable that it should have a flag.

@derekwaynecarr
Copy link
Member

Are there examples where we have gone too far? Or is it you know when you see it?

@deads2k
Copy link
Contributor Author

deads2k commented May 4, 2016

Are there examples where we have gone too far? Or is it you know when you see it?

I don't know of any where we've gone overboard yet. I'd like to be sure that we all share a common vision of where we thing kubectl create <resource> commands fit into our other commands so that we can apply some sort of standard on new flags and commands.

@bgrant0607
Copy link
Member

I agree with the PR description.

See also #18064 (create --edit), #24649 (poor-man's package installation), #21648 (set commands).

cc @janetkuo

@@ -2,6 +2,7 @@

- [v1.3.0-alpha.3](#v130-alpha3)
- [Downloads](#downloads)
- [v1.3.0-alpha.3](#v130-alpha3)
Copy link
Member

Choose a reason for hiding this comment

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

How did this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did this happen?

I ran hack/update-generated-docs.sh which I thought I needed to do to update the munged TOC and this appeared. I assumed the script knew what it was doing. Should it come back out?

Copy link
Member

Choose a reason for hiding this comment

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

Presumably backing it would break the verify script.

@david-mcmahon Could you PTAL?

Copy link
Contributor

Choose a reason for hiding this comment

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

That file was updated by hand for the last alpha release and there are some extra lines that the TOC generator is taking into account. I can send a separate PR or you can just remove the second occurrences of these lines in the file:

v1.3.0-alpha.3

Documentation & Examples

Copy link
Member

Choose a reason for hiding this comment

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

@david-mcmahon I'm seeing this happening in other PRs, such as https://github.com/kubernetes/kubernetes/pull/25524/files#r63061933. Are you planning to send a PR to fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was fixed in 39c98a8

@bgrant0607
Copy link
Member

Also relevant: #20304 (stable kubectl behavior), #22569 (kubectl run best practices)

Once that valid object is created, it can be further manipulated using `kubectl edit` or the eventual `kubectl set` commands.

`kubectl create <resource> <special-case>` commands help in cases where are two or more primary "forks" of a command that have different sets of logically required flags.
`kubectl create secret` is a good example, there's a `generic` flavor with keys mapping to files, then there's a `docker-registry` flavor that is tailored for creating an image pull secret,
Copy link
Member

Choose a reason for hiding this comment

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

I would characterize this as performing non-trivial configuration generation/transformation tailored for a common use case.

@bgrant0607 bgrant0607 added e2e-not-required release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 4, 2016
@@ -253,6 +254,18 @@ func (g *NamespaceGeneratorV1) validate() error {
The generator struct (`NamespaceGeneratorV1`) holds the necessary fields for namespace generation. It also satisfies the `kubectl.StructuredGenerator` interface by implementing the `StructuredGenerate() (runtime.Object, error)` method which configures the generated namespace that callers of the generator (`kubectl create namespace` in our case) need to create.
* `--dry-run` should output the resource that would be created, without creating it.

## Create commands
Copy link
Member

Choose a reason for hiding this comment

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

Suggest moving this to a sub-section under "Command conventions"

@bgrant0607
Copy link
Member

Re. going to far: docker run basically exposes everything via the CLI. I view this as a statement that our objective is not the same.

@deads2k
Copy link
Contributor Author

deads2k commented May 5, 2016

Comments addressed.

@fabianofranz
Copy link
Contributor

Looks correct to me.

Overall, one of my concerns is to make sure that kubectl create <resource> commands don't try to be smarter than they are supposed to be. I see it as mostly an arguments+flags way of doing what you can do with yaml/json (with less options, just enough arguments like you mentioned), but definitely not with additional logic. If they end up doing much more that just validating and calling client.<ResourceName>(namespace).Create(foo) then it's likely trying to do too much, and you need another command explicitly tied to the use case in question.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2016
@bgrant0607
Copy link
Member

LGTM, but needs rebase

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2016
@k8s-bot
Copy link

k8s-bot commented May 9, 2016

GCE e2e build/test passed for commit 8e8e9cd.

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit bd2b41c into kubernetes:master May 9, 2016
@deads2k deads2k deleted the create-guidelines branch September 6, 2016 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants