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

Allow the usage of --kubeconfig-dir and --config flags on kubeadm init #73998

Merged
merged 1 commit into from Feb 13, 2019

Conversation

@yagonobre
Copy link
Member

yagonobre commented Feb 13, 2019

What type of PR is this?
/kind bug
What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1403

Special notes for your reviewer:
I also made a little refactor to ValidateMixedArguments, is it ok?
And renamed from whitelist to allowed

Does this PR introduce a user-facing change?:

kubeadm: allow the usage of --kubeconfig-dir and --config flags on kubeadm init

/priority important-soon
/assign @neolit123

@yagonobre yagonobre force-pushed the yagonobre:fix-mixed-args branch from 83a03d2 to 277dfbb Feb 13, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 13, 2019

@yagonobre yagonobre changed the title Add --kubeconfig-dir to validate mixed arguments whitelist Allow the usage of --kubeconfig-dir and --config flags on kubeadm init Feb 13, 2019

@yagonobre

This comment has been minimized.

Copy link
Member Author

yagonobre commented Feb 13, 2019

/retest

@neolit123
Copy link
Member

neolit123 left a comment

this looks good, thanks @yagonobre

/retest
/lgtm
/approve
/hold
@kubernetes/sig-cluster-lifecycle-pr-reviews

comments from others?

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 13, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

fabriziopandini commented Feb 13, 2019

@yagonobre thanks for this PR
I'm ok with this change, considering that as commented on the issues, the --kubeconfig-dir remain an additional facility that apply to the kubeconfig phase only.

/approve
/lgtm

Other changes suggested on the issue (e.g. to promote --kubeconfig-dir as a option to be supported by the kubeadm config) IMO require approval from the broader sig before execution.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, neolit123, yagonobre

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

@yagonobre yagonobre referenced this pull request Feb 13, 2019

Merged

Add kubeadm init upload encrypted certs phase #73907

3 of 4 tasks complete
@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 13, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 2bfbbc3 into kubernetes:master Feb 13, 2019

17 checks passed

cla/linuxfoundation yagonobre 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-godeps 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
pull-publishing-bot-validate Skipped
tide In merge pool.
Details
func isAllowedFlag(flagName string) bool {
isAllowed := false
switch flagName {
case kubeadmcmdoptions.CfgPath,

This comment has been minimized.

@tedyu

tedyu Feb 13, 2019

Contributor

How about putting these into a Set and check whether flagName is contained in the Set ?

This comment has been minimized.

@neolit123

neolit123 Feb 13, 2019

Member

our flags are kind of sparse and cover different functionality i.e. the flags don't make sense to be a part of the same set outside of this validatation, but i guess we can consider it for a future refactor.

This comment has been minimized.

@tedyu

tedyu Feb 13, 2019

Contributor

I have a patch ready, should I send out PR ?

This comment has been minimized.

@neolit123

neolit123 Feb 13, 2019

Member

given the flags are mostly unrelated, my initial reaction is, no.
but you can still send if you want to get feedback on the change.

This comment has been minimized.

@tedyu

tedyu Feb 13, 2019

Contributor

See #74032

@yagonobre yagonobre deleted the yagonobre:fix-mixed-args branch Feb 13, 2019

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.