Skip to content

Commit c8ce372

Browse files
tonistiigitonistiigi
committed
dfgitutil: add querystring style URLs to dfgitutil
Signed-off-by: Tonis Tiigi <[email protected]>
1 parent e667628 commit c8ce372

File tree

6 files changed

+201
-127
lines changed

6 files changed

+201
-127
lines changed

‎frontend/dockerfile/dfgitutil/git_ref.go‎

Lines changed: 89 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
package dfgitutil
33

44
import (
5+
"log"
56
"net/url"
67
"strings"
78

@@ -23,9 +24,12 @@ type GitRef struct {
2324
// e.g., "bar" for "https://github.com/foo/bar.git"
2425
ShortName string
2526

26-
// Commit is a commit hash, a tag, or branch name.
27-
// Commit is optional.
28-
Commit string
27+
// Ref is a commit hash, a tag, or branch name.
28+
// Ref is optional.
29+
Ref string
30+
31+
// Checksum is a commit hash.
32+
Checksum string
2933

3034
// SubDir is a directory path inside the repo.
3135
// SubDir is optional.
@@ -60,14 +64,15 @@ func ParseGitRef(ref string) (*GitRef, error) {
6064
)
6165

6266
if strings.HasPrefix(ref, "./") || strings.HasPrefix(ref, "../") {
63-
return nil, cerrdefs.ErrInvalidArgument
67+
return nil, errors.WithStack(cerrdefs.ErrInvalidArgument)
6468
} else if strings.HasPrefix(ref, "github.com/") {
6569
res.IndistinguishableFromLocal = true // Deprecated
66-
remote, err = gitutil.FromURL(&url.URL{
67-
Scheme: "https",
68-
Host: "github.com",
69-
Path: strings.TrimPrefix(ref, "github.com/"),
70-
})
70+
u, err := url.Parse(ref)
71+
if err != nil {
72+
return nil, err
73+
}
74+
u.Scheme = "https"
75+
remote, err = gitutil.FromURL(u)
7176
if err != nil {
7277
return nil, err
7378
}
@@ -89,7 +94,7 @@ func ParseGitRef(ref string) (*GitRef, error) {
8994
// An HTTP(S) URL is considered to be a valid git ref only when it has the ".git[...]" suffix.
9095
case gitutil.HTTPProtocol, gitutil.HTTPSProtocol:
9196
if !strings.HasSuffix(remote.Path, ".git") {
92-
return nil, cerrdefs.ErrInvalidArgument
97+
return nil, errors.WithStack(cerrdefs.ErrInvalidArgument)
9398
}
9499
}
95100
}
@@ -99,11 +104,84 @@ func ParseGitRef(ref string) (*GitRef, error) {
99104
_, res.Remote, _ = strings.Cut(res.Remote, "://")
100105
}
101106
if remote.Opts != nil {
102-
res.Commit, res.SubDir = remote.Opts.Ref, remote.Opts.Subdir
107+
res.Ref, res.SubDir = remote.Opts.Ref, remote.Opts.Subdir
103108
}
104109

105110
repoSplitBySlash := strings.Split(res.Remote, "/")
106111
res.ShortName = strings.TrimSuffix(repoSplitBySlash[len(repoSplitBySlash)-1], ".git")
107112

113+
if err := res.loadQuery(remote.Query); err != nil {
114+
log.Printf("query")
115+
return nil, err
116+
}
117+
108118
return res, nil
109119
}
120+
121+
func (gf *GitRef) loadQuery(query url.Values) error {
122+
if len(query) == 0 {
123+
return nil
124+
}
125+
var tag, branch string
126+
for k, v := range query {
127+
switch len(v) {
128+
case 0:
129+
return errors.Errorf("query %q has no value", k)
130+
case 1:
131+
if v[0] == "" {
132+
return errors.Errorf("query %q has no value", k)
133+
}
134+
// NOP
135+
default:
136+
return errors.Errorf("query %q has multiple values", k)
137+
}
138+
switch k {
139+
case "ref":
140+
if gf.Ref != "" && gf.Ref != v[0] {
141+
return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, v[0])
142+
}
143+
gf.Ref = v[0]
144+
case "tag":
145+
tag = v[0]
146+
case "branch":
147+
branch = v[0]
148+
case "subdir":
149+
if gf.SubDir != "" && gf.SubDir != v[0] {
150+
return errors.Errorf("subdir conflicts: %q vs %q", gf.SubDir, v[0])
151+
}
152+
gf.SubDir = v[0]
153+
case "checksum", "commit":
154+
gf.Checksum = v[0]
155+
default:
156+
return errors.Errorf("unexpected query %q", k)
157+
}
158+
}
159+
if tag != "" {
160+
const tagPrefix = "refs/tags/"
161+
if !strings.HasPrefix(tag, tagPrefix) {
162+
tag = tagPrefix + tag
163+
}
164+
if gf.Ref != "" && gf.Ref != tag {
165+
return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, tag)
166+
}
167+
gf.Ref = tag
168+
}
169+
if branch != "" {
170+
if tag != "" {
171+
// TODO: consider allowing this, when the tag actually exists on the branch
172+
return errors.New("branch conflicts with tag")
173+
}
174+
const branchPrefix = "refs/heads/"
175+
if !strings.HasPrefix(branch, branchPrefix) {
176+
branch = branchPrefix + branch
177+
}
178+
if gf.Ref != "" && gf.Ref != branch {
179+
return errors.Errorf("ref conflicts: %q vs %q", gf.Ref, branch)
180+
}
181+
gf.Ref = branch
182+
}
183+
if gf.Checksum != "" && gf.Ref == "" {
184+
gf.Ref = gf.Checksum
185+
}
186+
return nil
187+
}

