rewrite container copy #8570
rewrite container copy #8570
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci-robot
added
the
approved
|
@edsantiago, I would love to have your feedback on the system tests. I added a lot of them. Given |
| @@ -43,7 +40,7 @@ var ( | |||
| func cpFlags(cmd *cobra.Command) { | |||
| flags := cmd.Flags() | |||
| flags.BoolVar(&cpOpts.Extract, "extract", false, "Extract the tar file into the destination directory.") | |||
| flags.BoolVar(&cpOpts.Pause, "pause", copyPause(), "Pause the container while copying") | |||
There was a problem hiding this comment.
We should probably deprecate this - it's an unnecessary addition (we fixed the CVE it was supposed to resolve in a different way).
There was a problem hiding this comment.
Sounds good to me. Maybe something for a follow up PR when working on remote cp. Will spare us from adding the parameters.
I think we can leave it on the CLI but make it a NOP? Or full removal given 3.0 would allow us to?
There was a problem hiding this comment.
Leave it but remove documentation, and hide the option from --help.
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
I may have missed the convo, but any thoughts on moving copier out of Buildah and into c/common? This is one area that it would have been nice if Buildah/Podman used the same containers, we could make a bunch more of this code common between the two. |
I am constantly hitting the 90 seconds limit with my very slow connection. Signed-off-by: Valentin Rothberg <[email protected]>
SGTM |
|
No need to block this PR. I am not crazy about moving Copier out of Buidlah since it is really tied to buildah[-copy/COPY functions in buildah. |
vrothberg
force-pushed
the
run-950
branch
2 times, most recently
from
cd5b7ec to
3096826
Compare
|
Changed the system tests to be table driven where applicable 👍 |
* Add a new `pkg/copy` to centralize all container-copy related code. * The new code is based on Buildah's `copier` package. * The compat `/archive` endpoints use the new `copy` package. * Update docs and an several new tests. * Includes many fixes, most notably, the look-up of volumes and mounts. Breaking changes: * Podman is now expecting that container-destination paths exist. Before, Podman created the paths if needed. Docker does not do that and I believe Podman should not either as it's a recipe for masking errors. These errors may be user induced (e.g., a path typo), or internal typos (e.g., when the destination may be a mistakenly unmounted volume). Let's keep the magic low for such a security sensitive feature. Signed-off-by: Valentin Rothberg <[email protected]>
|
@containers/podman-maintainers PTAL |
|
Looks like the ' rdoproject.org/github-check' is failing here and in other PRs this morning with a retry limit failure of F31. I'm not sure if there's anything we can do with that. |
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Add a new
pkg/copyto centralize all container-copy related code.The new code is based on Buildah's
copierpackage.The compat
/archiveendpoints use the newcopypackage.Update docs and add several new tests.
Includes many fixes, most notably, the look-up of volumes and mounts.
Breaking changes:
Before, Podman created the paths if needed. Docker does not do
that and I believe Podman should not either as it's a recipe for
masking errors. These errors may be user induced (e.g., a path
typo), or internal typos (e.g., when the destination may be a
mistakenly unmounted volume). Let's keep the magic low for such
a security sensitive feature.
Signed-off-by: Valentin Rothberg [email protected]