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.

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

kubectl version should fail when given extra arguments #107967

Merged
merged 1 commit into from
Mar 4, 2022
Merged

kubectl version should fail when given extra arguments #107967

merged 1 commit into from
Mar 4, 2022

Conversation

jlsong01
Copy link
Contributor

@jlsong01 jlsong01 commented Feb 5, 2022 •
edited

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubectl version should fail when given extra arguments

Which issue(s) this PR fixes:

Fixes #107955

Special notes for your reviewer:

Does this PR introduce a user-facing change?

<kubectl version> fails for unexpected extra arguments

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

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. 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. labels Feb 5, 2022
@k8s-ci-robot
Copy link
Contributor

@jlsong01: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 5, 2022
@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 5, 2022
@jlsong01
Copy link
Contributor Author

jlsong01 commented Feb 5, 2022

/sig cli

@jlsong01 jlsong01 force-pushed the check_kubectl_version_extra_arguments branch from 9c42b07 to a3a8efb Compare February 6, 2022 14:45
@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 Feb 6, 2022
@jlsong01 jlsong01 requested a review from yxxhero February 6, 2022 15:08
@jlsong01
Copy link
Contributor Author

jlsong01 commented Feb 6, 2022

@ardaguclu @yxxhero PTAL

@jlsong01
Copy link
Contributor Author

jlsong01 commented Feb 7, 2022

/retest

@yxxhero
Copy link
Member

yxxhero commented Feb 8, 2022

@jlsong01 Incomplete unit test, missing verification with args length greater than 0. WDYT?

@jlsong01 jlsong01 force-pushed the check_kubectl_version_extra_arguments branch from a3a8efb to 3c3ec32 Compare February 8, 2022 09:17
@jlsong01
Copy link
Contributor Author

jlsong01 commented Feb 8, 2022 •
edited

verification with args length greater than 0

@yxxhero Yeah, Thanks for the comments. A unit test case with args length greater than 0 has been added.

@yxxhero
Copy link
Member

yxxhero commented Feb 8, 2022

@jlsong01 Validate is an export method. If you directly change the function signature, I don't know whether you need API review.

@jlsong01
Copy link
Contributor Author

jlsong01 commented Feb 8, 2022 •
edited

@jlsong01 Validate is an export method. If you directly change the function signature, I don't know whether you need API review.

@yxxhero Yeah, seems it's better to keep the export method signature unchanged.

So, I would add a new function like validateArgs(args []string) error which wraps the current Validate and additionally validate args length greater than 0. how about it?

Any other suggestions are welcome

@yxxhero
Copy link
Member

yxxhero commented Feb 8, 2022

@jlsong01 I don't know very well. Need to wait for the owner's reply

@jlsong01
Copy link
Contributor Author

@ardaguclu could you take another look please?

@jlsong01 jlsong01 force-pushed the check_kubectl_version_extra_arguments branch from 3c3ec32 to b2e6a02 Compare February 10, 2022 07:47
@jlsong01
Copy link
Contributor Author

/retest

@ardaguclu
Copy link
Member

/lgtm

@k8s-ci-robot
Copy link
Contributor

@ardaguclu: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@jlsong01
Copy link
Contributor Author

@dougsland @pwittrock could you review it please?

@jlsong01
Copy link
Contributor Author

@KnVerey @seans3 Could you please help to review it?

@jlsong01
Copy link
Contributor Author

ping @dougsland :-)

dougsland
dougsland reviewed Feb 20, 2022
View reviewed changes
Copy link
Member

@dougsland dougsland 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 Feb 20, 2022
@jlsong01
Copy link
Contributor Author

/assign @soltysh
for approval

@sftim
Copy link
Contributor

sftim commented Feb 22, 2022

Folks, I'd expect to see a changelog entry for this PR.

@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 release-note-none Denotes a PR that doesn't merit a release note. labels Feb 23, 2022
@jlsong01
Copy link
Contributor Author

Folks, I'd expect to see a changelog entry for this PR.

Thanks for the comments. The changelog entry has been added into the PR description. PTAL

@jlsong01
Copy link
Contributor Author

@sftim any further comments?

eddiezane
eddiezane suggested changes Mar 2, 2022
View reviewed changes
Copy link
Member

@eddiezane eddiezane left a comment

Choose a reason for hiding this comment

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

Thanks! Small change then lgtm

staging/src/k8s.io/kubectl/pkg/cmd/version/version.go Outdated Show resolved Hide resolved
@jlsong01 jlsong01 force-pushed the check_kubectl_version_extra_arguments branch from b2e6a02 to 969cc46 Compare March 3, 2022 08:28
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2022
@jlsong01 jlsong01 requested a review from eddiezane March 3, 2022 10:13
eddiezane
eddiezane approved these changes Mar 4, 2022
View reviewed changes
Copy link
Member

@eddiezane eddiezane left a comment

Choose a reason for hiding this comment

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

Thanks!

/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 Mar 4, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eddiezane, jlsong01

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 Mar 4, 2022
@jlsong01
Copy link
Contributor Author

jlsong01 commented Mar 4, 2022

/retest

@k8s-ci-robot k8s-ci-robot merged commit a330194 into kubernetes:master Mar 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 4, 2022
@github-actions github-actions bot mentioned this pull request Mar 22, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh left review comments

@dougsland dougsland dougsland left review comments

@ardaguclu ardaguclu ardaguclu left review comments

@eddiezane eddiezane eddiezane approved these changes

@pwittrock pwittrock Awaiting requested review from pwittrock

@yxxhero yxxhero Awaiting requested review from yxxhero

Assignees

@soltysh soltysh

@dougsland dougsland

@eddiezane eddiezane

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. 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.24
Development

Successfully merging this pull request may close these issues.

kubectl version should fail when given extra arguments
8 participants
@jlsong01 @k8s-ci-robot @yxxhero @ardaguclu @sftim @soltysh @dougsland @eddiezane