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

docs: restructure descriptions for add, copy, run flags #4688

Merged
merged 1 commit into from
Feb 28, 2024
Merged

docs: restructure descriptions for add, copy, run flags #4688

merged 1 commit into from
Feb 28, 2024

Conversation

dvdksn
Copy link
Collaborator

@dvdksn dvdksn commented Feb 24, 2024 •
edited
Loading

Restructure information about ADD/COPY flags,
describing flags/options in dedicated sections.

Also made some changes to RUN for sake of uniformity

Follow-up to #4561

Previews:

@dvdksn dvdksn force-pushed the docs-dockerfile-addcopy-flags-refactor branch 2 times, most recently from bb52ba7 to 0189bd0 Compare February 24, 2024 15:11
@dvdksn dvdksn changed the title docs: restructure add/copy flags docs: restructure descriptions for add, copy, run flags Feb 24, 2024
@dvdksn dvdksn mentioned this pull request Feb 24, 2024
Closed
dvdksn
dvdksn commented Feb 24, 2024
View reviewed changes
frontend/dockerfile/docs/reference.md Outdated
Comment on lines 647 to 658
## RUN --mount
### RUN --mount

> **Note**
>
> Added in [`docker/dockerfile:1.2`](#syntax)

```dockerfile
RUN --mount=[type=<TYPE>][,option=<value>[,option=<value>]...]
```

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to reviewer: These changes for RUN flags are just to keep things consistent between the different commands

frontend/dockerfile/docs/reference.md
Comment on lines -1141 to +1146
ADD [--chown=<user>:<group>] [--chmod=<perms> [--exclude=<exclude>]... [--checksum=<checksum>] <src>... <dest>
ADD [--chown=<user>:<group>] [--chmod=<perms> [--exclude=<exclude>]... ["<src>",... "<dest>"]
ADD [OPTIONS] <src> ... <dest>
ADD [OPTIONS] ["<src>", ... "<dest>"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to reviewer: IMO this instruction has too many flags now to show them as a one-liner, so I moved all the flags to a list below the synopsis, with links

frontend/dockerfile/docs/reference.md
@@ -1316,79 +1286,87 @@ doesn't support authentication.
- If `<dest>` doesn't exist, it's created, along with all missing directories
in its path.

### Verifying a remote file checksum `ADD --checksum=<checksum> <http src> <dest>`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to reviewer: I renamed this section (and the "Add a Git repository" one) to ADD --flag to keep consistency with the others

frontend/dockerfile/docs/reference.md Outdated Show resolved Hide resolved
frontend/dockerfile/docs/reference.md Show resolved Hide resolved
@dvdksn dvdksn force-pushed the docs-dockerfile-addcopy-flags-refactor branch from 0189bd0 to 30d3516 Compare February 24, 2024 15:36
@dvdksn dvdksn force-pushed the docs-dockerfile-addcopy-flags-refactor branch from 30d3516 to 3a32318 Compare February 24, 2024 19:14
@dvdksn dvdksn requested a review from crazy-max February 24, 2024 19:14
@tonistiigi tonistiigi added this to the v0.13.0 milestone Feb 26, 2024
tonistiigi
tonistiigi reviewed Feb 27, 2024
View reviewed changes
frontend/dockerfile/docs/reference.md Outdated
translating user and group names to IDs restricts this feature to only be viable for
Linux OS-based containers.

All new files and directories are created with a UID and GID of 0, unless the
Copy link
Member

Choose a reason for hiding this comment

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

This is only true for files copied from the build context. Otherwise, files keep their original uid/gid. Also, this conversion happens in build context transfer so is not COPY-specific. Eg. if mounts are used then it also build context files have already become uid/gid=0 before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. Since this isn't new or changed in this PR (I just moved it into a section) I think we can do a follow-up. I think we can call this out on the context page and link to there. https://docs.docker.com/build/building/context/

frontend/dockerfile/docs/reference.md
> **Note**
>
> Added in [`docker/dockerfile:1.1`](#syntax)
```dockerfile
Copy link
Member

Choose a reason for hiding this comment

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

Why the removal of the minimum versions for the older flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

References to very old versions (e.g. 1.1) were kinda noisy I thought, and at some point we'd probably want a cleanup, for example I think we might still carry references to Docker 17.x in some places. But maybe n-2 from stable is too aggressive.

With your other suggestion of adding it in the list it's less intrusive, so let's try that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @crazy-max wdyt about version callout for these flags? See also Tonis' other comment https://github.com/moby/buildkit/pull/4688/files#r1503506056

I wonder if we should have a table somewhere summarizing the new syntax features and when they were introduced (or what their status is, if they're still in labs for example)

frontend/dockerfile/docs/reference.md
>
> The `--exclude` option can be specified multiple times and cause files matching its patterns not to be copied,
> even if the files paths match the pattern specified in `<src>`. This feature requires the build tag `dfexcludepatterns`.
- [`--keep-git-dir`](#add---keep-git-dir)
Copy link
Member

Choose a reason for hiding this comment

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

In this list of flags for ADD/COPY, would it make sense to put "(since vX.X)" behind them so it is clear when they can be used.

@dvdksn dvdksn force-pushed the docs-dockerfile-addcopy-flags-refactor branch from 3a32318 to 9af155a Compare February 28, 2024 09:55
tonistiigi
tonistiigi approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Let's leave to possible versioning changes to follow-up then.

@tonistiigi tonistiigi merged commit aae0abb into moby:master Feb 28, 2024
15 checks passed
@dvdksn dvdksn deleted the docs-dockerfile-addcopy-flags-refactor branch February 29, 2024 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@crazy-max crazy-max Awaiting requested review from crazy-max

Assignees
No one assigned
Labels
area/dockerfile kind/docs
Projects
None yet
Milestone
v0.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@dvdksn @tonistiigi @crazy-max