Repository navigation
git: support all git transports - #416
Conversation
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
8382def to
1067c24
Compare
| if httpPrefix.MatchString(ref) && gitUrlPathWithFragmentSuffix.MatchString(ref) { | ||
| found = true | ||
| } | ||
|
|
There was a problem hiding this comment.
This is not robust, although go get does similar stuff with support for gitlab and bitbucket (IIRC docker does not)
Can we use another opt string?
There was a problem hiding this comment.
docker build uses similar logic to switch between http/git. If we use a separate field we just need to do this in a wrapper and users need to remember that docker build and dockerfile frontend take different opts. Also, we probably still would need to validate this value in a similar way as llb.Git() doesn't necessarily handle any string.
There was a problem hiding this comment.
IIUC docker build does not depend on "github.com" string, unlike this PR and go get?
https://github.com/moby/moby/blob/master/builder/remotecontext/git/gitutils.go
There was a problem hiding this comment.
https://github.com/moby/moby/blob/master/pkg/urlutil/urlutil.go#L13 (this is called by the CLI)
There was a problem hiding this comment.
Note that on the CLI, a test was added (docker/cli#390) to make sure that an existing local directory starting with github.com/ is not ignored; https://github.com/docker/cli/blob/0f11a310fd7d64a697d0ed9098205b6cdd585f1b/cli/command/image/build_test.go#L176-L196
|
Something is really up with |
|
LGTM but probably we should also add gitlab to both Docker/Moby and BuildKit later. EDIT: we should NOT, see moby/moby#37173 |
|
re: ci, is it possible that #417 broke something? |
|
restarted and green |
| ) | ||
|
|
||
| var httpPrefix = regexp.MustCompile("^https?://") | ||
| var gitUrlPathWithFragmentSuffix = regexp.MustCompile(".git(?:#.+)?$") |
There was a problem hiding this comment.
Super minor but I think the first . (in .git) needs escaping since you want a literal . not to match any char.
| found = true | ||
| } | ||
|
|
||
| for _, prefix := range []string{"git://", "github.com/", "git@"} { |
There was a problem hiding this comment.
If it weren't for the github.com/ (which I think is/should be Dockerfile specific) in here this would be quite a convenient utility function for the common case, is there some way we could extract/abstract it perhaps?
A new SourceFromPath function which produced either an llb.Git or an llb.Local depending what it found might be useful?
5 participants
Add clearer support for all supported git transports that optionally switch to another attribute field instead of making invalid URLs. Also, update dockerfile to accept https://../repo.git as a git URL instead of HTTP URL
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com