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

kubeadm: override node registration options from command line (follow-up) #71323

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Nov 21, 2018

What type of PR is this?

/kind bug

What this PR does / why we need it:

It turned out after the discussion in the similar PR that 'kubeadm init' also suffers from this issue. This fix is almost identical to the previous one:

'kubeadm init' silently ignores --node-name and --cri-socket
command line options if --config option is specified.

Implemented setting 'name' and 'criSocket' options from the command
line even if --config command line option is used.

Does this PR introduce a user-facing change?:

kubeadm init correctly uses --node-name and --cri-socket when --config option is also used

'kubeadm init' silently ignores --node-name and --cri-socket
command line options if --config option is specified.

Implemented setting 'name' and 'criSocket' options from the command
line even if --config command line option is used.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 21, 2018
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 21, 2018
@bart0sh
Copy link
Contributor Author

bart0sh commented Nov 21, 2018

/cc @neolit123
/cc @seh

@k8s-ci-robot
Copy link
Contributor

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

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

In response to this:

/cc @neolit123
/cc @seh

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.

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bart0sh thanks a lot
will double check it locally with git am just in case.

/lgtm
/priority critical-urgent
/kind bug

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 21, 2018
@neolit123
Copy link
Member

/assign @fabriziopandini @timothysc
/retest

@neolit123
Copy link
Member

will double check it locally with git am just in case.

tested locally with our integration test and manually. 👍

@seh
Copy link
Contributor

seh commented Nov 21, 2018

Fantastic! Thank you so much, Ed and Lubomir.

@fabriziopandini
Copy link
Member

@bart0sh thanks! @neolit123 thanks for testing!
As per slack discussion here
/milestone v1.13
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Nov 22, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bart0sh, fabriziopandini

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2018
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to mention I'm everything but happy about this (same for join) from a correctness and code health perspective, but being pragmatic it's a pretty useful feature, and we do appreciate your feedback on what should be improved in kubeadm. So as I'm letting this and the same for join merge, we simply make the general contract that flags should override config.

When this has merged, make sure you open a fresh issue for v1.14 that makes sure we track starting to merge flags with config generically and correctly in the next version.

/lgtm

@bart0sh
Copy link
Contributor Author

bart0sh commented Nov 22, 2018

@luxas

we simply make the general contract that flags should override config

I thought it's a common practice for command line utilities, no?

open a fresh issue for v1.14 that makes sure we track starting to merge flags with config generically and correctly in the next version

I'd be happy to work on that. Can you create an issue?

@k8s-ci-robot k8s-ci-robot merged commit b6a0718 into kubernetes:master Nov 22, 2018
@luxas
Copy link
Member

luxas commented Nov 22, 2018

I thought it's a common practice for command line utilities, no?

No, only the kubelet does it correctly. Hence the proposal of https://docs.google.com/document/d/1nZnzJD9dC0xrtEla2Xa-J6zobbC9oltdHIJ3KKSSIhk/edit#.

I'd be happy to work on that. Can you create an issue?

See linked proposal for work items. I'll create an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants