Skip to content

Use buildkit's gitutil package to detect remote git files#1710

Merged
jedevc merged 2 commits intodocker:masterfrom
jedevc:use-buildkit-gitutil-parsegitref
Apr 3, 2023
Merged

Use buildkit's gitutil package to detect remote git files#1710
jedevc merged 2 commits intodocker:masterfrom
jedevc:use-buildkit-gitutil-parsegitref

Conversation

@jedevc
Copy link
Copy Markdown
Collaborator

@jedevc jedevc commented Apr 3, 2023

🛠️ Fixes #1708.

BuildKit's gitutil package behaves slightly differently than moby's urlutil, so we should rely on BuildKit's gitutil when detecting URLs to avoid cases of accidentally producing invalid build requests that can confuse users.

@jedevc
build: use buildkit's gitutil package to detect remote files
589d4e4 
BuildKit's gitutil package behaves slightly differently than moby's
urlutil, so we should rely on BuildKit's gitutil when detecting URLs to
avoid cases of accidentally producing invalid build requests that can
confuse users.

Signed-off-by: Justin Chadwell <[email protected]>
@jedevc jedevc requested a review from crazy-max April 3, 2023 09:28
@crazy-max
Copy link
Copy Markdown
Member

crazy-max commented Apr 3, 2023

I think we also need changes in

buildx/bake/remote.go

Lines 86 to 129 in 6535f16

func IsRemoteURL(url string) bool {
if _, _, ok := detectHTTPContext(url); ok {
return true
}
if _, ok := detectGitContext(url); ok {
return true
}
return false
}
func detectHTTPContext(url string) (*llb.State, string, bool) {
if httpPrefix.MatchString(url) {
httpContext := llb.HTTP(url, llb.Filename("context"), llb.WithCustomName("[internal] load remote build context"))
return &httpContext, "context", true
}
return nil, "", false
}
func detectGitContext(ref string) (*llb.State, bool) {
found := false
if httpPrefix.MatchString(ref) && gitURLPathWithFragmentSuffix.MatchString(ref) {
found = true
}
for _, prefix := range []string{"git://", "github.com/", "git@"} {
if strings.HasPrefix(ref, prefix) {
found = true
break
}
}
if !found {
return nil, false
}
parts := strings.SplitN(ref, "#", 2)
branch := ""
if len(parts) > 1 {
branch = parts[1]
}
gitOpts := []llb.GitOption{llb.WithCustomName("[internal] load git source " + ref)}
st := llb.Git(parts[0], branch, gitOpts...)
return &st, true
}

@jedevc
bake: use build.IsRemoteURL
87a120e 
Signed-off-by: Justin Chadwell <[email protected]>
@jedevc
Copy link
Copy Markdown
Collaborator Author

jedevc commented Apr 3, 2023

I think we should be able to replace the method there with the new build.IsRemoteURL function (just pushed a commit to do this).

We probably also want to update detectGitContext to align with buildkit, but we should do that by exposing that method to be public first (will open a buildkit-side PR to do that).

@jedevc jedevc mentioned this pull request Apr 3, 2023
Merged
@crazy-max
Copy link
Copy Markdown
Member

crazy-max commented Apr 3, 2023

We probably also want to update detectGitContext to align with buildkit, but we should do that by exposing that method to be public first (will open a buildkit-side PR to do that).

Yes let's do this in a follow-up.

@jedevc jedevc merged commit 16e41ba into docker:master Apr 3, 2023
@jedevc jedevc deleted the use-buildkit-gitutil-parsegitref branch April 12, 2023 09:11
@jedevc jedevc mentioned this pull request Apr 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crazy-max crazy-max crazy-max approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consolidate logic for detection of remote git urls

2 participants

@jedevc @crazy-max