Update Kustomization integration KEP #684
Conversation
pwittrock
commented
Jan 10, 2019
pwittrock
commented
- add more background and context.
- revisit specifics of UX.
- provide justification for not using plugins.
- add more alternatives considered.
- add more background and context. - revisit specifics of UX. - provide justification for not using plugins. - add more alternatives considered.
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
size/L
|
/assign @monopole |
|
/assign @liggitt |
|
/assign @soltysh |
|
/assign @seans3 |
|
cc @kubernetes/sig-cli-maintainers |
| ### Option A: Kustomize Command | ||
|
|
||
| Publish the `kustomize build` command as `kubectl kustomize`. Update documentation demonstrate using Kustomize as | ||
| `kubectl kustomize -f dir/ | kubectl kustomize -f -`. Consider Option B as a follow up if there is consensus on |
There was a problem hiding this comment.
The second "kustomize" here should be "apply:"
kubectl kustomize -f dir/ | kubectl apply -f -.
It is unfortunate that this requires two separate invocations of kubectl.
There was a problem hiding this comment.
:( . Will address that in a follow up KEP
|
|
||
| - It is consistent with how tools that produce Resource Config but exist outside kubectl would integrate with | ||
| `kubectl apply` | ||
| - It is clear to the user that they are getting a new behavior than when they ran `kubectl apply -f dir/` |
| which was relatively restrictive in terms of what it did. | ||
| - Publishing it as a separate command keeps what the other kubectl commands do simpler | ||
| - We have talked about moving towards this approach for how we teach users to work with imperative generators - | ||
| e.g. `kubectl create deployment -o yaml --dry-run | kubectl apply -f -` (or something that doesn't exist yet like |
There was a problem hiding this comment.
It looks like the end of this sentence after "like" is missing.
| - Publishing it as a separate command keeps what the other kubectl commands do simpler | ||
| - We have talked about moving towards this approach for how we teach users to work with imperative generators - | ||
| e.g. `kubectl create deployment -o yaml --dry-run | kubectl apply -f -` (or something that doesn't exist yet like | ||
| - **Note:** this case still has friction (e.g. it defaults fields - such as creationTimestamp to null - yuck) |
There was a problem hiding this comment.
Use quotation marks or some formatting for the "creationTimestamp" field name.
| #### apply | ||
| - It can't address user friction requiring deeper integration - e.g. produce meaningful line numbers in error | ||
| messages. | ||
| - Most commands would require the same input pipe - e.g. get, delete, etc would all need pipes |
| - We don't do this for file, dir or url targets - e.g. we don't do `curl something | kubectl apply -f -` | ||
| - When demoed, the UX was considered more complicated than directly integrating into the resource builder | ||
| - It’s more typing for the user | ||
| - Kustomize is the way we are addressing issues with declarative workflows so it should be integrated into them. |
There was a problem hiding this comment.
I have wondered when to capitalize kustomize. Here it's at the beginning of the sentence, but elsewhere it's still written as all lowercase even in this position. That's more of a branding concern, but this caught my eye.
| ### Implementation Details/Notes/Constraints | ||
| #### Why we like this approach | ||
|
|
||
| - It is capable of friction that requires deeper integration - such as producing errors referencing line |
There was a problem hiding this comment.
Did you really mean "capable of friction," or should that have been something like, "It removes friction," or "It is capable of reducing friction?"
|
|
||
| - It is capable of friction that requires deeper integration - such as producing errors referencing line | ||
| numbers of the original files (rather than the output files). | ||
| - It integrates will all commands - get, delete, etc - would automatically work. |
|
This KEP is for the feature issue #633 |
There was a problem hiding this comment.
Since this KEP is meant as a decision point it should clearly state the winner, iow. that we decided to make kustomize optional built-in resource builder functionality, which means it will kick in when -f points to a kustomize.yaml file, in all other cases it will completely ignore kustomize.yaml and will proceed behaving like these days. This ensures opt-in behavior and backwards compatibility.
|
|
||
| [Kustomize](https://github.com/kubernetes-sigs/kustomize) | ||
| was developed as a subproject of sig-cli by kubectl maintainers with the goal of addressing | ||
| a collection of issues creating friction for declarative workflows in kubectl (e.g. `kubectl apply`). The |
There was a problem hiding this comment.
Since Brian nicely pointed out those issues, can we link them here for posterity?
There was a problem hiding this comment.
OK, I see you've added them below, mind adding a word they are linked below. People will look for them.
| - composing collections of Resource config across files and directories | ||
| - layering the above on top of one another | ||
|
|
||
| It is independent of, but complementary to, the *server-side apply* initiative that was started later and targeted |
There was a problem hiding this comment.
Link to #555 for lazy people like myself 😄
| - Exposing all kubectl generators and schema-aware transformations | ||
| - Providing simpler alternatives to the APIs for declaring Resource Config (e.g. a simple way to create deployments) | ||
| - Providing a templating mechanism (e.g. for generating Resource Config) | ||
| - Publishing with kubectl any other Kustomize sub-commands besides `build` |
There was a problem hiding this comment.
That's the one I had biggest issues with in the original PR. I'd like to narrow the list of those commands to a specific set. I don't want to end up with nasty commands like kubectl kustomize edit myresource which will confuse people with kubectl edit myresource, etc.
There was a problem hiding this comment.
Can we just keep this as a non-goal and not do it?
There was a problem hiding this comment.
Do we need to include it, now that we agree it won't be?
| #### Why don't like this approach | ||
|
|
||
| - Kustomize does more than the resource builder currently does and this could surprise users | ||
| - Commands are doing more than they were before |
There was a problem hiding this comment.
Not backwards compatible, which is a big deal breaker!
There was a problem hiding this comment.
Is it not? The error behavior is a bit different, but you would get an error before.
There was a problem hiding this comment.
The one where it picks kustomize.yaml based on name is breaking backwards compatibility. Because beforehand nothing as such would be picked or iirc would fail. With kustomize in it will pick it up and proceed with transformations, that's not backwards compatible. With the option where you explicitly point to kustomize.yaml that is fine.
There was a problem hiding this comment.
If it used to fail doesn't anymore does this break backwards compatibility? If it used to create some resources but fail on the kustomize resource and then exit non-0 is that breaking backwards compatibility? Guarantees do we provide in the cases where a command fails, are the documented?
There was a problem hiding this comment.
Addressing this by pulling into another KEP
| - It’s more typing for the user | ||
| - Kustomize is the way we are addressing issues with declarative workflows so it should be integrated into them. | ||
|
|
||
| ### Option B / C: Integration into resource builder |
There was a problem hiding this comment.
I'd like to see these 2 separated, there are distinct pros and cons of each that needs to clearly outlined and currently are missing.
| 30+ commands x 30MB). Before going down this route, we should consider how to we might want to design a solution | ||
| and the tradeoffs. | ||
|
|
||
| **AI's for kubectl extensibility:** |
There was a problem hiding this comment.
I'm not convinced these are meant to be here. They make sense in the overall kubectl sense, but not tide with kubectl extensions, maybe we should have a separate KEP that will be discussing just that and will outline the decision we'll make within our SIG.
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| In contrast to `kubectl apply`, which was developed directly in kubernetes/kubernetes and had minimal usage or | ||
| real world experience prior, Kustomize was built as a outside of kubernetes/kubernetes in the |
There was a problem hiding this comment.
s/as a outside/as an outside/
| have been discovered earlier. Implementing Kustomize independently allowed more time for gathering feedback and | ||
| identifying issues. | ||
|
|
||
| Kustomize library code will be moved from its current repository (location) to the cli-runtime repository used by |
There was a problem hiding this comment.
Wait, isn't that dependent on the actual decision. The KEP oulines 3 possible options, but doesn't pick a winner, but it should so that we can clearly state further actions like this one.
| identifying issues. | ||
|
|
||
| Kustomize library code will be moved from its current repository (location) to the cli-runtime repository used by | ||
| kubectl. This will be started after cli-runtime has been fully moved out of staging. |
There was a problem hiding this comment.
Drop that last statement, not relevant.
| - update the examples in kubectl commands | ||
| - Improve help messages or documentations to list kubectl subcommands that can work with kustomization directories | ||
| - update the examples in kubectl commands | ||
| - Improve help messages or documentations to list kubectl subcommands that can work with kustomization directories | ||
|
|
||
| ## Alternatives |
There was a problem hiding this comment.
This is now mixed up, alternatives should be proposed next to actual proposals, or we move those proposals that were not picked over here. Now, we have 2 places where alternatives are being discussed, which makes this document hard to follow.
|
@soltysh I think we have consensus on the semantics around a subcommand which we want regardless. I think we need to consider the semantics for implicit cli-runtime integration. I believe we have consensus that this is something we want, but the precise UX and implementation are still up for debate. I will put forth that we re-scope this PR to the command only, and follow up with a separate PR to add implicit cli-runtime integration for commands taking |
Go for it. |
k8s-ci-robot
added
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pwittrock If they are not already assigned, you can assign the PR to them by writing 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 |
k8s-ci-robot
removed
the
approved
|
PTAL |
There was a problem hiding this comment.
I suggest changing the implementation History to
- [ ] vendor `kustomize/pkg` into kubernetes
- [ ] copy `kustomize/k8sdeps` into cli-runtime
- [ ] Implement a function in cli-runtime to run kustomize build with input as fSys and/or path.
- execute kustomize build to get a list of resources
- write the output to io.Writer
- [ ] Add a subcommand `kustomize` in kubectl. This command accepts one argument <dir> and write the output to stdout
kubectl kustomize <dir>
- [ ] documentation:
- Write full doc for `kubectl kustomize`
- Update the examples in kubectl apply/delete to include the usage of kustomize
| @@ -0,0 +1,102 @@ | |||
| --- | |||
| kep-number: 31 | |||
| @@ -199,35 +338,50 @@ Most implementation will be in cli-runtime | |||
| - [x] vendor `kustomize/pkg` into kubernetes | |||
| - [x] copy `kustomize/k8sdeps` into cli-runtime | |||
| - [x] Implement a Visitor for kustomization directory which | |||
There was a problem hiding this comment.
We don't need to implement a visitor to kustomization directory since the subcommand kustomize we're adding doesn't rely on builder or visitor interface.
|
Er, I have a separate KEP but same PR. LMK how you prefer it. The separate KEP is provisional, so nothing is set in stone with that one. |
|
@soltysh This isn't getting updates from the pwittrock/enhancements. Closing and trying again. |
|
Reopened as #698 |
