-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Support querystring form Git URLs #6172
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
Changes from all commits
e667628
c8ce372
8159942
1ef2851
8ef9b54
3afd92a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
package llb | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
|
||
"github.com/moby/buildkit/solver/pb" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestGit(t *testing.T) { | ||
t.Parallel() | ||
|
||
type tcase struct { | ||
name string | ||
st State | ||
identifier string | ||
attrs map[string]string | ||
} | ||
|
||
tcases := []tcase{ | ||
{ | ||
name: "refarg", | ||
st: Git("github.com/foo/bar.git", "ref"), | ||
identifier: "git://github.com/foo/bar.git#ref", | ||
attrs: map[string]string{ | ||
"git.authheadersecret": "GIT_AUTH_HEADER", | ||
"git.authtokensecret": "GIT_AUTH_TOKEN", | ||
"git.fullurl": "https://github.com/foo/bar.git", | ||
}, | ||
}, | ||
{ | ||
name: "refarg with subdir", | ||
st: Git("github.com/foo/bar.git", "ref:subdir"), | ||
identifier: "git://github.com/foo/bar.git#ref:subdir", | ||
attrs: map[string]string{ | ||
"git.authheadersecret": "GIT_AUTH_HEADER", | ||
"git.authtokensecret": "GIT_AUTH_TOKEN", | ||
"git.fullurl": "https://github.com/foo/bar.git", | ||
}, | ||
}, | ||
{ | ||
name: "refarg with subdir func", | ||
st: Git("github.com/foo/bar.git", "ref", GitSubDir("subdir")), | ||
identifier: "git://github.com/foo/bar.git#ref:subdir", | ||
attrs: map[string]string{ | ||
"git.authheadersecret": "GIT_AUTH_HEADER", | ||
"git.authtokensecret": "GIT_AUTH_TOKEN", | ||
"git.fullurl": "https://github.com/foo/bar.git", | ||
}, | ||
}, | ||
{ | ||
name: "refarg with override", | ||
st: Git("github.com/foo/bar.git", "ref:dir", GitRef("v1.0")), | ||
identifier: "git://github.com/foo/bar.git#v1.0:dir", | ||
attrs: map[string]string{ | ||
"git.authheadersecret": "GIT_AUTH_HEADER", | ||
"git.authtokensecret": "GIT_AUTH_TOKEN", | ||
"git.fullurl": "https://github.com/foo/bar.git", | ||
}, | ||
}, | ||
{ | ||
name: "funcs only", | ||
st: Git("github.com/foo/bar.git", "", GitRef("v1.0"), GitSubDir("dir")), | ||
identifier: "git://github.com/foo/bar.git#v1.0:dir", | ||
attrs: map[string]string{ | ||
"git.authheadersecret": "GIT_AUTH_HEADER", | ||
"git.authtokensecret": "GIT_AUTH_TOKEN", | ||
"git.fullurl": "https://github.com/foo/bar.git", | ||
}, | ||
}, | ||
} | ||
|
||
for _, tc := range tcases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
st := tc.st | ||
def, err := st.Marshal(context.TODO()) | ||
|
||
require.NoError(t, err) | ||
|
||
m, arr := parseDef(t, def.Def) | ||
require.Equal(t, 2, len(arr)) | ||
|
||
dgst, idx := last(t, arr) | ||
require.Equal(t, 0, idx) | ||
require.Equal(t, m[dgst], arr[0]) | ||
|
||
g := arr[0].Op.(*pb.Op_Source).Source | ||
|
||
require.Equal(t, tc.identifier, g.Identifier) | ||
require.Equal(t, tc.attrs, g.Attrs) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,12 @@ type GitRef struct { | |
// e.g., "bar" for "https://github.com/foo/bar.git" | ||
ShortName string | ||
|
||
// Commit is a commit hash, a tag, or branch name. | ||
// Commit is optional. | ||
Commit string | ||
// Ref is a commit hash, a tag, or branch name. | ||
// Ref is optional. | ||
Ref string | ||
|
||
// Checksum is a commit hash. | ||
Checksum string | ||
|
||
// SubDir is a directory path inside the repo. | ||
// SubDir is optional. | ||
|
@@ -48,10 +51,8 @@ type GitRef struct { | |
UnencryptedTCP bool | ||
} | ||
|
||
// var gitURLPathWithFragmentSuffix = regexp.MustCompile(`\.git(?:#.+)?$`) | ||
|
||
// ParseGitRef parses a git ref. | ||
func ParseGitRef(ref string) (*GitRef, error) { | ||
func ParseGitRef(ref string) (*GitRef, bool, error) { | ||
res := &GitRef{} | ||
|
||
var ( | ||
|
@@ -60,21 +61,25 @@ func ParseGitRef(ref string) (*GitRef, error) { | |
) | ||
|
||
if strings.HasPrefix(ref, "./") || strings.HasPrefix(ref, "../") { | ||
return nil, cerrdefs.ErrInvalidArgument | ||
return nil, false, errors.WithStack(cerrdefs.ErrInvalidArgument) | ||
} else if strings.HasPrefix(ref, "github.com/") { | ||
res.IndistinguishableFromLocal = true // Deprecated | ||
remote = gitutil.FromURL(&url.URL{ | ||
Scheme: "https", | ||
Host: "github.com", | ||
Path: strings.TrimPrefix(ref, "github.com/"), | ||
}) | ||
u, err := url.Parse(ref) | ||
if err != nil { | ||
return nil, false, err | ||
} | ||
u.Scheme = "https" | ||
remote, err = gitutil.FromURL(u) | ||
if err != nil { | ||
return nil, false, err | ||
} | ||
} else { | ||
remote, err = gitutil.ParseURL(ref) | ||
if errors.Is(err, gitutil.ErrUnknownProtocol) { | ||
return nil, err | ||
return nil, false, err | ||
} | ||
if err != nil { | ||
return nil, err | ||
return nil, false, err | ||
} | ||
|
||
switch remote.Scheme { | ||
|
@@ -86,7 +91,7 @@ func ParseGitRef(ref string) (*GitRef, error) { | |
// An HTTP(S) URL is considered to be a valid git ref only when it has the ".git[...]" suffix. | ||
case gitutil.HTTPProtocol, gitutil.HTTPSProtocol: | ||
if !strings.HasSuffix(remote.Path, ".git") { | ||
return nil, cerrdefs.ErrInvalidArgument | ||
return nil, false, errors.WithStack(cerrdefs.ErrInvalidArgument) | ||
} | ||
} | ||
} | ||
|
@@ -96,11 +101,83 @@ func ParseGitRef(ref string) (*GitRef, error) { | |
_, res.Remote, _ = strings.Cut(res.Remote, "://") | ||
} | ||
if remote.Opts != nil { | ||
res.Commit, res.SubDir = remote.Opts.Ref, remote.Opts.Subdir | ||
res.Ref, res.SubDir = remote.Opts.Ref, remote.Opts.Subdir | ||
} | ||
|
||
repoSplitBySlash := strings.Split(res.Remote, "/") | ||
res.ShortName = strings.TrimSuffix(repoSplitBySlash[len(repoSplitBySlash)-1], ".git") | ||
|
||
return res, nil | ||
if err := res.loadQuery(remote.Query); err != nil { | ||
return nil, true, err | ||
} | ||
|
||
return res, true, nil | ||
} | ||
|
||
func (gf *GitRef) loadQuery(query url.Values) error { | ||
if len(query) == 0 { | ||
return nil | ||
} | ||
var tag, branch string | ||
for k, v := range query { | ||
switch len(v) { | ||
case 0: | ||
return errors.Errorf("query %q has no value", k) | ||
case 1: | ||
if v[0] == "" { | ||
return errors.Errorf("query %q has no value", k) | ||
} | ||
// NOP | ||
default: | ||
return errors.Errorf("query %q has multiple values", k) | ||
} | ||
switch k { | ||
case "ref": | ||
if gf.Ref != "" && gf.Ref != v[0] { | ||
return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, v[0]) | ||
} | ||
gf.Ref = v[0] | ||
case "tag": | ||
tag = v[0] | ||
case "branch": | ||
branch = v[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess #4905 (comment) wasn't convincing enough to drop branch and tag in favor of just ref? 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use |
||
case "subdir": | ||
if gf.SubDir != "" && gf.SubDir != v[0] { | ||
return errors.Errorf("subdir conflicts: %q vs %q", gf.SubDir, v[0]) | ||
} | ||
gf.SubDir = v[0] | ||
case "checksum", "commit": | ||
gf.Checksum = v[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a weird word to use for a commit hash 🤔 it is technically a checksum, but I don't think there's really any prior art for calling one of them "checksum" is there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
default: | ||
return errors.Errorf("unexpected query %q", k) | ||
} | ||
} | ||
if tag != "" { | ||
const tagPrefix = "refs/tags/" | ||
if !strings.HasPrefix(tag, tagPrefix) { | ||
tag = tagPrefix + tag | ||
} | ||
if gf.Ref != "" && gf.Ref != tag { | ||
return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, tag) | ||
} | ||
gf.Ref = tag | ||
} | ||
if branch != "" { | ||
if tag != "" { | ||
// TODO: consider allowing this, when the tag actually exists on the branch | ||
return errors.New("branch conflicts with tag") | ||
} | ||
const branchPrefix = "refs/heads/" | ||
if !strings.HasPrefix(branch, branchPrefix) { | ||
branch = branchPrefix + branch | ||
} | ||
if gf.Ref != "" && gf.Ref != branch { | ||
return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, branch) | ||
} | ||
gf.Ref = branch | ||
} | ||
if gf.Checksum != "" && gf.Ref == "" { | ||
gf.Ref = gf.Checksum | ||
} | ||
return nil | ||
} |
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.
fragment
needs explanationThere 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.
Note that there is no functional change in here. The previous var was incorrectly named; it was actually a
ref:substring
pair.