Support for set / substitution #1962

pwittrock · 2019-12-13T19:30:53Z

No description provided.

k8s-ci-robot · 2019-12-13T19:32:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [pwittrock]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tkellen · 2019-12-13T19:53:23Z

cmd/config/docs/commands/subset.md

+
+    # set a substitution for spec.ports[name=http].port on Service example using
+    # the existing value as the marker
+    kustmoize config sub dir/ port --marker "8080" --type "int" \


tkellen · 2019-12-13T19:57:20Z

This, to me, seems to be walking back the significant strides kustomize made in not trying to do templating in manifests. I believe the approach of specifying replacements in a kustomization.yml is sufficient to solve all use-cases as long as there is some method of dynamically generating (aka, templating) only the kustomization.yml itself.

tkellen · 2019-12-13T20:11:55Z

This comment shows an approach that could provide this sort of functionality without putting templating constructs in manifests:
#318 (comment)

Is there some venue to engage with the maintainers of this project in a video/audio call? Failing that, some location I can fly to to have a discussion with ya'll? It's very perplexing to see a project with such brilliant ideas flailing a few feet away from the literal finish line.

Indirect modification of structured data through json patches is a revelation. We need some reasonable method to template the creation of those patches and we're done.

tkellen · 2019-12-13T20:21:16Z

Here is another approach attempting the same while only using yaml (effectively asking consumers to write an AST to gain template-like constructs): #1631 (comment)

pwittrock · 2019-12-13T20:28:01Z

Thanks for reaching out. Best place to engage would be one of the bi-weekly sig-cli meetings:

https://github.com/kubernetes/community/tree/master/sig-cli#meetings

I'll take a look at your link. Some context for how we seeing this used may be helpful.

As noted in the docs, this is an [Alpha] command which is disabled by default, and it is expect that we will iterate on the UX.

Note that this is invented as an exploration for the next generation imperative set commands kubectl set.

tkellen · 2019-12-13T21:51:34Z

Thanks--I'll endeavor to be at the next meeting!

I think I'm tracking what you mean about supporting an imperative and declarative set of commands like kubectl does. Viewed through that lens it is a lot easier to understand the motivation of this approach.

Still, I think bringing the duality of those concepts enjoyed in kubectl to kustomize proper would be more fruitful if they were implemented by generating a "kustomization" in memory and using it to apply the patches it contains directly to the resources being targeted. This would ensure the same underlying declarative constructs are present for all methods of control.

pwittrock · 2019-12-15T22:33:55Z

@tkellen I agree we want a alignment between what is possible imperative and declaratively -- hopefully the design in this PR lends itself to this.

The core kustomize transforms are not well suited to this at this time because they are built on the apimachinery libraries rather than the kustomize/kyaml libraries. This means they drop things like comments, structure, origin file, etc. Building new infrastructure on this core means it will suffer from the same issues. Instead I am building new infrastructure on the kyaml libraries.

I'd like to first rebase kustomize on kyaml, breaking all dependencies on kubernetes/apimachinery and friends. I think after that, the relationship between imperative commands such as this one, and declarative machinery such as kustomize build will be a lot more clear and elegant.

Note: rather than implementing every transform as a kustomization field, and making other tools generate a kustomization, I would prefer that each transform exists as its own composable unit, which both kustomizations and imperative tools could build upon.

justinsb · 2019-12-16T16:12:36Z

cmd/config/docs/commands/subset.md

+Substitution markers may be:
+
+- valid field values (e.g. `8080` for a port)
+  - Note: `008080` would be preferred because it is more recognizable as a marker


Perhaps in a follow on PR, can you expand on this? The idea that 8080 != 008080 feels like magic, so I'm guessing this is because you're operating at a different level than the fully-parsed and normalized yaml.

justinsb · 2019-12-16T17:13:21Z

cmd/config/docs/commands/subset.md

+- **be pre-filled in with a value** -- e.g. set the value when setting the marker
+
+
+    port: 8080 # {"substitutions":[{"name":"port","marker":"[MARKER]","value":"8080""}],"type":"int"}


I'm confused why I need a marker at all in this case (?)

justinsb · 2019-12-16T17:15:33Z

cmd/config/docs/commands/subset.md

+
+    $ kustomize config set create dir/ app-image-tag v1.0.01 --type "string" --field "image"
+
+    image: gcr.io/example/app:v1.0.1 # {"substitutions":[{"name":"app-image-tag","marker":"[MARKER]","value":"v1.0.1"}]}


In the edge case where I specified app-image-tag == app, would you record a disambiguator?

justinsb · 2019-12-16T17:17:01Z

cmd/config/internal/commands/cmdsub.go

+	c.Flags().BoolVar(&r.Perform.Override, "override", true,
+		"override previously substituted values.")
+	c.Flags().BoolVar(&r.Perform.Revert, "revert", false,
+		"override previously substituted values.")


