Implement --archive flag for podman cp #10804

matejvasek · 2021-06-28T19:20:14Z

Implement --archive flag for podman cp

This PR implements for direction when a container is a destination, not a source.

resolves #10801

Signed-off-by: Matej Vasek [email protected]

cmd/podman/containers/cp.go

pkg/domain/entities/engine_container.go

pkg/api/handlers/compat/containers_archive.go

rhatdan · 2021-06-28T20:15:50Z

You should also add the other follow-link option

-L, --follow-link Always follow symbol link in SRC_PATH

I am not sure what it does, but we need to have it implemented.

matejvasek · 2021-06-29T00:53:20Z

test/apiv2/23-containersArchive.at

+  ARCHIVE_TEST_ERROR="1"
+fi
+
+# TODO: uid/gid should be also preserved on way back (GET request)


This is however task for another PR.

matejvasek · 2021-06-29T00:56:52Z

You should also add the other follow-link option

-L, --follow-link Always follow symbol link in SRC_PATH

I am not sure what it does, but we need to have it implemented.

I would prefer this to be done in another PR.

cmd/podman/containers/cp.go

docs/source/markdown/podman-cp.1.md

libpod/container_api.go

pkg/domain/entities/engine_container.go

vrothberg · 2021-06-29T12:47:48Z

You should also add the other follow-link option
-L, --follow-link Always follow symbol link in SRC_PATH
I am not sure what it does, but we need to have it implemented.

I would prefer this to be done in another PR.

Yes, this should be another PR.

vrothberg

Thanks! Looking like a good direction.

cmd/podman/containers/cp.go

pkg/api/handlers/compat/containers_archive.go

pkg/bindings/containers/archive.go

pkg/domain/entities/containers.go

vrothberg

I just realized it's a breaking change to the bindings.

We should be able to handle everything on the client side in cmd/podman though. Both, copier.Put and copier.Get allow for chowning. In order to get the data, I think we need to extend podman container inspect with the necessary information about the user.

The only drawback is that the libpod/archive endpoint would not chown by default anymore, which is a risk I am willing to take.

cmd/podman/containers/cp.go

docs/source/markdown/podman-cp.1.md

vrothberg · 2021-06-29T14:52:20Z

pkg/bindings/containers/archive.go

@@ -49,12 +49,17 @@ func Stat(ctx context.Context, nameOrID string, path string) (*entities.Containe
 	return statReport, finalErr
 }