‎frontend/dockerfile/dfgitutil/git_ref_test.go‎

Lines changed: 80 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package dfgitutil
22

33
import (
4+
"fmt"
45
"testing"
56

67
"github.com/stretchr/testify/require"
@@ -10,6 +11,7 @@ func TestParseGitRef(t *testing.T) {
1011
cases := []struct {
1112
ref string
1213
expected *GitRef
14+
err string
1315
}{
1416
{
1517
ref: "https://example.com/",
@@ -31,15 +33,15 @@ func TestParseGitRef(t *testing.T) {
3133
expected: &GitRef{
3234
Remote: "https://example.com/foo.git",
3335
ShortName: "foo",
34-
Commit: "deadbeef",
36+
Ref: "deadbeef",
3537
},
3638
},
3739
{
3840
ref: "https://example.com/foo.git#release/1.2",
3941
expected: &GitRef{
4042
Remote: "https://example.com/foo.git",
4143
ShortName: "foo",
42-
Commit: "release/1.2",
44+
Ref: "release/1.2",
4345
},
4446
},
4547
{
@@ -66,6 +68,15 @@ func TestParseGitRef(t *testing.T) {
6668
IndistinguishableFromLocal: true,
6769
},
6870
},
71+
{
72+
ref: "github.com/moby/buildkit#master",
73+
expected: &GitRef{
74+
Remote: "github.com/moby/buildkit",
75+
ShortName: "buildkit",
76+
IndistinguishableFromLocal: true,
77+
Ref: "master",
78+
},
79+
},
6980
{
7081
ref: "custom.xyz/moby/buildkit.git",
7182
expected: nil,
@@ -114,7 +125,7 @@ func TestParseGitRef(t *testing.T) {
114125
expected: &GitRef{
115126
Remote: "https://github.com/foo/bar.git",
116127
ShortName: "bar",
117-
Commit: "baz/qux",
128+
Ref: "baz/qux",
118129
SubDir: "quux/quuz",
119130
},
120131
},
@@ -143,17 +154,80 @@ func TestParseGitRef(t *testing.T) {
143154
expected: &GitRef{
144155
Remote: "https://github.com/docker/docker.git",
145156
ShortName: "docker",
146-
Commit: "v1.0.0",
157+
Ref: "v1.0.0",
158+
SubDir: "/subdir",
159+
},
160+
},
161+
{
162+
ref: "https://github.com/moby/buildkit.git?subdir=/subdir#v1.0.0",
163+
expected: &GitRef{
164+
Remote: "https://github.com/moby/buildkit.git",
165+
ShortName: "buildkit",
166+
Ref: "v1.0.0",
167+
SubDir: "/subdir",
168+
},
169+
},
170+
{
171+
ref: "https://github.com/moby/buildkit.git?tag=v1.0.0",
172+
expected: &GitRef{
173+
Remote: "https://github.com/moby/buildkit.git",
174+
ShortName: "buildkit",
175+
Ref: "refs/tags/v1.0.0",
176+
},
177+
},
178+
{
179+
ref: "github.com/moby/buildkit?tag=v1.0.0",
180+
expected: &GitRef{
181+
Remote: "github.com/moby/buildkit",
182+
ShortName: "buildkit",
183+
Ref: "refs/tags/v1.0.0",
184+
IndistinguishableFromLocal: true,
185+
},
186+
},
187+
{
188+
ref: "https://github.com/moby/buildkit.git?branch=v1.0",
189+
expected: &GitRef{
190+
Remote: "https://github.com/moby/buildkit.git",
191+
ShortName: "buildkit",
192+
Ref: "refs/heads/v1.0",
193+
},
194+
},
195+
{
196+
ref: "https://github.com/moby/buildkit.git?ref=v1.0.0#v1.2.3",
197+
err: "ref conflicts",
198+
},
199+
{
200+
ref: "https://github.com/moby/buildkit.git?ref=v1.0.0&tag=v1.2.3",
201+
err: "ref conflicts",
202+
},
203+
{
204+
// TODO: consider allowing this, when the tag actually exists on the branch
205+
ref: "https://github.com/moby/buildkit.git?tag=v1.0.0&branch=v1.0",
206+
err: "branch conflicts with tag",
207+
},
208+
{
209+
ref: "[email protected]:moby/buildkit.git?subdir=/subdir#v1.0.0",
210+
expected: &GitRef{
211+
Remote: "[email protected]:moby/buildkit.git",
212+
ShortName: "buildkit",
213+
Ref: "v1.0.0",
147214
SubDir: "/subdir",
148215
},
149216
},
217+
{
218+
ref: "https://github.com/moby/buildkit.git?invalid=123",
219+
err: "unexpected query \"invalid\"",
220+
},
150221
}
151-
for _, tt := range cases {
152-
t.Run(tt.ref, func(t *testing.T) {
222+
for i, tt := range cases {
223+
t.Run(fmt.Sprintf("case%d", i+1), func(t *testing.T) {
153224
got, err := ParseGitRef(tt.ref)
154225
if tt.expected == nil {
155226
require.Nil(t, got)
156227
require.Error(t, err)
228+
if tt.err != "" {
229+
require.ErrorContains(t, err, tt.err)
230+
}
157231
} else {
158232
require.NoError(t, err)
159233
require.Equal(t, tt.expected, got)

‎frontend/dockerfile/dockerfile2llb/convert.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1513,7 +1513,7 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
15131513
return errors.New("source can't be a git ref for COPY")
15141514
}
15151515
// TODO: print a warning (not an error) if gitRef.UnencryptedTCP is true
1516-
commit := gitRef.Commit
1516+
commit := gitRef.Ref
15171517
if gitRef.SubDir != "" {
15181518
commit += ":" + gitRef.SubDir
15191519
}

‎frontend/dockerui/context.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ func DetectGitContext(ref string, keepGit bool) (*llb.State, bool) {
145145
if err != nil {
146146
return nil, false
147147
}
148-
commit := g.Commit
148+
commit := g.Ref
149149
if g.SubDir != "" {
150150
commit += ":" + g.SubDir
151151
}

0 commit comments

Comments
 (0)