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.

Sign up for GitHub

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

Jump to bottom

Support COPY --chown flag #962

Merged
merged 5 commits into from
Feb 4, 2020
Merged

Support COPY --chown flag #962

merged 5 commits into from
Feb 4, 2020

Conversation

xanonid
Copy link
Contributor

@xanonid xanonid commented Jan 10, 2020 •
edited by tejal29
Loading

This PR fixes #9, #550, #579

Description

This PR adds the --chown flag to kaniko. This allows to build e.g. root-less containers more easily and avoids unnecessary additional layers from manual chown calls.

The PR does not yet contain integration tests as the container-diff cannot yet check file ownership (GoogleContainerTools/container-diff#308).

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

kaniko supports now the --chown flag for the COPY command.

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Jan 10, 2020
@xanonid xanonid force-pushed the support_copy_with_chown branch from 5c42e63 to d1e6c96 Compare January 10, 2020 21:58
tejal29
tejal29 reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@tejal29 tejal29 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@xanonid Thank you for your contribution.
The change looks really good. Can you please add some unit tests especially for the new function GetUIDAndGIDFromString ?

Thanks
Tejal

@tejal29
Copy link
Contributor

tejal29 commented Jan 14, 2020

@xanonid Can you explain more on "This allows to build e.g. root-less containers more easily and avoids unnecessary additional layers from manual chown calls." ?

cvgw
cvgw reviewed Jan 17, 2020
View reviewed changes
cmd/executor/cmd/root.go Outdated
@@ -200,7 +200,7 @@ func resolveDockerfilePath() error {
// copy Dockerfile to /kaniko/Dockerfile so that if it's specified in the .dockerignore
// it won't be copied into the image
func copyDockerfile() error {
if _, err := util.CopyFile(opts.DockerfilePath, constants.DockerfilePath, ""); err != nil {
if _, err := util.CopyFile(opts.DockerfilePath, constants.DockerfilePath, "", -1, -1); err != nil {
Copy link
Contributor

@cvgw cvgw Jan 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

lets make the two integers into constants so that their meaning is more clear to readers

cvgw
cvgw reviewed Jan 17, 2020
View reviewed changes
pkg/util/fs_util.go Outdated Show resolved Hide resolved
cvgw
cvgw reviewed Jan 17, 2020
View reviewed changes
pkg/util/fs_util.go Outdated Show resolved Hide resolved
@xanonid xanonid force-pushed the support_copy_with_chown branch 3 times, most recently from 34d3ca8 to 2439a1a Compare January 24, 2020 16:20
@tejal29
Copy link
Contributor

tejal29 commented Jan 29, 2020

Thanks @xanonid Looks like there is conflict due to a my PR that went in before. I will try to pick this up and get it in this release.

@tejal29 tejal29 force-pushed the support_copy_with_chown branch from 2439a1a to 91dfa48 Compare January 31, 2020 22:15
@tejal29 tejal29 mentioned this pull request Feb 3, 2020
Closed
@tejal29 tejal29 force-pushed the support_copy_with_chown branch from 91dfa48 to 0c6f6b7 Compare February 3, 2020 23:23
@tejal29 tejal29 mentioned this pull request Feb 3, 2020
Closed
@tejal29 tejal29 force-pushed the support_copy_with_chown branch from 0c6f6b7 to d500962 Compare February 4, 2020 02:34
@tejal29 tejal29 force-pushed the support_copy_with_chown branch from d500962 to 31f626c Compare February 4, 2020 06:16
@tejal29 tejal29 merged commit bd59b60 into GoogleContainerTools:master Feb 4, 2020
@tejal29 tejal29 mentioned this pull request Feb 5, 2020
Merged
@anton-johansson
Copy link

I just bumped into this issue and was very happy to find this PR, being merged and all, nice work! However, it made it to master just after the latest release - 0.17.1 was created. Any plans of creating 0.18.0 or maybe even 0.17.2 anytime soon?

I will use a latest version for now, but I'll be more happy to use a release soon. :)

@xanonid
Copy link
Contributor Author

xanonid commented Mar 9, 2020

@anton-johansson @tejal29 0.18.0 was released some days ago. Unfortunately, the change by this PR is not mentioned in the release notes.

@MaesterZ
Copy link

MaesterZ commented Mar 9, 2020 •
edited
Loading

If this takes too long to release, I don't mind creating a quick PR to mention in documentation that --chown flag is not supported on ADD and COPY instructions. It's kind of a limitation?

@xanonid
Copy link
Contributor Author

xanonid commented Mar 9, 2020

@Misteur-Z This PR is merged and released, so chown is supported for COPY now, but not yet for ADD.

@rgschmitz1 rgschmitz1 mentioned this pull request May 3, 2022
Closed
3 tasks
@JeromeJu JeromeJu mentioned this pull request Feb 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johannsoden johannsoden johannsoden left review comments

@cvgw cvgw cvgw left review comments

@tejal29 tejal29 tejal29 left review comments

Assignees
No one assigned
Labels
candidate-release cla: yes CLA signed by all commit authors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for --chown flag to COPY command
8 participants
@xanonid @tejal29 @anton-johansson @MaesterZ @cvgw @johannsoden @googlebot @kokoro-team