Add docs for Dockerfile ADD/COPY --chown flag #467

estesp · 2017-08-23T16:25:01Z

Document the new --chown flag added to the ADD and COPY commands in
the Dockerfile format.

Signed-off-by: Phil Estes estesp@linux.vnet.ibm.com

The actual implementation is ready to merge in moby/moby#34263

codecov-io · 2017-08-23T16:29:22Z

Codecov Report

Merging #467 into master will decrease coverage by 2.67%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
- Coverage   49.41%   46.74%   -2.68%     
==========================================
  Files         208      199       -9     
  Lines       17186    16526     -660     
==========================================
- Hits         8493     7725     -768     
- Misses       8261     8400     +139     
+ Partials      432      401      -31

mdlinville

This review is for the docs only, not the merit of the feature. I found a couple places where there was room for improvement. My comments are about ADD but they also apply to COPY.

mdlinville · 2017-08-23T17:56:11Z

docs/reference/builder.md

 whitespace)

 The `ADD` instruction copies new files, directories or remote file URLs from `<src>`
 and adds them to the filesystem of the image at the path `<dest>`.

-Multiple `<src>` resource may be specified but if they are files or
+Multiple `<src>` resources may be specified but if they are files or
 directories then they must be relative to the source directory that is


directories, they must be

Also, "relative" I think means that they must be subdirectories of the build context? Maybe you can reword this to make it clearer.

mdlinville · 2017-08-23T17:58:50Z

docs/reference/builder.md

-All new files and directories are created with a UID and GID of 0.
+All new files and directories are created with a UID and GID of 0, unless the
+optional `--chown` flag is used to provide a user and group definition which
+will be used to change the ownership of the created files and directories. The


How about "optional --chown flag specifies a given username, groupname, or UID/GID combination."

(if you only want to give a GID, how does Docker know it's not a UID? I think if you only provide a single integer it's always assumed to be a UID. Maybe best to state this too.)

mdlinville · 2017-08-23T17:59:54Z

cc/ @gbarr01 for FYI

shouze · 2017-09-08T08:17:45Z

docs/reference/builder.md

@@ -817,14 +817,14 @@ change them using `docker run --env <key>=<value>`.

 ADD has two forms:

- `ADD <src>... <dest>`
- `ADD ["<src>",... "<dest>"]` (this form is required for paths containing
+- `ADD [--chown <user>:<group>] <src>... <dest>`


Sure about that syntax? Not working for me & @presto9292 see moby/moby#34263 (comment)

Thanks @shouze ; you are correct that it requires an =; I will update this PR as it also has other edits required before being complete.

estesp · 2017-09-08T17:33:45Z

@mstanleyjones thanks for the review; I have updated per your comments; PTAL!

@shouze examples and flag documentation updated with the correct argument style--thanks for the catch!

rcjsuen · 2017-09-09T05:35:09Z

What is the behaviour of using this flag on Windows as the /etc/passwd and /etc/group files do not exist there? Should this be included in the documentation?

mdlinville · 2017-09-11T18:33:21Z

docs/reference/builder.md

@@ -848,7 +848,26 @@ named `arr[0].txt`, use the following;
    ADD arr[[]0].txt /mydir/    # copy a file named "arr[0].txt" to /mydir/


-All new files and directories are created with a UID and GID of 0.
+All new files and directories are created with a UID and GID of 0, unless the


How do the concepts of UID and GID 0 map to Windows?

They do not apply/map at all.. I assume we need a note about that :) A Dockerfile with the --chown flag on an ADD or COPY command will fail during docker build if it is built on a Windows Docker daemon.

This was discussed in the PR for the feature implementation itself--the other option was to "ignore" the flag on Windows but today this is not the way it is implemented.

Perhaps we should add a header above this section; both for easier linking (anchor), and an introduction "This is a Linux-only feature". Also, the sections for COPY and ADD for this feature could then be combined if we think that's a good idea ("All new files and directories that are added through ADD or COPY ...."). (I'm sure @mstanleyjones has better suggestions 😄)

What is actually the owner of the files on Windows? (@johnstep?)

@estesp What I've seen is that passing --chown succeeds on Windows, if a number is supplied, but fails for a name since the code attempts to resolve the name by looking in \etc\passwd on Windows, which does not make sense.

Failing outright probably makes more sense, or at least warning, but I would prefer a more deliberate check and message. We have an issue on adding a warning, but it needs some more details: moby/moby#35026

