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

Update Kustomization integration KEP #698

Merged
merged 6 commits into from Jan 24, 2019

Conversation

@pwittrock
Copy link
Member

pwittrock commented Jan 18, 2019

  • Split into 2 parts
    • Add subcommand
    • New kep for integration into file handling

pwittrock added some commits Jan 10, 2019

Update Kustomization integration KEP
- add more background and context.
- revisit specifics of UX.
- provide justification for not using plugins.
- add more alternatives considered.
@pwittrock

This comment has been minimized.

Copy link
Member Author

pwittrock commented Jan 18, 2019

/assign @soltysh
/assign @liggitt
/assign @monopole
/assign @Liujingfang1

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 18, 2019

@pwittrock: GitHub didn't allow me to assign the following users: Liujingfang1.

Note that only kubernetes members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @soltysh
/assign @liggitt
/assign @monopole
/assign @Liujingfang1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.


## Summary

This is a follow up to [KEP-0031](https://github.com/kubernetes/enhancements/blob/master/keps/sig-cli/0031-kustomize.md)

This comment has been minimized.

@bgrant0607

bgrant0607 Jan 18, 2019

Member

This PR is removing that file.

This comment has been minimized.

@pwittrock

pwittrock Jan 22, 2019

Author Member

Updated

@justaugustus
Copy link
Member

justaugustus left a comment

Please remove any references to NEXT_KEP_NUMBER and rename the KEP to just be the draft date and KEP title.
KEP numbers will be obsolete once #703 merges.

@pwittrock pwittrock force-pushed the pwittrock:master branch 3 times, most recently from 23d3fe4 to f2d6e2a Jan 22, 2019

@pwittrock pwittrock force-pushed the pwittrock:master branch from f2d6e2a to e5d6599 Jan 22, 2019

@pwittrock

This comment has been minimized.

Copy link
Member Author

pwittrock commented Jan 22, 2019

@justaugustus That is great news. Updated.

@soltysh
Copy link
Contributor

soltysh left a comment

I left you a few nits here and there, but overall this lgtm.
Thank you 🥇

Kubectl commands taking the common flags (`-f`, `--filename`, `-R`, `--recursive`) will support `kustomization.yaml`
files.

Cli-runtime will add the flags `-k, --kustomize=[]`, which will be registered along side the other file processing

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

Quick question, I assume -k and -f will be exclusive? If so, maybe it's worth adding that here.

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Hm. Good question. We could start that way and open it up in the future.

This comment has been minimized.

@soltysh

soltysh Jan 24, 2019

Contributor

Yeah, exclusive seems the best to start with, it'll leave us more room for change than the other way around.


Link to tracking issue: kubernetes/enhancements#633

See [KEP FAQ](kep-faq.md) for "why not as as plugin".

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

and Why should this be part of kubectl? section too.

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done


Kustomize Standalone Sub Command

Publish the `kustomize build` command as `kubectl kustomize`. Update documentation demonstrate using kustomize as

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

to demonstrate

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

Publish the `kustomize build` command as `kubectl kustomize`. Update documentation demonstrate using kustomize as
`kubectl kustomize <dir> | kubectl apply -f -`.

`kubectl kustomize` takes a single argument with is the location of a directory containing a file named `Kustomization`

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

file named kustomization.yaml

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

@monopole WDYT of changing the file name pre-integration (we could still recognize the old one, but not advertise it)? This follows the pattern of Makefile and Dockerfile.

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

FWIW, We went with .yaml (and, alas, .yml) because emacs and vi both still use the legacy extension as a pretty reliable edit model signal. I just tried it without the extension, and my stock emacs and vim installs failed to do the Right Thing based on content detection.

Makefile, being a unique text formatting class with one instantiation, doesn't suffer from this problem (emacs/vi do the Right Thing). Dockerfile detection hasn't made it into my stock emacs install yet :)

I'm for allowing all of them ( Kustomization, kustomization.yaml, kustomization.yml) pre-integration, erroring if none or more than one found (no ambiguity).

This comment has been minimized.

@ahmetb

ahmetb Jan 23, 2019

Member

Have we actually considered abandoning the brand name Kustomization and actually call it customization.yaml?

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

Yes we have.

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

keeping this kustomization.yaml

This comment has been minimized.

@soltysh

soltysh Jan 24, 2019

Contributor

Honestly, since we're introducing -k flag which is meant to point to kustomization.yaml I think we can safely assume that any name is supported, right? Or did I miss something? Eventually we should make this more clearer in description of the flag, if so. But I remember from the discussion we've had during KubeCon we wanted to explicitly point to the file to run kustomization, unless the fact that we're using -k/--kustomize allows users to pass file OR the directory containing the file. In which case there are two possibilities:

  1. if I pass file name to -k the name doesn't matter, only the kind specified in file must be Kustomization.
  2. if I pass directory we can search for kustomization.yaml or just look for a file with Kustomization kind.
    So actually the name doesn't matter. Other thoughts?

