Skip to content

Support kubectl more flexible matching method to match whether the current resource type is node #88723

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 28, 2020
Merged

Support kubectl more flexible matching method to match whether the current resource type is node #88723

merged 1 commit into from
Apr 28, 2020
+20 −8

Conversation

wawa0210
Copy link
Contributor

@wawa0210 wawa0210 commented Mar 2, 2020 •
edited
Loading

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Before the kubectl taint command only supported ["node", "nodes"], Now supports substitution by o.Mapper.KindFor, more flexible

Does this PR introduce a user-facing change?:

kubectl supports taint no without specifying(without having to type the full resource name)

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

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 Mar 2, 2020
@wawa0210
Copy link
Contributor Author

wawa0210 commented Mar 2, 2020

/assign @mengqiy

@k8s-ci-robot k8s-ci-robot requested review from mengqiy and soltysh March 2, 2020 12:24
@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 2, 2020
@zhouya0
Copy link
Contributor

zhouya0 commented Mar 3, 2020

/lgtm Thanks for this fix.

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 18, 2020
@wawa0210
Copy link
Contributor Author

wawa0210 commented Apr 7, 2020

@soltysh @mengqiy

Can you spare some time to review this pr, thank you very much

soltysh
soltysh requested changes Apr 23, 2020
View reviewed changes
staging/src/k8s.io/kubectl/pkg/cmd/taint/taint.go Outdated
@@ -219,7 +219,7 @@ func (o TaintOptions) validateFlags() error {
// Validate checks to the TaintOptions to see if there is sufficient information run the command.
func (o TaintOptions) Validate() error {
resourceType := strings.ToLower(o.resources[0])
validResources, isValidResource := []string{"node", "nodes"}, false
validResources, isValidResource := []string{"node", "nodes", "no"}, false
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very hacky, I'd prefer you use o.Mapper.KindFor instead and then just verify if that's node or not. That's the preferred approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I have some doubts, can you describe the details

@wawa0210 wawa0210 force-pushed the kubectl_taint_short_forms branch from b4044b5 to 21bafa5 Compare April 24, 2020 10:22
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 24, 2020
@wawa0210 wawa0210 changed the title Kubectl Taint now also accepts short forms for "node","nodes","no" Support kubectl more flexible matching method to match whether the current resource type is node Apr 24, 2020
@wawa0210 wawa0210 force-pushed the kubectl_taint_short_forms branch 3 times, most recently from af2fe86 to 147bde6 Compare April 26, 2020 02:45
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2020
@wawa0210 wawa0210 force-pushed the kubectl_taint_short_forms branch from 147bde6 to 7d66a0b Compare April 26, 2020 02:58
@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 26, 2020
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 26, 2020
@wawa0210 wawa0210 force-pushed the kubectl_taint_short_forms branch 2 times, most recently from c5c8295 to a5c2529 Compare April 26, 2020 03:46
soltysh
soltysh requested changes Apr 27, 2020
View reviewed changes
staging/src/k8s.io/kubectl/pkg/cmd/taint/taint.go Outdated

gvk, _ := o.Mapper.KindFor(fullySpecifiedGVR)
if gvk.Empty() {
gvk, err = o.Mapper.KindFor(fullySpecifiedGVR.GroupResource().WithVersion(""))
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need that fallback, it will either match or not so you should check error from KindFor execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@wawa0210 wawa0210 force-pushed the kubectl_taint_short_forms branch from a5c2529 to 5549527 Compare April 27, 2020 15:10
@wawa0210
Kubectl Taint now also accepts short forms for "node","nodes","no"

Verified

This commit was signed with the committer’s verified signature. The key has expired.
wawa0210 Xiao Zhang
GPG key ID: 900C83A2C098B3B1
Expired
Verified
Learn about vigilant mode
8fda1f3
@wawa0210 wawa0210 force-pushed the kubectl_taint_short_forms branch from 5549527 to 8fda1f3 Compare April 27, 2020 15:21
soltysh
soltysh approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, wawa0210

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 Apr 27, 2020
@soltysh
Copy link
Contributor

soltysh commented Apr 27, 2020

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 27, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@wawa0210
Copy link
Contributor Author

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot merged commit 7814f3a into kubernetes:master Apr 28, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Apr 28, 2020
@wawa0210 wawa0210 deleted the kubectl_taint_short_forms branch April 28, 2020 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh

@mengqiy mengqiy

Assignees

@soltysh soltysh

@mengqiy mengqiy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl 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/feature Categorizes issue or PR as related to a new feature. 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/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wawa0210 @zhouya0 @k8s-ci-robot @soltysh @fejta-bot @mengqiy