@thaJeztah We'll look into how the owner is set on Windows. From a quick test, it seems to be set differently based on which directory a file is copied to, but we need to verify.

mdlinville · 2017-09-11T18:33:36Z

docs/reference/builder.md

+or direct integer UID and GID in any combination. Providing a username without
+groupname or a UID without GID will use the name numeric UID as the GID. If a
+username or groupname is provided, the container's root filesystem
+`/etc/passwd` and `/etc/group` files will be used to perform the translation


How does this map to Windows?

thaJeztah

Left some thoughts 😄

thaJeztah · 2017-09-12T12:52:16Z

docs/reference/builder.md

+combination to request specific ownership of the content added. The
+format of the `--chown` flag allows for either username and groupname strings
+or direct integer UID and GID in any combination. Providing a username without
+groupname or a UID without GID will use the name numeric UID as the GID. If a


s/name/same/ (probably)

thaJeztah · 2017-09-12T12:54:57Z

docs/reference/builder.md

@@ -848,7 +848,26 @@ named `arr[0].txt`, use the following;
    ADD arr[[]0].txt /mydir/    # copy a file named "arr[0].txt" to /mydir/


-All new files and directories are created with a UID and GID of 0.
+All new files and directories are created with a UID and GID of 0, unless the


Perhaps we should add a header above this section; both for easier linking (anchor), and an introduction "This is a Linux-only feature". Also, the sections for COPY and ADD for this feature could then be combined if we think that's a good idea ("All new files and directories that are added through ADD or COPY ...."). (I'm sure @mstanleyjones has better suggestions 😄)

What is actually the owner of the files on Windows? (@johnstep?)

thaJeztah · 2017-09-12T12:57:49Z

docs/reference/builder.md

+combination to request specific ownership of the copied content. The
+format of the `--chown` flag allows for either username and groupname strings
+or direct integer UID and GID in any combination. Providing a username without
+groupname or a UID without GID will use the name numeric UID as the GID. If a


s/name/same/

mdlinville · 2017-09-12T21:53:23Z

I like the idea of a note like "The --chown and --chgrp features are only supported on Dockerfiles for Linux containers, and will not work on Windows containers." Maybe you can elaborate on what "will not work" looks like.

thaJeztah · 2017-09-29T21:12:25Z

ping @estesp 😇

Document the new `--chown` flag added to the ADD and COPY commands in the Dockerfile format. Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>

estesp · 2017-10-05T16:37:24Z

@mstanleyjones @thaJeztah updated; PTAL.

I did not combine the sections yet; I'm definitely willing to do so, but currently the anchor points are to each possible command in a Dockerfile; so I don't know what to name that anchor. Is it "ADD, COPY" or "ADD and COPY" or "ADD/COPY" ? :)

Given they are still distinct I added the same exact note about lack of capability on Windows to use the --chown feature.

thaJeztah

LGTM

thaJeztah · 2017-10-05T17:03:39Z

docs/reference/builder.md

