Add option `--keep-ownership` to ADD/COPY

[Dexus]

This PR is to keep the ownership of local files, when used with ADD or COPY - BUT WITHOUT --chown flag

fixes #226 , Ref: https://github.com/containers/buildah/pull/3767, containers/buildah/pull/4001

What are the the intention for the PR:

ADD or COPY

• without --chown and --keep-ownership result in root:root for the files.
• with --chown 123:456 result in 123:456 for the files.
• with --keep-ownership result in UID:GID for the files, same as the origin source.

[openshift-ci[bot]]

Hi @Dexus. Thanks for your PR.

I'm waiting for a openshift 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.

[Dexus]

/assign @mrunalp

[flouthoc]

I think this flag would be buildah only but it will also get parsed for dockerclient as well but would not be recognized by docker daemon, maybe we should just drop this fmt.Errorf from imagebuilder and let implementations take care of it. @nalind WDYT ?

default:
   return fmt.Errorf("ADD only supports the --keep-ownership --chmod=<permissions> and the --chown=<uid:gid> flag")
			

[openshift-ci[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Dexus To complete the pull request process, please ask for approval from mrunalp after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[flouthoc]

@Dexus Could you squash commits into one I don't think additional commits are needed you can keep the only first original commit. Adding a small test for this should be easy in dispatcher_test.

[flouthoc]

I think this flag would be buildah only but it will also get parsed for dockerclient as well but would not be recognized by docker daemon, maybe we should just drop this fmt.Errorf from imagebuilder and let implementations take care of it. @nalind WDYT ?

default:
   return fmt.Errorf("ADD only supports the --keep-ownership --chmod=<permissions> and the --chown=<uid:gid> flag")
			

Just waiting for a review by other maintainers on this point. Otherwise LGTM ( also this needs small test in dispatcher_test.go )

[Dexus]

/cc @TomSweeneyRedHat, @mrunalp, @nalind, @rhatdan, @smarterclayton Would not like to push you, but would be very happy about a quick review and approval. Thanks, Josef

[rhatdan]

What are the defaults right now.

Default -> Ownership is root or ownership is the current User? --chown USER -> Ownership is USER --keep-ownership -> Current Owner UID of source?

[Dexus]

What are the defaults right now.

Default -> Ownership is root or ownership is the current User?

Default is currently that the file get root:root

--chown USER -> Ownership is USER --keep-ownership -> Current Owner UID of source?

--keep-ownership keeps the same UID:GID of the source.

Added the current state in the first comment.

[rhatdan]

LGTM @nalind PTAL

[nalind]

I think this flag would be buildah only but it will also get parsed for dockerclient as well but would not be recognized by docker daemon, maybe we should just drop this fmt.Errorf from imagebuilder and let implementations take care of it. @nalind WDYT ?

Yes, dockerclient would either need to implement it or flag it as an error, to avoid a problem where it just starts silently ignoring a new flag that it doesn't implement, one which it previously flagged as an error when people tried to use it.

[Dexus]

I think this flag would be buildah only but it will also get parsed for dockerclient as well but would not be recognized by docker daemon, maybe we should just drop this fmt.Errorf from imagebuilder and let implementations take care of it. @nalind WDYT ?

Yes, dockerclient would either need to implement it or flag it as an error, to avoid a problem where it just starts silently ignoring a new flag that it doesn't implement, one which it previously flagged as an error when people tried to use it.

@nalind @flouthoc if we speak from dockerclient we talking ~~not~~ from the directory in this repo right? ~~You mean github.com/fsouza/go-dockerclient - is it correct?~~ I'm maybe not right up, but I'm not see where I should add it for dockerclient

[nalind]

@nalind @flouthoc if we speak from dockerclient we talking ~not~ from the directory in this repo right? ~You mean github.com/fsouza/go-dockerclient - is it correct?~ I'm maybe not right up, but I'm not see where I should add it for dockerclient

Sorry for being unintentionally vague there - by dockerclient I mean the dockerclient package in this repository, which provides an executor for the builder logic by calling into go-dockerclient. That separation, between the parser, the builder logic that drives the build, and the executor that it uses, made imagebuilder a natural choice for adding Dockerfile support to buildah.

[nalind]

This doesn't seem to be an option that docker build supports in its current iteration, and we've historically shied away from implementing syntax features that docker build doesn't implement because we don't want to "fork" the format. Has this been proposed there?

[Dexus]

This doesn't seem to be an option that docker build supports in its current iteration, and we've historically shied away from implementing syntax features that docker build doesn't implement because we don't want to "fork" the format. Has this been proposed there?

No, that was not proposed there. But I will do this. Not sure where exactly is the right place for it. Moby?

[rhatdan]

Yes Moby. But if this is a change to Dockerfile/Containerfile format, then they will probably send you to buildkit.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten /remove-lifecycle stale

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen. Mark the issue as fresh by commenting /remove-lifecycle rotten. Exclude this issue from closing again by commenting /lifecycle frozen.

/close

[openshift-ci[bot]]

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen. Mark the issue as fresh by commenting /remove-lifecycle rotten. Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.