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

Fix 'kubectl cp' with no arguments causes a panic #65482

Merged
merged 1 commit into from
Jul 3, 2018
Merged

Fix 'kubectl cp' with no arguments causes a panic #65482

merged 1 commit into from
Jul 3, 2018
+3 −0

Conversation

wgliang
Copy link
Contributor

@wgliang wgliang commented Jun 26, 2018

What this PR does / why we need it:
/kind bug

"kubectl cp" with no arguments causes a panic

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #65475

Special notes for your reviewer:

Release note:

Fix 'kubectl cp' with no arguments causes a panic

@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/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. labels Jun 26, 2018
@k8s-ci-robot k8s-ci-robot requested review from dshulyak and eparis June 26, 2018 14:26
@wgliang
Copy link
Contributor Author

wgliang commented Jun 26, 2018

/priority important-soon

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 26, 2018
juanvallejo
juanvallejo reviewed Jun 26, 2018
View reviewed changes
pkg/kubectl/cmd/cp.go Outdated
@@ -164,6 +164,9 @@ func (o *CopyOptions) Validate(cmd *cobra.Command, args []string) error {
}

func (o *CopyOptions) Run(args []string) error {
if len(args) < 2 {
return fmt.Errorf("Unexpected args: %v", args)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think suggesting to the user what is required might make more sense:
a source and a destination are required or something along those lines

@wgliang wgliang force-pushed the master.handle-args branch from 14d6e4d to 630bbf1 Compare June 26, 2018 22:54
@wgliang
Copy link
Contributor Author

wgliang commented Jun 27, 2018

/test pull-kubernetes-e2e-gce

@CaoShuFeng
Copy link
Contributor

/lgtm

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

wgliang commented Jun 27, 2018

/assign @janetkuo
PTAL

@wgliang
Copy link
Contributor Author

wgliang commented Jun 27, 2018

NOTE:
Need to cherrypick to past versions.

@nikhita
Copy link
Member

nikhita commented Jun 27, 2018

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jun 27, 2018
@tuminoid
Copy link

Does this also fix panic in #65475 (comment) ie. kubectl cp file container: ?

@tuminoid
Copy link

Answering myself: no. The fix specifically handles case with no arguments at all.

@tuminoid tuminoid mentioned this pull request Jun 27, 2018
Closed
@nikhita
Copy link
Member

nikhita commented Jul 1, 2018 via email

@wgliang
Copy link
Contributor Author

wgliang commented Jul 2, 2018

/ping @kubernetes/sig-cli-maintainers
PTAL

@juanvallejo
Copy link
Contributor

/lgtm

@soltysh for approval

soltysh
soltysh requested changes Jul 2, 2018
View reviewed changes
pkg/kubectl/cmd/cp.go Outdated
@@ -164,6 +164,9 @@ func (o *CopyOptions) Validate(cmd *cobra.Command, args []string) error {
}

func (o *CopyOptions) Run(args []string) error {
if len(args) < 2 {
return fmt.Errorf("Unexpected args: %v, a source and a destination are required", args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just source and destination are required will be sufficient.

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,
for approve

@wgliang wgliang force-pushed the master.handle-args branch from 630bbf1 to 254b02b Compare July 3, 2018 02:09
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2018
@nikhita
Copy link
Member

nikhita commented Jul 3, 2018

/retest

soltysh
soltysh approved these changes Jul 3, 2018
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 Jul 3, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, juanvallejo, soltysh, wgliang

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 Jul 3, 2018
@wgliang
Copy link
Contributor Author

wgliang commented Jul 3, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 0c698a4 into kubernetes:master Jul 3, 2018
@wgliang wgliang mentioned this pull request Jul 3, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Jul 7, 2018
Merge pull request #65789 from wgliang/automated-cherry-pick-of-#6548…
ba1529e 
…2-upstream-release-1.11

Automatic merge from submit-queue.

Automated cherry pick of #65482: fix 'kubectl cp' with no arguments causes a panic

Cherry pick of #65482 on release-1.11.

#65482: Fix 'kubectl cp' with no arguments causes a panic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanvallejo juanvallejo juanvallejo approved these changes

@soltysh soltysh soltysh approved these changes

@dshulyak dshulyak Awaiting requested review from dshulyak

@eparis eparis Awaiting requested review from eparis

Assignees

@soltysh soltysh

@juanvallejo juanvallejo

@janetkuo janetkuo

@CaoShuFeng CaoShuFeng

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"kubectl cp" with no arguments causes a panic
9 participants
@wgliang @CaoShuFeng @nikhita @tuminoid @juanvallejo @k8s-ci-robot @k8s-github-robot @soltysh @janetkuo