Description looks copy-pasted. Should be "revert..." ?

justinsb · 2019-12-16T17:18:39Z

cmd/config/internal/commands/cmdsub.go

+	if r.Perform.Override {
+		mutex++
+	}
+	if mutex > 1 {


Maybe just do r.Perform.Revert && r.Perform.Override ... the use of the word "mutex" is confusing.

justinsb · 2019-12-16T17:26:43Z

cmd/config/internal/sub/addkio.go

+		if s.ResourceMeta.Name != "" && m.Name != s.ResourceMeta.Name {
+			continue
+		}
+		if s.ResourceMeta.Kind != "" && m.Kind != s.ResourceMeta.Kind {


It's surprising not to see namespace here, but I guess we can add it when we need it!

justinsb · 2019-12-16T17:27:17Z

cmd/config/internal/sub/addyaml.go

+// substituteResource substitutes a Marker value on a field
+type Marker struct {
+	// Path is the path of the field to add the substitution for
+	Field string


Nit: comment doesn't match field name

justinsb · 2019-12-16T17:27:23Z

cmd/config/internal/sub/addyaml.go

+
+var _ yaml.Filter = &Marker{}
+
+// substituteResource substitutes a Marker value on a field


Nit: comment doesn't match type name

justinsb · 2019-12-16T21:37:31Z

cmd/config/internal/sub/dokio.go

+
+func (s *PerformSubstitutions) Filter(input []*yaml.RNode) ([]*yaml.RNode, error) {
+	for i := range input {
+		p := &performSubstitutions{


The similar names might get confusing. Maybe performOneSubstitution or substitutionOp? Or just fold the types into one?

justinsb · 2019-12-16T21:39:26Z

Some nits but nothing that can't be address in follow-on PRs if you agree

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 13, 2019

pwittrock changed the title ~~support for inline substitutions~~ Support for set / substitution Dec 13, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2019

k8s-ci-robot requested review from Liujingfang1 and mengqiy December 13, 2019 19:32

tkellen reviewed Dec 13, 2019

View reviewed changes

pwittrock force-pushed the sub branch from e2018e3 to f591b15 Compare December 15, 2019 22:27

pwittrock force-pushed the sub branch 2 times, most recently from 4650aa2 to d970f53 Compare December 15, 2019 22:58

cmd/config set: Support for setting fields imperatively from the cli

62e969c

pwittrock force-pushed the sub branch from d970f53 to 62e969c Compare December 16, 2019 18:40

justinsb reviewed Dec 16, 2019

View reviewed changes

k8s-ci-robot assigned justinsb Dec 16, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2019

k8s-ci-robot merged commit b38c0bc into kubernetes-sigs:master Dec 16, 2019

tkellen mentioned this pull request Jan 10, 2020

kustomize vars - enhance or replace? #2052

Open

pwittrock deleted the sub branch March 20, 2020 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for set / substitution #1962

Support for set / substitution #1962

pwittrock commented Dec 13, 2019

k8s-ci-robot commented Dec 13, 2019

tkellen Dec 13, 2019

pwittrock Dec 15, 2019

tkellen commented Dec 13, 2019 •

edited

Loading

tkellen commented Dec 13, 2019

tkellen commented Dec 13, 2019

pwittrock commented Dec 13, 2019 •

edited

Loading

tkellen commented Dec 13, 2019

pwittrock commented Dec 15, 2019 •

edited

Loading

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb Dec 16, 2019

justinsb commented Dec 16, 2019

		- be pre-filled in with a value -- e.g. set the value when setting the marker


		port: 8080 # {"substitutions":[{"name":"port","marker":"[MARKER]","value":"8080""}],"type":"int"}


		$ kustomize config set create dir/ app-image-tag v1.0.01 --type "string" --field "image"

		image: gcr.io/example/app:v1.0.1 # {"substitutions":[{"name":"app-image-tag","marker":"[MARKER]","value":"v1.0.1"}]}


		var _ yaml.Filter = &Marker{}

		// substituteResource substitutes a Marker value on a field

Support for set / substitution #1962

Support for set / substitution #1962

Conversation

pwittrock commented Dec 13, 2019

k8s-ci-robot commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkellen commented Dec 13, 2019 • edited Loading

tkellen commented Dec 13, 2019

tkellen commented Dec 13, 2019

pwittrock commented Dec 13, 2019 • edited Loading

tkellen commented Dec 13, 2019

pwittrock commented Dec 15, 2019 • edited Loading

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

Choose a reason for hiding this comment

justinsb commented Dec 16, 2019

tkellen commented Dec 13, 2019 •

edited

Loading

pwittrock commented Dec 13, 2019 •

edited

Loading

pwittrock commented Dec 15, 2019 •

edited

Loading