-func CopyFromArchive(ctx context.Context, nameOrID string, path string, reader io.Reader) (entities.ContainerCopyFunc, error) {
+func CopyFromArchive(ctx context.Context, nameOrID string, path string, reader io.Reader, options *CopyOptions) (entities.ContainerCopyFunc, error) {


Sorry for noticing just now. This would break the bindings which is something we cannot do at the moment without having to bump Podman to 4.0.

matejvasek · 2021-06-29T15:05:06Z

I just realized it's a breaking change to the bindings.

@vrothberg
There should be some compatibility:
User can use old podman-remote or an application compiled with old Binding lib it should work against new server.

vrothberg · 2021-06-29T15:09:11Z

I just realized it's a breaking change to the bindings.

@vrothberg
There should be some compatibility:
User can use old podman-remote or an application compiled with old Binding lib it should work against new server.

Very fair point. Seems like we'd break one way or another :(

@baude @rhatdan WDYT?

matejvasek · 2021-06-29T15:53:10Z

Very fair point. Seems like we'd break one way or another :(

Too bad that Go doesn't have function overloading 😞

vrothberg · 2021-06-29T16:01:51Z

Very fair point. Seems like we'd break one way or another :(

Too bad that Go doesn't have function overloading disappointed

An alternative solution would be to add a new function to the bindings which supports the new option. Add a clear comment that this will be removed with 4.0 and should not be used by any outside user. It's a trick but sounds like a good compromise at the moment. We can move on with the work and existing (bindings) users won't suddenly break.

matejvasek · 2021-06-30T13:33:24Z

@vrothberg @rhatdan PTAL

matejvasek · 2021-06-30T13:42:38Z

That would work as well. Nice idea. Please make sure to drop a `// FIXME: drop variadic args with Podman 4.0`` in the code.

I thought we would keep the variadic. If they are to be deleted then I rather do original suggestion.

vrothberg · 2021-06-30T13:46:10Z

That would work as well. Nice idea. Please make sure to drop a `// FIXME: drop variadic args with Podman 4.0`` in the code.

I thought we would keep the variadic.

Yes, for consistency with other bindings.

If they are to be deleted then I rather do original suggestion.

OK, sounds good to me. Thank you!

openshift-ci · 2021-06-30T16:33:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matejvasek, rhatdan

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:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/bindings/containers/archive.go

vrothberg

Other than the naming nit: LGTM
@mheon PTAL

Signed-off-by: Matej Vasek <[email protected]>

rhatdan · 2021-07-01T17:14:34Z

/lgtm
@matejvasek Could you open a PR with containers/common to add --archive as a option, so users who want the Docker defaults can change the way podman works.

rhatdan · 2021-07-01T17:14:56Z

BTW @matejvasek nice work and thanks.

matejvasek · 2021-07-01T17:31:23Z

@matejvasek Could you open a PR with containers/common to add --archive as a option, so users who want the Docker defaults can change the way podman works.

to containers/common? where?

rhatdan · 2021-07-01T17:58:30Z

Yes https://github.com/containers/common/tree/main/pkg/config

vrothberg · 2021-07-01T18:02:53Z

To add a new field to containers.conf which, if set, would overwrite the default value of --archive.

I think it should go under the "[engine]" table. Naming is always the hardest ... "chown_copied_files"?

rhatdan · 2021-07-01T19:34:30Z

Yes this one will need a good description.

ocafebabe · 2021-09-30T16:51:05Z

@matejvasek are there still plans to implement "follow-link" (-L) like docker?

matejvasek · 2021-09-30T19:41:15Z

@ocafebabe I haven't seen need for it so far (for my use), but feel free to submit PR.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2021

matejvasek force-pushed the fix-cp-sub-cmd branch from 815d15b to c9ddf67 Compare June 28, 2021 19:48

matejvasek commented Jun 28, 2021

View reviewed changes

cmd/podman/containers/cp.go Outdated Show resolved Hide resolved

matejvasek commented Jun 28, 2021

View reviewed changes

pkg/domain/entities/engine_container.go Outdated Show resolved Hide resolved

matejvasek commented Jun 28, 2021

View reviewed changes

pkg/api/handlers/compat/containers_archive.go Outdated Show resolved Hide resolved

matejvasek force-pushed the fix-cp-sub-cmd branch 3 times, most recently from cdafa5d to 56d259c Compare June 29, 2021 00:52

matejvasek commented Jun 29, 2021

View reviewed changes

matejvasek changed the title ~~[WIP]Implement --archive flag for podman cp~~ Implement --archive flag for podman cp Jun 29, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2021

rhatdan approved these changes Jun 29, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2021

matejvasek force-pushed the fix-cp-sub-cmd branch from 56d259c to ec4f9d4 Compare June 29, 2021 12:23

matejvasek force-pushed the fix-cp-sub-cmd branch 2 times, most recently from 3d5e292 to 1b58fb6 Compare June 29, 2021 12:58

vrothberg reviewed Jun 29, 2021

View reviewed changes

cmd/podman/containers/cp.go Outdated Show resolved Hide resolved

pkg/api/handlers/compat/containers_archive.go Outdated Show resolved Hide resolved

pkg/bindings/containers/archive.go Show resolved Hide resolved

pkg/domain/entities/containers.go Show resolved Hide resolved

matejvasek force-pushed the fix-cp-sub-cmd branch 4 times, most recently from 4c3f4e0 to 596bfaa Compare June 29, 2021 13:56

vrothberg reviewed Jun 29, 2021

View reviewed changes

matejvasek force-pushed the fix-cp-sub-cmd branch from 596bfaa to 33bd2f6 Compare June 29, 2021 16:05

matejvasek force-pushed the fix-cp-sub-cmd branch from 41ccfb9 to 95f32a6 Compare June 30, 2021 13:33

matejvasek force-pushed the fix-cp-sub-cmd branch from 95f32a6 to 759ee27 Compare June 30, 2021 13:40

matejvasek force-pushed the fix-cp-sub-cmd branch 6 times, most recently from 9e7a39b to f71d271 Compare June 30, 2021 19:22

vrothberg reviewed Jul 1, 2021

View reviewed changes

pkg/bindings/containers/archive.go Outdated Show resolved Hide resolved

vrothberg reviewed Jul 1, 2021

View reviewed changes

Implement --archive flag for podman cp

86c6014

Signed-off-by: Matej Vasek <[email protected]>

matejvasek force-pushed the fix-cp-sub-cmd branch from f71d271 to 86c6014 Compare July 1, 2021 10:01

openshift-ci bot assigned rhatdan Jul 1, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2021

openshift-merge-robot merged commit 955c1d2 into containers:main Jul 1, 2021

matejvasek mentioned this pull request Jul 11, 2022

File ownership on files copied to the <app> dir are set to root when using Podman buildpacks/pack#1395

Closed

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 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement --archive flag for podman cp #10804

Implement --archive flag for podman cp #10804

matejvasek commented Jun 28, 2021 •

edited

Loading

rhatdan commented Jun 28, 2021

matejvasek Jun 29, 2021

matejvasek commented Jun 29, 2021

vrothberg commented Jun 29, 2021

vrothberg left a comment

vrothberg left a comment

vrothberg Jun 29, 2021

matejvasek commented Jun 29, 2021

vrothberg commented Jun 29, 2021

matejvasek commented Jun 29, 2021

vrothberg commented Jun 29, 2021

matejvasek commented Jun 30, 2021

matejvasek commented Jun 30, 2021

vrothberg commented Jun 30, 2021

openshift-ci bot commented Jun 30, 2021

vrothberg left a comment

rhatdan commented Jul 1, 2021

rhatdan commented Jul 1, 2021

matejvasek commented Jul 1, 2021

rhatdan commented Jul 1, 2021 •

edited

Loading

vrothberg commented Jul 1, 2021

rhatdan commented Jul 1, 2021

ocafebabe commented Sep 30, 2021

matejvasek commented Sep 30, 2021

Implement --archive flag for podman cp #10804

Implement --archive flag for podman cp #10804

Conversation

matejvasek commented Jun 28, 2021 • edited Loading

rhatdan commented Jun 28, 2021

matejvasek Jun 29, 2021

Choose a reason for hiding this comment

matejvasek commented Jun 29, 2021

vrothberg commented Jun 29, 2021

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Jun 29, 2021

Choose a reason for hiding this comment

matejvasek commented Jun 29, 2021

vrothberg commented Jun 29, 2021

matejvasek commented Jun 29, 2021

vrothberg commented Jun 29, 2021

matejvasek commented Jun 30, 2021

matejvasek commented Jun 30, 2021

vrothberg commented Jun 30, 2021

openshift-ci bot commented Jun 30, 2021

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Jul 1, 2021

rhatdan commented Jul 1, 2021

matejvasek commented Jul 1, 2021

rhatdan commented Jul 1, 2021 • edited Loading

vrothberg commented Jul 1, 2021

rhatdan commented Jul 1, 2021

ocafebabe commented Sep 30, 2021

matejvasek commented Sep 30, 2021

matejvasek commented Jun 28, 2021 •

edited

Loading

rhatdan commented Jul 1, 2021 •

edited

Loading