- `COPY <src>... <dest>`
- `COPY ["<src>",... "<dest>"]` (this form is required for paths containing
+- `COPY [--chown=<user>:<group>] <src>... <dest>`
+- `COPY [--chown=<user>:<group>] ["<src>",... "<dest>"]` (this form is required for paths containing


lol, this will be a bit confusing as the first square brackets are to indicate "it's optional" and the second are literal brackets for JSON.

Don't have a better suggestion at this point though (other than having an example perhaps) - I'm okay with adding that in a follow up

estesp · 2017-10-25T20:40:58Z

This feature went into the 17.09 release, so it currently undocumented as far as online docs. Is there anything else not addressed here? Just don't want it to get lost for too long!

karser · 2017-10-25T20:50:30Z

@estesp keep track changelog on docker-ce releases page to stay current

mdlinville · 2017-10-25T22:36:48Z

@estesp well, this PR would have needed to be merged before docker/cli was merged into the appropriate docker/docker-ce branch. The docs currently pull the builder.md from https://github.com/docker/docker-ce/blob/17.09/components/cli/docs/reference/builder.md.

rcjsuen · 2017-10-25T22:57:40Z

Unfortunately, this didn't make 17.10 either as far as I can tell. That's too bad but hey, it happens.

What outstanding issues are there left that are preventing it from being merged? I would like to add support for this flag in my project once the documentation has been finalized and is made available online.

vdemeester

LGTM 🐸

thaJeztah · 2017-11-29T17:28:27Z

I'll go ahead and merge 👍 thanks!

estesp requested review from mdlinville, thaJeztah and vdemeester as code owners August 23, 2017 16:25

GordonTheTurtle added the status/0-triage label Aug 23, 2017

mdlinville suggested changes Aug 23, 2017

View reviewed changes

vdemeester added status/3-docs-review and removed status/0-triage labels Aug 25, 2017

shouze reviewed Sep 8, 2017

View reviewed changes

shouze mentioned this pull request Sep 8, 2017

Add --chown flag to ADD/COPY commands moby/moby#34263

Merged

estesp force-pushed the chown-flag-docs branch from 95e0972 to 8b7a8b0 Compare September 8, 2017 17:31

shouze approved these changes Sep 8, 2017

View reviewed changes

mdlinville reviewed Sep 11, 2017

View reviewed changes

thaJeztah requested changes Sep 12, 2017

View reviewed changes

This was referenced Oct 2, 2017

Add support for --chown flag in ADD and COPY rcjsuen/dockerfile-language-server#166

Closed

Incorporate --chown flag documentation into the language server rcjsuen/dockerfile-language-server#181

Closed

Add docs for Dockerfile ADD/COPY --chown flag

a83b9f1

Document the new `--chown` flag added to the ADD and COPY commands in the Dockerfile format. Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>

estesp force-pushed the chown-flag-docs branch from 8b7a8b0 to a83b9f1 Compare October 5, 2017 16:35

thaJeztah approved these changes Oct 5, 2017

View reviewed changes

kaczmarj mentioned this pull request Nov 6, 2017

ENH: generate Dockerfiles with neurodocker + migrate to CircleCI 2.0 nipy/nipype#2202

Merged

7 tasks

thaJeztah added the process/cherry-pick label Nov 17, 2017

vdemeester approved these changes Nov 27, 2017

View reviewed changes

thaJeztah merged commit 12765c3 into docker:master Nov 29, 2017

GordonTheTurtle added this to the 17.12.0 milestone Nov 29, 2017

This was referenced Dec 13, 2017

add --chown flag to docs for COPY and ADD #738

Closed

add --chown flag to docs for COPY and ADD docker/docs#5441

Closed

thaJeztah removed process/cherry-pick labels Jul 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docs for Dockerfile ADD/COPY --chown flag #467

Add docs for Dockerfile ADD/COPY --chown flag #467

estesp commented Aug 23, 2017

codecov-io commented Aug 23, 2017 •

edited

Loading

mdlinville left a comment

mdlinville Aug 23, 2017

mdlinville Aug 23, 2017

mdlinville commented Aug 23, 2017

shouze Sep 8, 2017

estesp Sep 8, 2017

estesp commented Sep 8, 2017

rcjsuen commented Sep 9, 2017

mdlinville Sep 11, 2017

estesp Sep 11, 2017

thaJeztah Sep 12, 2017

johnstep Oct 5, 2017

mdlinville Sep 11, 2017

thaJeztah left a comment

thaJeztah Sep 12, 2017

thaJeztah Sep 12, 2017

thaJeztah Sep 12, 2017

mdlinville commented Sep 12, 2017

thaJeztah commented Sep 29, 2017

estesp commented Oct 5, 2017

thaJeztah left a comment

thaJeztah Oct 5, 2017

estesp commented Oct 25, 2017

karser commented Oct 25, 2017

mdlinville commented Oct 25, 2017

rcjsuen commented Oct 25, 2017

vdemeester left a comment

thaJeztah commented Nov 29, 2017

Add docs for Dockerfile ADD/COPY --chown flag #467

Add docs for Dockerfile ADD/COPY --chown flag #467

Conversation

estesp commented Aug 23, 2017

codecov-io commented Aug 23, 2017 • edited Loading

Codecov Report

mdlinville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdlinville commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estesp commented Sep 8, 2017

rcjsuen commented Sep 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdlinville commented Sep 12, 2017

thaJeztah commented Sep 29, 2017

estesp commented Oct 5, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estesp commented Oct 25, 2017

karser commented Oct 25, 2017

mdlinville commented Oct 25, 2017

rcjsuen commented Oct 25, 2017

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 29, 2017

codecov-io commented Aug 23, 2017 •

edited

Loading