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

podman cp: do not overwrite non-dirs with dirs and vice versa #14526

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

vrothberg
Copy link
Member

Add a new --overwrite flag to podman cp to allow for overwriting in
case existing users depend on the behavior; they will have a workaround.
By default, the flag is turned off to be compatible with Docker and to
have a more sane behavior.

Fixes: #14420
Signed-off-by: Valentin Rothberg [email protected]

Does this PR introduce a user-facing change?

Fix a bug where `podman cp` would overwrite directories with non-directories and vice versa.  A new `--overwrite` flag allows for opting into the old behavior if needed.

@Luap99 @mheon @jwhonce PTAL

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 8, 2022
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.

I think the API still defaults to overwrite, at least I see no changes in here for that.

docs/source/markdown/podman-cp.1.md Outdated Show resolved Hide resolved
cmd/podman/containers/cp.go Outdated Show resolved Hide resolved
test/system/065-cp.bats Outdated Show resolved Hide resolved
test/system/065-cp.bats Outdated Show resolved Hide resolved
test/system/065-cp.bats Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2022
@vrothberg
Copy link
Member Author

Added more tests but just noticed that the remote errors don't work as expected.

@vrothberg
Copy link
Member Author

@flouthoc the recent buildah changes are biting my butt. Should the logfile test be skipped on remote? I don't see --logfile being plugged into remote.

@vrothberg
Copy link
Member Author

[+0543s] not ok 112 bud-logfile-with-split-logfile-by-platform
[+0543s] # (from function `assert' in file tests/helpers.bash, line 465,
[+0543s] #  from function `expect_output' in file tests/helpers.bash, line 492,
[+0543s] #  in test file tests/bud.bats, line 1722)
[+0543s] #   `expect_output --substring "FROM alpine"' failed
[+0543s] # /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.26.1-0.20220607182634-005447be07ee/tests /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.26.1-0.20220607182634-005447be07ee
[+0543s] # $ podman build --force-rm=false --layers=false --logfile /var/tmp/buildah_tests.dvbiqm/logfile --logsplit --platform linux/arm64,linux/amd64 --signature-policy /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.26.1-0.20220607182634-005447be07ee/tests/policy.json /var/tmp/buildah_tests.dvbiqm/my-dir
[+0543s] # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
[+0543s] # #|     FAIL: podman build --force-rm=false --layers=false --logfile /var/tmp/buildah_tests.dvbiqm/logfile --logsplit --platform linux/arm64,linux/amd64 --signature-policy /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.26.1-0.20220607182634-005447be07ee/tests/policy.json /var/tmp/buildah_tests.dvbiqm/my-dir
[+0543s] # #| expected: =~ 'FROM alpine'
[+0543s] # #|   actual:    'cat: /var/tmp/buildah_tests.dvbiqm/logfile_linux_arm64: No such file or directory'
[+0543s] # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[+0543s] # /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.26.1-0.20220607182634-005447be07ee

@flouthoc
Copy link
Collaborator

flouthoc commented Jun 8, 2022

@vrothberg It would need implementation on API end. Yes please feel free to skip it for now and i'll pick it up in a followup.

@vrothberg
Copy link
Member Author

@vrothberg It would need implementation on API end. Yes please feel free to skip it for now and i'll pick it up in a followup.

Does it make sense for remote? The file should ultimately be written on the client-side ... I guess

@flouthoc
Copy link
Collaborator

flouthoc commented Jun 8, 2022

@vrothberg It would need implementation on API end. Yes please feel free to skip it for now and i'll pick it up in a followup.

Does it make sense for remote? The file should ultimately be written on the client-side ... I guess

@vrothberg Umm.. I am confused 😕 as of now since i don't see a skip for bud-logfile so I am assuming it works for remote and writes all the downloaded logs to client side so --logsplit should work as well in theory but it would be too much of a new implementation on client side and API end to make it work. So even if it happens or does not happens it looks like too big of a change to happen in this PR.

@vrothberg On the podman man-page could we add that --logsplit is not supported on remote as of now.

Note that the bud-logfile-with-split-logfile-by-platform test is skipped
on the remote client (see containers#14544).

Signed-off-by: Valentin Rothberg <[email protected]>
Add a new `--overwrite` flag to `podman cp` to allow for overwriting in
case existing users depend on the behavior; they will have a workaround.
By default, the flag is turned off to be compatible with Docker and to
have a more sane behavior.

Fixes: containers#14420
Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

Ready to merge. @flouthoc @Luap99 PTAL

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, vrothberg

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

@vrothberg
Copy link
Member Author

@rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Jun 10, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2022
@openshift-merge-robot openshift-merge-robot merged commit efc1936 into containers:main Jun 10, 2022
@vrothberg vrothberg deleted the fix-14420 branch June 10, 2022 11:53
Comment on lines +961 to +963
if ! is_remote; then # remote just returns a 500
is "$output" ".* error creating \"/tmp/foo\": .*: file exists.*"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Why is the error message different? We should return a reasonable error for the remote client.

Copy link
Member Author

Choose a reason for hiding this comment

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

We send 200 before (and hence can't overwrite the code) and use the ResponseWriter as the writer. To fix it, we should probably copy the archive to a temporary file and then copy that over the wire.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman cp can overwrite existing files/directories
7 participants