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

Fix handling of ":" in filenames in kubectl cp #78622

Open
wants to merge 3 commits into
base: master
from

Conversation

@odinuge
Copy link
Member

commented Jun 1, 2019

What type of PR is this?
/kind cleanup
/kind bug
/kind feature

What this PR does / why we need it:

This cleans up the filename handling and parsing inside kubectl-cp. Makes handling of files with ":" better, and adds a note in the docs about how it works.

It also adds support for the scp-like copy format "kubectl cp file ns/pod:", essentially resulting in "kubectl cp file ns/pod:.". This allows you to copy files to the containers working direcory without writing the trailing dot (a must for us who are used to scp).

And, it fixes the error message (and the strange behavior trying copyToPod) when trying to copy from pod to pod like: kubectl cp pod:file pod:file. Today it results in the strange error message "error: src doesn't exist in local filesystem"

/priority important-soon

Which issue(s) this PR fixes:

Fixes #72501

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add support for "kubectl cp file pod:" shorthand to copy into pod working directory
Make kubectl cp handle files with ":" properly
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

Hi @odinuge. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 requested review from adohe and juanvallejo Jun 1, 2019

@odinuge odinuge force-pushed the odinuge:kubectl-cp-cleanup branch from d7364a9 to dd156fd Jun 1, 2019

@odinuge odinuge changed the title Kubectl cp cleanup and fixes Fix handling of ":" in filenames in kubectl cp Jun 6, 2019

@odinuge

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

/assign @apelisse

Long: templates.LongDesc(`
Copy files and directories to and from containers.
Make local files containing ':' explicit by using relative or

This comment has been minimized.

Copy link
@apelisse

apelisse Jun 14, 2019

Member

I find this a little confusing. I'm not sure how to fix it though.

This comment has been minimized.

Copy link
@odinuge

odinuge Jun 14, 2019

Author Member

Yee, I kinda agree. I took the "idea" from the scp manpage, https://linux.die.net/man/1/scp, but couldn't find a better phrasing.. Hmm

This comment has been minimized.

Copy link
@odinuge

odinuge Jun 14, 2019

Author Member

What do you think about this, from the manpage of scp

"Local file names can be made explicit using absolute or relative pathnames to avoid scp kubectl cp treating file names containing ':' as pod specifiers."

This comment has been minimized.

Copy link
@apelisse

apelisse Jun 14, 2019

Member

the wording is much better, I would definitely give an example right next to that sentence though, otherwise it still might not make sense.

I read "relative or absolute" as complementary, hence not very useful.

But if that means it has to be either starting with / or ./ then I think an example might help.

This comment has been minimized.

Copy link
@odinuge

odinuge Jun 15, 2019

Author Member

Good point! Updated the description and added a small example now.

@odinuge

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Ref #78884 (comment):
/assign @soltysh

odinuge added some commits Jun 1, 2019

Cleanup file handling in kubectl cp
Cleanup parsing of filenames/paths, and make copies with filenames
including colons better.

Signed-off-by: Odin Ugedal <odin@ugedal.com>
Add error message on pod-pod cp in kubectl
Previously trying to copy from pod to pod would result in:
"src doesn't exist in local filesystem"

Signed-off-by: Odin Ugedal <odin@ugedal.com>
Add support for scp-like kubectl cp file pod:
Add support for the scp-like copy format "kubectl cp file ns/pod:",
essentially resulting in "kubectl cp file ns/pod:.". This allows you to
copy files to the containers working direcory without writing the
trailing dot.

Signed-off-by: Odin Ugedal <odin@ugedal.com>

@odinuge odinuge force-pushed the odinuge:kubectl-cp-cleanup branch from dd156fd to b35a3de Jun 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: odinuge
To complete the pull request process, please assign apelisse
You can assign the PR to them by writing /assign @apelisse in a comment when ready.

The full list of commands accepted by this bot can be found 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

@apelisse

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Change looks good to me, I'll let @soltysh approve.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 17, 2019

@odinuge

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@odinuge: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@apelisse

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

/ok-to-test
/retest

@odinuge

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

Ping @soltysh 😄

@odinuge odinuge referenced this pull request Jul 1, 2019

Closed

REQUEST: New membership for odinuge #975

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.