This comment has been minimized.

@liggitt

liggitt Jan 24, 2019

Member

I'd expect -k to take a directory, and look for the kustomization file the same way we look for the file when processing a base referenced from another file. For these directories to be composable, they will need to use kustomize filenames findable via base reference, anyway.

This comment has been minimized.

@soltysh

soltysh Jan 25, 2019

Contributor

That's a reasonable argument for -k accepting a dir.

`kubectl kustomize` takes a single argument with is the location of a directory containing a file named `Kustomization`
and writes to stdout the kustomized Resource Config.

If the directory does not contain a `Kustomization` file, it returns an error.

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

kustomize.yaml

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done


## Kustomize Example

Following is an example of a kustomization.yaml file used by kustomize:

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

Put kustomization.yaml into back-quotes.

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

Following is an example of a kustomization.yaml file used by kustomize:

```
apiVersion: v1beta1

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

👍


## Graduation Criteria

The API version for kustomize is defined in the kustomization.yaml file. The KEP is targeted `v1beta1`.

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

Back-quote kustomization.yaml

The API version for kustomize is defined in the kustomization.yaml file. The KEP is targeted `v1beta1`.

The criteria for graduating from `v1beta1` for the kustomize sub-command should be determined as part of
evaluating the success and maturity of kustomize as a command within kubectl.

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

👍

Most implementation will be in cli-runtime

- [ ] vendor `kustomize/pkg` into kubernetes
- [ ] copy `kustomize/k8sdeps` into cli-runtime

This comment has been minimized.

@soltysh

soltysh Jan 23, 2019

Contributor

Yeah, at the end of the day I'm hoping that cli-runtime will contain the core of kustomize, but everything above (commands, etc) should exist outside cli-runtime. Maybe worth putting that here.

## Summary

[Kustomize](https://github.com/kubernetes-sigs/kustomize)
was developed as a subproject of sig-cli by kubectl maintainers with the goal of addressing

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

s/with the goal of addressing/to address/

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done


[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 (See [motivation](#motivation)) creating friction for declarative workflows in kubectl

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

issues can link to #motivation - can drop the (See ...)

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

It is independent of, but complementary to, the [*server-side apply*](https://github.com/kubernetes/enhancements/issues/555)
initiative that was started later and targeted at a separate collection of `kubectl apply` issues.

**Note:**

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

This paragraph (the "Note") and the next offer more details on the purpose / history of kustomize,

Trying to rephrase without the "Note":

Kustomize offers generators and transformations in a declarative form that improve
on functionality provided by existing imperative commands in kubectl.

The declarative approach offers a clear path to accountability (all input can be kept
in version control), can safely exploit a holistic, unbounded view of disparate resources
and their interdependence (it's a plan about what to do, not a direct action), and
can be easily constrained to verifiable rules across this view (all edits must be
structured, no removal semantics, no environment side-effects, etc.).

Imperative kubectl commands / flags available through kustomize:
....

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

- updating Resources from Resource Config without wiping out control-plane defined fields (e.g. Service clusterIp)
- automatically deciding whether to create, update or delete Resources

**Motivation:**

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

heading repeated

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

@@ -0,0 +1,101 @@
---
title: Integrate Kustomize into cli-runtime

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

minor point: This KEP has three titles - this one (in the table), the file name, and the text after the first #. Let's pick one.

more importantly - We've discussed -f vs -k / visitation at length, and my views/concerns are missing here or perhaps just not expressed as I'd express them.

We want declarative features delivered as part of kubectl, and have established that the quickest way to do so is via adding a new subcommand (the other KEP), and deferring debate about -f and its baggage.

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

If the directory does not contain a `Kustomization` file, it returns an error.

Defer deeper integration into ResourceBuilder (e.g. `kubectl apply -k <dir>`) as a follow up after discussing
the precise UX and technical tradeoffs. (i.e. deeper integration is more delicate and can be done independently.)

This comment has been minimized.

@monopole

monopole Jan 23, 2019

Contributor

This is enough about -f / -k for this PR

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Ack

literals:
- JAVA_HOME=/opt/java/jdk
- JAVA_TOOL_OPTIONS=-agentlib:hprof

This comment has been minimized.

@dsansot-ru

dsansot-ru Jan 23, 2019

The indentation is off for literals.

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

- name: app-sec
commands:
username: "echo admin"
password: "echo secret"

This comment has been minimized.

@dsansot-ru

dsansot-ru Jan 23, 2019

The indentation is off for commands.

This comment has been minimized.

@pwittrock

pwittrock Jan 23, 2019

Author Member

Done

@monopole

This comment has been minimized.

Copy link
Contributor

monopole commented Jan 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 24, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 24, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: monopole, pwittrock

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

@k8s-ci-robot k8s-ci-robot merged commit 595feb5 into kubernetes:master Jan 24, 2019

2 checks passed

cla/linuxfoundation pwittrock authorized
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.