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

Add podman machine cp subcommand #25331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakecorrenti
Copy link
Member

Does this PR introduce a user-facing change?

Adds a new podman machine cp subcommand. This will allow users to copy files or directories between a running Podman Machine and their host.

Add a new `podman machine cp` subcommand

Copy link
Contributor

openshift-ci bot commented Feb 14, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jakecorrenti
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

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

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Note we generally squash all commits into one for a new feature, i.e. code, docs, test should be in one commit. Of course if there are doing different features/bugs then not but in this case it is only one.

cmd/podman/machine/cp.go Outdated Show resolved Hide resolved
pkg/machine/config.go Outdated Show resolved Hide resolved
pkg/machine/e2e/cp_test.go Outdated Show resolved Hide resolved
pkg/machine/e2e/cp_test.go Outdated Show resolved Hide resolved
@jakecorrenti jakecorrenti force-pushed the machine-cp branch 2 times, most recently from 914a438 to 55cf202 Compare February 17, 2025 23:24
pkg/machine/e2e/cp_test.go Outdated Show resolved Hide resolved
cmd/podman/machine/cp.go Outdated Show resolved Hide resolved
cmd/podman/machine/cp.go Outdated Show resolved Hide resolved
Comment on lines 135 to 145
if testProvider.VMType() == define.AppleHvVirt {
Expect(session.errorToString()).To(ContainSubstring(fmt.Sprintf("scp: open local \"%s\": No such file or directory", hostDirPath)))
} else {
Expect(session.errorToString()).To(ContainSubstring(fmt.Sprintf("scp: open local \"%s\": Is a directory", hostDirPath)))
}
Copy link
Member Author

@jakecorrenti jakecorrenti Feb 18, 2025

Choose a reason for hiding this comment

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

I don't have a windows machine accessible at the moment, so I still need to figure out the error message checking here (I should also be checking if GOOS is macos not the provider here...)

@jakecorrenti jakecorrenti force-pushed the machine-cp branch 3 times, most recently from eaf21be to 0ed0ba3 Compare February 18, 2025 18:50
cmd/podman/machine/cp.go Outdated Show resolved Hide resolved
@baude
Copy link
Member

baude commented Feb 19, 2025

one question i guess else LGTM

@jakecorrenti jakecorrenti force-pushed the machine-cp branch 2 times, most recently from ea13c54 to 602102f Compare February 19, 2025 19:26
Add a new `podman machine cp` subcommand to allow users to copy files or
directories between a running Podman Machine and their host.

Tests cover the following cases:
- Copy a file from the host machine to the VM
- Copy a directory from the host machine to the VM
- Copy a file from the VM to the host machine
- Copy a directory from the VM to the host machine
- Copy a file to a directory
- Copy a directory to a file

Signed-off-by: Jake Correnti <[email protected]>
cmd/podman/machine/machine.go Show resolved Hide resolved
cmd/podman/machine/machine.go Show resolved Hide resolved
}

// Passing an absolute windows path of the format <volume>:\<path> will cause
// `copy.ParseSourceAndDestination` to think the volume is a Machine. Check
Copy link
Member

Choose a reason for hiding this comment

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

just thinking about it does podman cp not have the exact same issue, i.e. should this fix be moved into ParseSourceAndDestination()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't that result in the same issue mentioned below having to do with single name containers?

cmd/podman/machine/cp.go Show resolved Hide resolved
Comment on lines +70 to +78
if specgen.IsHostWinPath(args[0]) {
srcMachine = ""
srcPath = args[0]
}

if specgen.IsHostWinPath(args[1]) {
destMachine = ""
destPath = args[1]
}
Copy link
Member

Choose a reason for hiding this comment

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

Also isn't this breaking for machine names with one letter? i.e. I think it runs into the same issue as shown in #25323

We do allow a machine called C so that can be a problem although super unlikely that anyone would be using single letter machine names hopefully so I am fine to ignore that part for now. Though adding this issue in a comment here might help future readers.

pkg/machine/e2e/cp_test.go Show resolved Hide resolved
pkg/machine/e2e/cp_test.go Show resolved Hide resolved
pkg/machine/e2e/cp_test.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants