`ADD|COPY` option `--keep-ownership` by Dexus · Pull Request #4001 · containers/buildah

Dexus · 2022-05-17T17:26:26Z

This is the follow-up PR that includes the initial PR from #3767 and is now fulfilled with additional changes, PRs (openshift/imagebuilder/pull/227) and tests.

Please follow the initial conversations in #3767.

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.

Closes #3767

⚠️ Notice:

go.mod needs to add a replace for github.com/openshift/imagebuilder as long openshift/imagebuilder/pull/227 is unmerged

go mod edit -replace github.com/openshift/imagebuilder=github.com/Dexus-Forks/imagebuilder

Dexus · 2022-05-17T17:38:45Z

/assign @flouthoc
/assign @rhatdan

Dexus · 2022-05-17T17:40:05Z

/assign @lsm5

openshift-ci · 2022-05-17T17:55:29Z

[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 flouthoc after the PR has been reviewed.

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

Details

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

edsantiago

Thanks for your PR! One showstopper in the tests, one strong encouragement to rework another.

flouthoc · 2022-05-18T07:31:37Z

@Dexus Commit title for dc90aed5127822f6b210bf2ed0e2742e98a5e797 is too long could you move PR link in commit title to description please and also please sign the commits in-order to make DCO check happy.

Dexus · 2022-05-18T07:42:36Z

@Dexus Commit title for dc90aed5127822f6b210bf2ed0e2742e98a5e797 is too long could you move PR link in commit title to description please and also please sign the commits in-order to make DCO check happy.

I'm not abel to make DOC happy because the first commit is not from me, I took the first commit from the linked initial PR, from Ruben Jenster r.jenster@drachenfels.de, I'm not sure how can I fix this. Never used the Signed-off-by feature before.

The too long title for the one commit, will I change once I'm back on Place.

Dexus · 2022-05-18T15:26:07Z

So now all tests are green! 🥳
So, what happens next?

This exposes the already existing KeepOwnership flag to the copy and add CLI commands. Co-authored-by: Ruben Jenster <r.jenster@drachenfels.de> Signed-off-by: Josef Fröhle <github@josef-froehle.de>

allow to keep the owner uid/gid off added/copied local files Signed-off-by: Josef Fröhle <github@josef-froehle.de>

Dexus · 2022-05-18T16:56:16Z

fresh rebased

flouthoc · 2022-05-19T04:59:11Z

So now all tests are green! partying_face So, what happens next?

Now we have to wait for this first: openshift/imagebuilder#227 once imagebuilder PR is merged you can rebase this PR and point to upstream imagebuilder.

TomSweeneyRedHat · 2022-05-19T20:51:38Z


 replace github.com/opencontainers/image-spec => github.com/opencontainers/image-spec v1.0.2-0.20211123152302-43a7dee1ec31
+
+replace github.com/openshift/imagebuilder => github.com/Dexus-Forks/imagebuilder v1.2.4-0.20220518131701-b5c6586df000


We'll need to fix this up once the imagebuilder merges and before we merge this PR.

TomSweeneyRedHat · 2022-05-19T20:53:43Z

One nit request. If you could rename the test Dockerfile* files to Containerfile*, that would be appreciated.

nalind · 2022-05-19T21:04:26Z

+FROM busybox AS basis
+RUN echo hello > /newfile && chown 123:456 /newfile
+FROM basis
+COPY --keep-ownership --from=basis /newfile /newfile_123_456


The --keep-ownership option is redundant when there's a --from, so I don't know that we need this test.

you are right

openshift-ci · 2022-06-10T16:31:41Z

@Dexus: PR needs rebase.

Details

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.

github-actions · 2022-07-11T00:10:25Z

A friendly reminder that this PR had no activity for 30 days.

flouthoc · 2022-07-11T11:36:54Z

Correct me if i am wrong i think this is on hold because of moby/buildkit#2875 since dockerclient does not implements this yet and imagebuilder would end up with no-op for docker implementation.

github-actions · 2022-11-16T00:12:12Z

A friendly reminder that this PR had no activity for 30 days.

Dexus force-pushed the copy-keep-ownership branch 3 times, most recently from abebc1b to 4e161be Compare May 17, 2022 17:37

openshift-ci Bot assigned flouthoc and rhatdan May 17, 2022

openshift-ci Bot assigned lsm5 May 17, 2022

edsantiago suggested changes May 17, 2022

View reviewed changes

Comment thread tests/bud.bats Outdated

Comment thread tests/bud.bats Outdated

flouthoc reviewed May 18, 2022

View reviewed changes

Comment thread imagebuildah/stage_executor.go Outdated

flouthoc reviewed May 18, 2022

View reviewed changes

Comment thread imagebuildah/stage_executor.go Outdated

Dexus force-pushed the copy-keep-ownership branch 2 times, most recently from d060a16 to 99607c2 Compare May 18, 2022 08:08

Dexus requested a review from edsantiago May 18, 2022 08:11

Dexus force-pushed the copy-keep-ownership branch 3 times, most recently from 6231a0c to 29828b3 Compare May 18, 2022 14:10

edsantiago added the do-not-merge/work-in-progress label May 18, 2022

Dexus requested a review from flouthoc May 18, 2022 15:26

Dexus force-pushed the copy-keep-ownership branch from 29828b3 to f990ef0 Compare May 18, 2022 16:50

Dexus and others added 2 commits May 18, 2022 18:54

copy: add --keep-ownership flag

b72f266

This exposes the already existing KeepOwnership flag to the copy and add CLI commands. Co-authored-by: Ruben Jenster <r.jenster@drachenfels.de> Signed-off-by: Josef Fröhle <github@josef-froehle.de>

add/copy add --keep-ownership flag

ed44bea

allow to keep the owner uid/gid off added/copied local files Signed-off-by: Josef Fröhle <github@josef-froehle.de>

Dexus force-pushed the copy-keep-ownership branch from f990ef0 to ed44bea Compare May 18, 2022 16:54

Dexus marked this pull request as draft May 18, 2022 16:55

Dexus marked this pull request as ready for review May 18, 2022 16:56

openshift-ci Bot removed the do-not-merge/work-in-progress label May 18, 2022

Dexus mentioned this pull request May 18, 2022

Add option --keep-ownership to ADD/COPY openshift/imagebuilder#227

Closed

r10r mentioned this pull request May 18, 2022

Add --keep-ownership option to add/copy commands to keep file ownership. #3767

Closed

TomSweeneyRedHat reviewed May 19, 2022

View reviewed changes

nalind reviewed May 19, 2022

View reviewed changes

Dexus mentioned this pull request May 23, 2022

[feature request/propsal] add option --keep-ownership for ADD and COPY commands moby/buildkit#2875

Open

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022

github-actions Bot added the stale-pr label Jul 11, 2022

github-actions Bot removed the stale-pr label Oct 15, 2022

github-actions Bot added the stale-pr label Nov 16, 2022

Dexus closed this Nov 16, 2022

github-actions Bot added the locked - please file new issue/PR label Sep 2, 2023

github-actions Bot locked as resolved and limited conversation to collaborators Sep 2, 2023


		replace github.com/opencontainers/image-spec => github.com/opencontainers/image-spec v1.0.2-0.20211123152302-43a7dee1ec31

		replace github.com/openshift/imagebuilder => github.com/Dexus-Forks/imagebuilder v1.2.4-0.20220518131701-b5c6586df000

Conversation

Dexus commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dexus commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dexus commented May 17, 2022

Uh oh!

openshift-ci Bot commented May 17, 2022

Uh oh!

edsantiago left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flouthoc commented May 18, 2022

Uh oh!

Dexus commented May 18, 2022

Uh oh!

Dexus commented May 18, 2022

Uh oh!

Dexus commented May 18, 2022

Uh oh!

flouthoc commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomSweeneyRedHat May 19, 2022

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented May 19, 2022

Uh oh!

nalind May 19, 2022

Choose a reason for hiding this comment

Uh oh!

Dexus May 20, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jun 10, 2022

Uh oh!

github-actions Bot commented Jul 11, 2022

Uh oh!

flouthoc commented Jul 11, 2022

Uh oh!

github-actions Bot commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Dexus commented May 17, 2022 •

edited

Loading

Dexus commented May 17, 2022 •

edited

Loading

flouthoc commented May 19, 2022 •

edited

Loading