dockerfile (labs): implement `ADD <git ref>` by AkihiroSuda · Pull Request #2799 · moby/buildkit

AkihiroSuda · 2022-04-12T08:29:59Z

e.g.,

# syntax=docker/dockerfile-upstream:master-labs
# ↑ Will be included in `syntax=docker/dockerfile:1.5-labs`
FROM alpine
ADD --keep-git-dir=true https://github.com/moby/buildkit.git#v0.10.1 /buildkit

Close #775

Expected to be used with:

llbsolver: support pinning sources (same as the "Dockerfile.pin" proposal but agnostic to frontends) #2943

tonistiigi

@AkihiroSuda Is this supposed to be still draft?

util/gitutil/git_ref_test.go

frontend/dockerfile/docs/syntax.md

util/gitutil/git_ref_test.go

thaJeztah · 2022-04-21T13:11:28Z

frontend/dockerfile/docs/syntax.md

+```dockerfile
+# syntax=docker/dockerfile:1.5
+FROM alpine
+ADD https://github.com/moby/buildkit.git#v0.10.1 /buildkit


Perhaps a silly question; how does this relate to the named contexts that were added in docker/buildx#904 / docker/buildx#965 ? Mostly curious if some of these would make sense to be implemented as a "context".

I think the named contexts is a separate topic.
ADD instructions would be easier to use for typical users.

thaJeztah · 2022-04-21T13:12:19Z

Somewhat orthogonal to this PR; I recently moved some code around in moby/moby, and improved some of the docs of the urlutil package;
https://github.com/moby/moby/blob/c5f18aac9f27ad88166ddfbec7822a2f1c6f0c20/builder/remotecontext/urlutil/urlutil.go#L24-L78

When working on those changes, I noticed that the "moby" variant (used by the classic builder) is hardcoded to master, and when I started implementing that, I saw that BuildKit had exactly the code I was about to write 😂.

If possible, I think we should try to unify those packages (moby/moby consuming it from BuildKit), however (need to look again to double-check) the BuildKit code was in a package that also imported llb, which made it difficult to consume it for the classic builder, but wondering if it would be possible to somehow split some of the code to make it consumable without that dependency.

tonistiigi · 2022-04-22T04:59:13Z

frontend/dockerfile/dockerfile2llb/convert.go

+			if err != nil {
+				return errors.Wrapf(err, "failed to parse a git ref %q", src)
+			}
+			st := llb.Git(g.Repo, g.Commit, llb.KeepGitDir())


KeepGitDir() default correct in here? On build context it is not default? I guess we could use a build-arg for this as well?

I guess we could use a build-arg for this as well?

Should this be rather an flag for ADD instruction? e.g., ADD --keep-git-dir=(true|false) ....

I'm not sure. Don't like to add lots of very specific flags either.

While the git dir is not preserved for build contexts, I'd prefer to keep the git dir for the ADD instruction, as Makefiles often need git rev-parse, git describe, etc.

https://github.com/containerd/containerd/blob/8e74f2af0096a046ac004dc954105b010b9ee2d5/Makefile#L33

Could we still do something for this that would allow have same defaults as context but opt-in to keep-git-dir. Maybe a convention on the URL, or a ARG definition before the command that controls this(or maybe a flag is fine)?

Not a huge fan of complicating the URL microformat.
I also fear that adding an ARG like ARG KEEPGITDIR=true may conflict with user-defined ARG arguments.

Maybe ADD --keep-git-dir=(true|false) ... is the best?

@tonistiigi SGTY? ↑

I think it is fine yes. In addition to plain keep-git we might need a way to configure the depth size as well in the future.

Added --keep-git-dir. Defaults to false.

tonistiigi · 2022-04-22T05:02:56Z

frontend/dockerfile/dockerfile2llb/convert.go

+				CreateDestPath: true,
+			}}, copyOpt...)
+			if a == nil {
+				a = llb.Copy(st, "/", dest, opts...)


We should make sure that

FROM scratch ADD git:// / (or .)

Is converted to direct llb.Git(), not llb.Scratch().File(llb.Copy(llb.Git(), "/", "/")) . Possibly this could be also optimized in the llb libarary.

Do we need to work on this in this PR or a separate PR?

Would be better in this PR. Separate commit is ok. If you want to do this in llb client level then it can be done in parallel on separate PR but we should avoid the possibility of having the git feature but not this optimization merged.

Do we have that optimization for http sources?

Added an optimizer in the second commit

frontend/dockerfile/dockerfile_test.go

frontend/dockerfile/docs/syntax.md

frontend/dockerfile/builder/build.go

tonistiigi · 2022-05-09T22:15:41Z

frontend/dockerfile/dockerfile2llb/convert.go

+				CreateDestPath: true,
+			}}, copyOpt...)
+			if a == nil {
+				a = llb.Copy(st, "/", dest, opts...)


Would be better in this PR. Separate commit is ok. If you want to do this in llb client level then it can be done in parallel on separate PR but we should avoid the possibility of having the git feature but not this optimization merged.

AkihiroSuda · 2022-07-21T15:59:05Z

Rebased

AkihiroSuda · 2022-07-27T05:23:18Z

@tonistiigi PTAL 🙏

tonistiigi

Sorry for getting this PR stuck in the fileop+scratch optimization, but I don't like the FileOptimize public function at all. Split the second commit to another PR so we can get the main part merged.

For the optimization(follow-up), it should be either in Dockerfile or on always. Or if it is opt-in then optimization bool passed to Marshal.

Should we leave this in labs channel for one release to make sure we got the syntax right and commit to backwards compatibility then?

AkihiroSuda · 2022-08-02T07:26:42Z

Sorry for getting this PR stuck in the fileop+scratch optimization, but I don't like the FileOptimize public function at all. Split the second commit to another PR so we can get the main part merged.

Removed the second commit from the PR.

Should we leave this in labs channel for one release to make sure we got the syntax right and commit to backwards compatibility then?

Yes, moved to the labs channel with dfaddgit tag

e.g., # syntax=docker/dockerfile-upstream:master-labs FROM alpine ADD https://github.com/moby/buildkit.git#v0.10.1 /buildkit Close issue 775 Signed-off-by: Akihiro Suda <[email protected]>

Talv · 2022-10-07T22:00:44Z

What's the syntax to get it to clone a repo via SSH on non-default port? Or is it even possible with current implementation? @AkihiroSuda @tonistiigi

ADD ssh://[email protected]:10022/path/repo.git /src doesn't work - it attempts to use HTTPS...

------
 > git://gitlab.localhost/10022/path/repo.git#master:
#0 0.243 fatal: unable to access 'https://ssh://[email protected]:10022/path/repo.git/': Could not resolve host: ssh
------
failed to solve: failed to load cache key: failed to fetch remote https://ssh://[email protected]:10022/path/repo.git: exit status 128

AkihiroSuda · 2022-10-07T23:01:36Z

@Talv Please try ADD git@HOST:REPO.git /src

Talv · 2022-10-08T00:02:34Z

@Talv Please try ADD git@HOST:REPO.git /src

Well, there's no port in your example, which is what I'm getting at.

Here's what I've tried besides the first example:

# ADD [email protected]:10022/path/repo.git#master /src
#  > git://gitlab.localhost/10022/path/repo.git#master:
# #0 129.5 ssh: connect to host gitlab.localhost port 22: Connection timed out

With the short syntax - git@, where ssh is implied - it ignores the port and treats it as a path which is valid according to the specs [1], but not what I want here...

# ADD git://gitlab.localhost:10022/path/repo.git#master /src
#  > git://gitlab.localhost:10022/path/repo.git#master:
#0 0.235 fatal: protocol error: bad line length character: SSH-

With git:// I was able to customize the port of a host, however it establishes the connection via git protocol which is different from ssh, so it fails (correct behavior).

So my issue here is not being able to specify an ssh port other than default (22). Using git cli it's only possible using the full syntax with ssh://, and as seen in my previous post it doesn't work, because docker seems to default to https:// on anything unknown.

[1] https://git-scm.com/docs/git-clone#_git_urls

AkihiroSuda · 2022-10-08T01:52:30Z

Workaround: create .ssh/config
https://stackoverflow.com/questions/5767850/git-on-custom-ssh-port

AkihiroSuda marked this pull request as draft April 12, 2022 08:30

AkihiroSuda added kind/enhancement area/dockerfile labels Apr 12, 2022

AkihiroSuda force-pushed the dockerfile-add-git branch 2 times, most recently from 32ab7ed to ca8d552 Compare April 12, 2022 08:36

AkihiroSuda mentioned this pull request Apr 12, 2022

RFC: dockerfile: support Dockerfile.sum for pinning sources #2794

Closed

AkihiroSuda requested a review from tonistiigi April 18, 2022 19:41

AkihiroSuda mentioned this pull request Apr 20, 2022

dockerfile: support Dockerfile.pin for pinning sources #2816

Closed

tonistiigi reviewed Apr 21, 2022

View reviewed changes

AkihiroSuda force-pushed the dockerfile-add-git branch from ca8d552 to c12674f Compare April 21, 2022 08:03

thaJeztah reviewed Apr 21, 2022

View reviewed changes

tonistiigi reviewed Apr 22, 2022

View reviewed changes

tonistiigi reviewed Apr 23, 2022

View reviewed changes

frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved

frontend/dockerfile/docs/syntax.md Outdated Show resolved Hide resolved

AkihiroSuda force-pushed the dockerfile-add-git branch 5 times, most recently from ce2d58f to 1e14e51 Compare April 27, 2022 10:27

AkihiroSuda force-pushed the dockerfile-add-git branch from 1e14e51 to 9fe2edb Compare May 9, 2022 05:08

AkihiroSuda marked this pull request as ready for review May 9, 2022 05:08

AkihiroSuda requested review from thaJeztah and tonistiigi May 9, 2022 05:09

tonistiigi reviewed May 9, 2022

View reviewed changes

AkihiroSuda force-pushed the dockerfile-add-git branch 3 times, most recently from e3e5765 to 2667f5a Compare May 30, 2022 08:44

AkihiroSuda changed the title ~~dockerfile: implement ADD <git ref>~~ dockerfile: implement ADD <git ref> ; llb: add FileOp optimizer May 30, 2022

AkihiroSuda requested a review from tonistiigi May 30, 2022 08:46

AkihiroSuda force-pushed the dockerfile-add-git branch from 2667f5a to 0ae20b2 Compare May 30, 2022 09:13

AkihiroSuda force-pushed the dockerfile-add-git branch from 0365e67 to 192be4c Compare June 16, 2022 17:29

AkihiroSuda force-pushed the dockerfile-add-git branch from 192be4c to 3734ca1 Compare July 21, 2022 15:58

thaJeztah mentioned this pull request Jul 28, 2022

Stop shallow-cloning remote Git build contexts by default moby/moby#43873

Open

tonistiigi reviewed Aug 2, 2022

View reviewed changes

AkihiroSuda force-pushed the dockerfile-add-git branch from 3734ca1 to 784a780 Compare August 2, 2022 07:24

AkihiroSuda changed the title ~~dockerfile: implement ADD <git ref> ; llb: add FileOp optimizer~~ dockerfile (labs): implement ADD <git ref> ; llb: add FileOp optimizer Aug 2, 2022

AkihiroSuda changed the title ~~dockerfile (labs): implement ADD <git ref> ; llb: add FileOp optimizer~~ dockerfile (labs): implement ADD <git ref> Aug 2, 2022

AkihiroSuda force-pushed the dockerfile-add-git branch from 784a780 to 8bfeafa Compare August 2, 2022 07:44

tonistiigi approved these changes Aug 2, 2022

View reviewed changes

tonistiigi merged commit 28817fc into moby:master Aug 2, 2022

crazy-max removed the area/dockerfile label Sep 13, 2022

thaJeztah mentioned this pull request Oct 4, 2022

[v0.10 backport] update to Go 1.19 #3146

Closed

AkihiroSuda mentioned this pull request Oct 8, 2022

git: support ssh:// syntax with port number #3156

Closed

thaJeztah mentioned this pull request Nov 24, 2022

Dockerfile: use TARGETPLATFORM moby/moby#44513

Merged

This was referenced Jan 30, 2023

docker/dockerfile:1-labs is outdated (still 1.4, not 1.5) #3550

Closed

Dockerfile 'ADD' to support git repositories moby/moby#14704

Closed

thaJeztah added the area/dockerfile label Feb 4, 2023

thaJeztah mentioned this pull request Feb 4, 2023

labs: ADD using git@ should not show git:// as protocol in output/error #3585

Open

crazy-max mentioned this pull request Feb 8, 2023

[0.10] git context: fix support for empty git ref with subdir #3603

Closed

AkihiroSuda mentioned this pull request Jun 30, 2023

dockerfile: graduate ADD <git ref> and ADD --checksum=<checksum> from labs #3979

Merged

AkihiroSuda mentioned this pull request Mar 20, 2025

update slirp4netns (1.3.2), containerd (2.0.4), runc (1.2.6)...; verify the git tags with the commit hashes containerd/nerdctl#4017

Merged

apostasie mentioned this pull request Mar 20, 2025

[CI] Dockerfile enhancements: cloning containerd/nerdctl#4022

Closed

Conversation

AkihiroSuda commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jul 21, 2022

Uh oh!

AkihiroSuda commented Jul 27, 2022

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Aug 2, 2022

Uh oh!

Talv commented Oct 7, 2022

Uh oh!

AkihiroSuda commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Talv commented Oct 8, 2022

Uh oh!

AkihiroSuda commented Oct 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AkihiroSuda commented Apr 12, 2022 •

edited

Loading

AkihiroSuda commented Oct 7, 2022 •

edited

Loading