-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
Workaround "docker build giturl" requiring the url to end in ".git" #15
Conversation
meta.jq
Outdated
# without ".git" in the url "docker build url" fails and tries to build the html repo page as a Dockerfile | ||
# https://github.com/moby/moby/blob/4f9c865eddc09da0c0cb9fe08c76b81b804093f5/builder/remotecontext/urlutil/urlutil.go#L15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(included in the better later suggestion)
# without ".git" in the url "docker build url" fails and tries to build the html repo page as a Dockerfile | |
# https://github.com/moby/moby/blob/4f9c865eddc09da0c0cb9fe08c76b81b804093f5/builder/remotecontext/urlutil/urlutil.go#L15 | |
# without ".git" in the url "docker buildx build url" fails and tries to build the html repo page as a Dockerfile | |
# https://github.com/moby/buildkit/blob/0e1e36ba9eb8142968b2c5cfa2f12549bf9246d9/util/gitutil/git_ref.go#L81-L87 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That regular expression is ".git(?:#.+)?$"
, which allows for anchors while ($gitRepo | endswith(".git") | not)
does not. Is this okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine being stricter than docker build
. And the newer link from buildkit is also stricter and is the one we'll hit (since we are doing docker buildx build
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that BuildKit code parses the URL and matches specifically on the Path portion of the URL, which does not include any query or fragment portions of the URL.
But perhaps there are not uses of queries or fragments in the bashbrew files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think git clone
on a URL with a query parameter would actually work correctly, but maybe there's some esoteric server somewhere? I'm not worried about the regex being .git
because that's just a bug (a 6 year old bug, to be fair, so could be considered "load bearing", but BuildKit implemented the same check more correctly and nobody's noticed, so I don't think anyone's put real load on it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and the anchors are a Docker-ism and we generate the correct anchor lower down in this file (git clone
doesn't support anchors and they're a Docker-ism specifically to specify -b
and a directory path).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In other words, any GitRepo:
with an anchor in it would be invalid already)
Aha! docker/cli#1738 |
Requirements like in moby/buildkit
git_ref.go
(even though git URLs don't need to end in.git
).