🌱 Add generate yaml subcommand to clusterctl #3406

wfernandes · 2020-07-27T21:35:52Z

Currently, this only works with --from flag since that is the most generic and can be used for any template.
It also provides the --list-variables flag to list variables present in the template

What this PR does / why we need it:
This PR adds the generate yaml subcommand. It works as follows:

# Print out the processed yaml with variables substituted.
clusterctl generate yaml --from <file-path>
# Prints out the list of variables available in the template.
clusterctl generate yaml --from <file-path> --list-variables

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3379

wfernandes · 2020-07-27T22:14:19Z

cmd/clusterctl/client/config.go

@@ -58,6 +58,37 @@ func (c *clusterctlClient) GetProviderComponents(provider string, providerType c
 	return components, nil
 }

+type ProcessYAMLOptions struct {


In the future we can specify the following option if it is needed but for now this sub-command uses the SimpleProcessor default.

// YamlProcessor defines the yaml processor to use for the cluster¬ // template processing. If not defined, SimpleProcessor will be used.¬ YamlProcessor Processor¬

wfernandes · 2020-07-27T22:20:34Z

cmd/clusterctl/client/config.go

+	// Technically we do not need to connect to the cluster. However, we are
+	// leveraging the template client which exposes GetFromURL() is available
+	// on the cluster client so we create a cluster client with default
+	// configs to access it.
+	cluster, err := c.clusterClientFactory(
+		ClusterClientFactoryInput{
+			// use the default kubeconfig
+			kubeconfig: Kubeconfig{},
+		},
+	)


Since GetFromURL has nothing to do with a cluster, "ideally" we would refactor it out into a standalone template client but that is quite a decent amount of refactoring. I opted to do it this way to reuse as much of the existing logic and not blow this PR out of proportions.

vincepri · 2020-07-27T22:21:34Z

/assign
/milestone v0.3.8

cmd/clusterctl/cmd/generate_yaml.go

wfernandes · 2020-07-28T15:26:54Z

/test pull-cluster-api-verify

cmd/clusterctl/client/client.go

cmd/clusterctl/client/config.go

cmd/clusterctl/cmd/generate_yaml.go

wfernandes · 2020-07-28T20:04:38Z

/hold
Putting this on hold so that we can support reading files from stdin.

wfernandes · 2020-07-29T19:41:31Z

/hold cancel
@vincepri I added support for reading in files from stdin. Take a look when you can. Thanks!

cmd/clusterctl/client/config.go

docs/book/src/clusterctl/commands/generate-yaml.md

vincepri · 2020-07-30T15:38:29Z

/assign @CecileRobertMichon

for another review

vincepri · 2020-07-30T15:38:44Z

Code changes and functionaliy lgtm so far 👍

CecileRobertMichon · 2020-07-30T16:37:01Z

docs/book/src/clusterctl/overview.md

 mis-configurations or in managing day 2 operations such as upgrades.

 * use [`clusterctl init`](commands/init.md) to install Cluster API providers
 * use [`clusterctl upgrade`](commands/upgrade.md) to upgrade Cluster API providers
 * use [`clusterctl delete`](commands/delete.md) to delete Cluster API providers

 * use [`clusterctl config cluster`](commands/config-cluster.md) to spec out workload clusters
+* use [`clusterctl generate yaml`](commands/generate-yaml.md) to process yaml


Is the idea to consolidate generate and config later on? I imagine there is some overlap between the two (eg. --list-variables). It might be confusing to a user when to use one vs. the other

We have this issue documenting some of the refactors we'd like to have for the config sub-command. #3360

Got it. Also in the issue for user story you wrote "As a developer I would like to have a sub-command in clusterctl that could process yaml using the built in yaml processor so that we can leverage clusterctl's internal yaml processor within scripts." I agree that the main audience for this command would be a developer, do you also foresee a user story for cluster api end users? If not, would it be confusing to users to include this one in the user documentation? Maybe we can add something like "for development purposes" in the description, wdyt?

These could also be used in CI scripts and other custom pipelines

@CecileRobertMichon Yes. That makes sense actually. I guess for this specific sub-command the main audience would be for developers or maintainers of cluster-api providers (from the use cases we've seen so far).

Unless there are users who have customized templates and want to use clusterctl's default variable processor as well.
I can try and add some more clarity towards this in the book documentation.

wfernandes · 2020-07-30T16:37:16Z

/test pull-cluster-api-e2e

CecileRobertMichon · 2020-07-30T16:46:46Z

/cc @jackfrancis

Jack has been experimenting with clusterctl recently and might have some thoughts on this as a new-ish user

k8s-ci-robot · 2020-07-30T16:46:47Z

@CecileRobertMichon: GitHub didn't allow me to request PR reviews from the following users: jackfrancis.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jackfrancis

Jack has been experimenting with clusterctl recently and might have some thoughts on this as a new-ish user

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.

jackfrancis · 2020-07-30T16:51:20Z

@wfernandes I like the idea of having a "generic" yaml input interface (as opposed to just --infrastructure <provider> --kubernetes-version <version> --control-plane-machine-count <num> --worker-machine-count <num> etc for other options) but am concerned that the spec for creating that input yaml + template layer (i.e., the view-layer conveniences that allow for variable substitution, computed properties, etc) is not well-defined. Is there a well-defined API for this template/view layer, or do we expect users to be familiar with the underlying clusterctl yaml generator code?

wfernandes · 2020-07-30T17:18:47Z

@jackfrancis This specific sub-command is meant to alleviate the need to know the internal implementation (i.e. drone/envsubst) of the default yaml processor.

clusterctl does a bunch of stuff for us internally as part of the clusterctl config cluster command which will be refactored to clusterctl generate cluster to make it more intuitive but this is a separate issue (#3360).
The flags you mentioned above were added probably for user convenience to override those values. These flags map to variables within the template e.g. --kubernetes-version --> KUBERNETES_VERSION.

Is there a well-defined API for this template/view layer, or do we expect users to be familiar with the underlying clusterctl yaml generator code?

Regarding the spec to actually authoring the yaml, unfortunately I wasn't part of the original clusterctl redesign however I believe the idea was iterative. That is, we started out with simple variable substitution like ${VAR}. Since the need for having defaults became apparent we decided on the least intrusive change by using envsubst style variables ${VAR:=default}.
So I guess for the simple processor, the spec would be envsubst which I believe is known by many users already and for which documentation is readily available.

Now, if for some reason we needed to deviate and introduce some special functionality into the templating, we can then either define a proposal for the spec that clusterctl should follow by default (as you suggested) OR it can be supported by injecting another yaml processor. Currently, injecting yaml processor is supported by the library. BUT if we do implement some kind of plugin system, we "could potentially" leverage that.

Hope that clarifies some things.

docs/book/src/clusterctl/commands/generate-yaml.md

vincepri

/approve

@wfernandes Squash commits? Over to @CecileRobertMichon for final lgtm 🎉

k8s-ci-robot · 2020-07-30T21:49:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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

CecileRobertMichon · 2020-07-30T22:43:25Z

lgtm pending rebase

Currently, this only works with --from flag since that is the most generic and can be used for any template. It also provides the --list-variables flag to list variables present in the template By default, it supports reading in from stdin.

k8s-ci-robot · 2020-07-30T23:18:48Z

@wfernandes: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	`78c886c`	link	`/test pull-cluster-api-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

CecileRobertMichon

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 27, 2020

k8s-ci-robot requested review from JoelSpeed and vincepri July 27, 2020 21:36

wfernandes changed the title ~~WIP 🌱 Add generate yaml subcommand to clusterctl~~ 🌱 Add generate yaml subcommand to clusterctl Jul 27, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2020

wfernandes force-pushed the clusterctl-generate-yaml branch from 41d9fba to 08255b5 Compare July 27, 2020 21:45

wfernandes commented Jul 27, 2020

View reviewed changes

wfernandes force-pushed the clusterctl-generate-yaml branch from 08255b5 to f881a45 Compare July 27, 2020 22:18

wfernandes commented Jul 27, 2020

View reviewed changes

k8s-ci-robot assigned vincepri Jul 27, 2020

k8s-ci-robot added this to the v0.3.8 milestone Jul 27, 2020

wfernandes force-pushed the clusterctl-generate-yaml branch from f881a45 to 057cc82 Compare July 27, 2020 22:23

wfernandes commented Jul 28, 2020

View reviewed changes

cmd/clusterctl/cmd/generate_yaml.go Show resolved Hide resolved

vincepri reviewed Jul 28, 2020

View reviewed changes

wfernandes force-pushed the clusterctl-generate-yaml branch from 057cc82 to 12a1960 Compare July 28, 2020 19:41

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2020

wfernandes force-pushed the clusterctl-generate-yaml branch from d313d6f to 5cd0ba6 Compare July 29, 2020 19:35

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2020

wfernandes force-pushed the clusterctl-generate-yaml branch from 5cd0ba6 to dc76005 Compare July 29, 2020 19:49

wfernandes commented Jul 29, 2020

View reviewed changes

cmd/clusterctl/client/config.go Show resolved Hide resolved

wfernandes force-pushed the clusterctl-generate-yaml branch from dc76005 to d7368fa Compare July 30, 2020 14:44

vincepri reviewed Jul 30, 2020

View reviewed changes

cmd/clusterctl/client/config.go Outdated Show resolved Hide resolved

vincepri reviewed Jul 30, 2020

View reviewed changes

docs/book/src/clusterctl/commands/generate-yaml.md Show resolved Hide resolved

wfernandes force-pushed the clusterctl-generate-yaml branch from d7368fa to bac0fc4 Compare July 30, 2020 15:14

k8s-ci-robot assigned CecileRobertMichon Jul 30, 2020

CecileRobertMichon reviewed Jul 30, 2020

View reviewed changes

wfernandes force-pushed the clusterctl-generate-yaml branch from bac0fc4 to d0e003b Compare July 30, 2020 19:48

wfernandes commented Jul 30, 2020

View reviewed changes

docs/book/src/clusterctl/commands/generate-yaml.md Show resolved Hide resolved

vincepri approved these changes Jul 30, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2020

Add generate yaml subcommand to clusterctl

78c886c

Currently, this only works with --from flag since that is the most generic and can be used for any template. It also provides the --list-variables flag to list variables present in the template By default, it supports reading in from stdin.

wfernandes force-pushed the clusterctl-generate-yaml branch from d0e003b to 78c886c Compare July 30, 2020 23:13

CecileRobertMichon approved these changes Jul 31, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2020

k8s-ci-robot merged commit 4de2d9b into kubernetes-sigs:master Jul 31, 2020

wfernandes deleted the clusterctl-generate-yaml branch July 31, 2020 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add generate yaml subcommand to clusterctl #3406

🌱 Add generate yaml subcommand to clusterctl #3406

wfernandes commented Jul 27, 2020

wfernandes Jul 27, 2020

wfernandes Jul 27, 2020

vincepri commented Jul 27, 2020

wfernandes commented Jul 28, 2020

wfernandes commented Jul 28, 2020

wfernandes commented Jul 29, 2020

vincepri commented Jul 30, 2020

vincepri commented Jul 30, 2020

CecileRobertMichon Jul 30, 2020

wfernandes Jul 30, 2020

CecileRobertMichon Jul 30, 2020

vincepri Jul 30, 2020

wfernandes Jul 30, 2020

wfernandes commented Jul 30, 2020

CecileRobertMichon commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020

jackfrancis commented Jul 30, 2020

wfernandes commented Jul 30, 2020

vincepri left a comment

k8s-ci-robot commented Jul 30, 2020

CecileRobertMichon commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020

CecileRobertMichon left a comment

🌱 Add generate yaml subcommand to clusterctl #3406

🌱 Add generate yaml subcommand to clusterctl #3406

Conversation

wfernandes commented Jul 27, 2020

wfernandes Jul 27, 2020

Choose a reason for hiding this comment

wfernandes Jul 27, 2020

Choose a reason for hiding this comment

vincepri commented Jul 27, 2020

wfernandes commented Jul 28, 2020

wfernandes commented Jul 28, 2020

wfernandes commented Jul 29, 2020

vincepri commented Jul 30, 2020

vincepri commented Jul 30, 2020

CecileRobertMichon Jul 30, 2020

Choose a reason for hiding this comment

wfernandes Jul 30, 2020

Choose a reason for hiding this comment

CecileRobertMichon Jul 30, 2020

Choose a reason for hiding this comment

vincepri Jul 30, 2020

Choose a reason for hiding this comment

wfernandes Jul 30, 2020

Choose a reason for hiding this comment

wfernandes commented Jul 30, 2020

CecileRobertMichon commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020

jackfrancis commented Jul 30, 2020

wfernandes commented Jul 30, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 30, 2020

CecileRobertMichon commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment