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: improve tips of incorrect input of kubedm subcommand #111277

Merged

Conversation

chymy
Copy link
Contributor

@chymy chymy commented Jul 20, 2022

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

Now kubeadm all commands behave the same way. For beginners, there should be more friendly tips for some sub commands,
Refer to this kubernetes/kubeadm#2728 for details

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2728

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: when a subcommand is needed but not provided for a kubeadm command, print a help screen instead of showing a short message.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 20, 2022
@chymy
Copy link
Contributor Author

chymy commented Jul 20, 2022

/cc @neolit123

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.

what happens with kubeadm {init|join|upgrade node} phase i guess they are unaffected by this change?

@neolit123
Copy link
Member

we definitely need to ensure this does not introduce behavior changes on the cli other than printing the help screen.

@chymy
Copy link
Contributor Author

chymy commented Jul 20, 2022

what happens with kubeadm {init|join|upgrade node} phase i guess they are unaffected by this change?

I just modified the command to call the SubCmdRun function, it only prints help sceen and prompts for incorrect input, and the status continue to be 1. For the checkErr, the switch case of help sceen and invalid subcommand are added.

kubeadm {init|join|upgrade node} phase does not use SubCmdRun, so they are unaffected by this change.

@chymy
Copy link
Contributor Author

chymy commented Jul 20, 2022

/test pull-kubernetes-e2e-gce-ubuntu-containerd

Copy link
Contributor

@yangjunmyfm192085 yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

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

/release-note-none

@neolit123
Copy link
Member

if this change makes it in it should have a release note.

/release-note-edit

kubeadm: when a subcommand is needed but not provided for a kubeadm command, print a help screen instead of showing a short message.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 20, 2022
@chymy chymy force-pushed the improve-kubeadm-subcommand-tips branch from f4d7de1 to 7bd1d90 Compare July 22, 2022 01:35
@RA489
Copy link

RA489 commented Jul 22, 2022

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 22, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2022
@pacoxu
Copy link
Member

pacoxu commented Aug 5, 2022

The code is frozen for v1.25 now, and this PR needs to be rebased and waited.

@chymy chymy force-pushed the improve-kubeadm-subcommand-tips branch from 7bd1d90 to ff2071b Compare August 8, 2022 08:24
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2022
@chymy
Copy link
Contributor Author

chymy commented Aug 8, 2022

The code is frozen for v1.25 now, and this PR needs to be rebased and waited.

Thanks, fixed.

@chymy
Copy link
Contributor Author

chymy commented Aug 8, 2022

/retest

@pacoxu
Copy link
Member

pacoxu commented Aug 8, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2022
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.

@SataQiu
please double check and approve if you think this is good to go.

@chymy chymy force-pushed the improve-kubeadm-subcommand-tips branch from ff2071b to 5086b88 Compare August 25, 2022 10:51
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2022
Signed-off-by: chymy <chang.min1@zte.com.cn>
@chymy chymy force-pushed the improve-kubeadm-subcommand-tips branch from 5086b88 to 749256e Compare August 26, 2022 01:19
@chymy
Copy link
Contributor Author

chymy commented Aug 26, 2022

Thanks for your review @neolit123 . fixed.

Copy link
Member

@pacoxu pacoxu left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2022
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.

/approve

let's watch if we somehow break user expectation.
thanks for the pr.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chymy, neolit123, pacoxu

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 Sep 9, 2022
@k8s-ci-robot k8s-ci-robot merged commit 2b2be7f into kubernetes:master Sep 9, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Sep 9, 2022
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve tips of incorrect input of kubedm some subcommand
7 participants