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

kubeadm: Introduce config print init/join-defaults #69617

Merged
merged 1 commit into from Oct 20, 2018

Conversation

@rosti
Contributor

rosti commented Oct 10, 2018

What this PR does / why we need it:

In order to improve the UX of kubeadm, it was decided to introduce the
following subcommands:

  • kubeadm config print - this is currently only a placeholder for subcommands
    that deal printing of some kind of configuration.
  • kubeadm config print init-defaults - prints the default combination of
    InitConfiguration and ClusterConfiguration. Selected component configs can be
    printed too if the --component-configs command line switch is used.
  • kubeadm config print join-defaults - prints the default JoinConfiguration.
    This command also supports the use of --component-configs.
  • kubeadm config print-defaults is deprecated in favor of
    kubeadm config print init/join-defaults.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Refs kubernetes/kubeadm#1152

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/kind bug
/assign @fabriziopandini
/assign @timothysc
/cc @neolit123

Release note:

kubeadm: Introduce config print init/join-defaults that deprecate config print-defaults by decoupling init and join configs.
@neolit123

thanks @rosti
added a couple of comments.

cmd := &cobra.Command{
Use: "print",
Short: "Print configuration",
Long: "This command serves as a wrapper for subcommands that are meant to print configuration or part of it.",

This comment has been minimized.

@neolit123

neolit123 Oct 10, 2018

Member

optionally:
meant to print configuration or part of it.
->
meant to print the default configuration or part of it.

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

"This command prints default configurations for subcommands provided"

This comment has been minimized.

@rosti

rosti Oct 11, 2018

Contributor

Updated with Tim's version, but omitted the "default" word from it as kubectl config print may also be used to print non default configs in the future (such as current ClusterConfiguration from config map, etc.)

For documentation visit: https://godoc.org/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1
Note that sensitive values like the Bootstrap Token fields are replaced with silly values like %q in order to pass validation but

This comment has been minimized.

@neolit123

neolit123 Oct 10, 2018

Member

we should probably replace silly with placeholder.

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

+1 the language should be professional.

@timothysc

/approve

I understand what you are doing, but the readability of having a single generic function needs to be broken up a bit.

cmd := &cobra.Command{
Use: "print",
Short: "Print configuration",
Long: "This command serves as a wrapper for subcommands that are meant to print configuration or part of it.",

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

"This command prints default configurations for subcommands provided"

Long: fmt.Sprintf(dedent.Dedent(`
This command prints objects such as the default %s configuration that is used for 'kubeadm %s'.
For documentation visit: https://godoc.org/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

Yeah don't hard code this. We should provide a jump point otherwise this is a maintenance burden.

For documentation visit: https://godoc.org/k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta1
Note that sensitive values like the Bootstrap Token fields are replaced with silly values like %q in order to pass validation but

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

+1 the language should be professional.

not perform the real computation for creating a token.
`), action, action, sillyToken),
Run: func(cmd *cobra.Command, args []string) {
allBytes := [][]byte{}

This comment has been minimized.

@timothysc

timothysc Oct 11, 2018

Member

split out the run into it's own function for readability, please.

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 11, 2018

Updated the PR. @neolit123 @timothysc, PTAL

@neolit123

This comment has been minimized.

Member

neolit123 commented Oct 11, 2018

LGTM, thanks.
/retest

@fabriziopandini

@rosti sorry for the late review
I think that this PR could be further simplified by adding the "action" parameter to getSupportedAPIObjects and getAllAPIObjectNames (that we can eventually merge since there is no historical objects to manage now)

Then we can have the deprecated print-defaults, print init-defaults and join-defaults insisting on the same code.
wdyt?

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 11, 2018

@fabriziopandini my first idea was on those lines, but that way, when kubeadm config print-defaults goes away, the code will not get pretty straight.
In the current implementation the code for init/join-defaults is very simple (no YAML splitting, etc.) and when in the next release cycle the code for print-defaults gets deleted things will get really simple. Furthermore, getSupportedAPIObjects and getAllAPIObjectNames are only used by print-defaults, so they will be goners too.

@fabriziopandini

This comment has been minimized.

Contributor

fabriziopandini commented Oct 12, 2018

@rosti thanks for the explanation.
Please open an issue for keeping track of the todo for the next cycle
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 12, 2018

@fabriziopandini

This comment has been minimized.

Contributor

fabriziopandini commented Oct 12, 2018

/approve

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 12, 2018

For the docs approval:

/assign @brendandburns
/assign @pwittrock

@fabriziopandini

This comment has been minimized.

Contributor

fabriziopandini commented Oct 14, 2018

/test pull-kubernetes-e2e-kops-aws

@fabriziopandini

This comment has been minimized.

Contributor

fabriziopandini commented Oct 14, 2018

/approve

@Bradamant3

This comment has been minimized.

Member

Bradamant3 commented Oct 15, 2018

@tengqm do we have formatting issues here still with the generated docs? Or is the fix on our end anyway?

kubeadm: Introduce config print init/join-defaults
In order to improve the UX of kubeadm, it was decided to introduce the
following subcommands:

- `kubeadm config print` - this is currently only a placeholder for subcommands
  that deal printing of some kind of configuration.
- `kubeadm config print init-defaults` - prints the default combination of
  InitConfiguration and ClusterConfiguration. Selected component configs can be
  printed too if the `--component-configs` command line switch is used.
- `kubeadm config print join-defaults` - prints the default JoinConfiguration.
  This command also supports the use of `--component-configs`.
- `kubeadm config print-defaults` is deprecated in favor of
  `kubeadm config print init/join-defaults`.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 18, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 20, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: fabriziopandini, rosti, timothysc

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

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 20, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot k8s-ci-robot merged commit a6e2732 into kubernetes:master Oct 20, 2018

18 checks passed

cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment