Fix handling of remote "git@" notation#33696
Conversation
|
We also need to update https://github.com/docker/cli after this is merged, because the same code is used in the client |
There was a problem hiding this comment.
Alternatively, we can probably just add ssh:// here, but let me know what you prefer
Nevermind, that doesn't work of course because some-server:something isn't valid
|
I don't think we should be special casing the user So it looks like we need a cleverer way to distinguish ssh type targets from URLs. Since this syntactic quirk comes from git perhaps we should follow https://git-scm.com/docs/git-fetch#URLS which says:
It seems like "This syntax is only recognized if there are no slashes before the first colon." is the helpful part. |
|
@ijc the current code special-cases |
|
@thaJeztah out of curiosity, how are https://github.com/moby/moby and https://github.com/docker/cli versioned against each other? It looks like this code is imported at https://github.com/docker/cli/blob/a74e715b1a370f0b71dcae5f71b9ed147d7a2977/cli/command/image/build/context.go#L18. Is that reference versioned at all? |
|
@ecnerwala Go packages that are used in docker/cli are vendored. You can find the exact commit that is used in the And the vendored files can be found in the The import is still named |
There was a problem hiding this comment.
IsGitTransport already checks for IsURL, so the second check is unnecessary.
There was a problem hiding this comment.
oh, nice catch! I had it slightly different before, and moved this around
There was a problem hiding this comment.
Perhaps this whole function should be in the urlutil package so that the various hardcoded assumptions made about git@ and other stuff here and in urlutil.IsGitTransport are all in the same place?
Or at least the tail should perhaps be put in a urlutil.ParseGitTransport (to match IsGitTransport)?
There was a problem hiding this comment.
I started making this change, but looking at it, doing so would also require making gitRepo an exported type. I'm not really sure we should make it an official "interface" (type)
I did look at IsGitTransport() and it's only used in this file, so instead, perhaps we should move the function here?
There was a problem hiding this comment.
diff --git a/builder/remotecontext/git/gitutils.go b/builder/remotecontext/git/gitutils.go
index 91a0f5b..b94d462 100644
--- a/builder/remotecontext/git/gitutils.go
+++ b/builder/remotecontext/git/gitutils.go
@@ -57,7 +57,7 @@ func Clone(remoteURL string) (string, error) {
func parseRemoteURL(remoteURL string) (gitRepo, error) {
repo := gitRepo{}
- if !urlutil.IsGitTransport(remoteURL) {
+ if !isGitTransport(remoteURL) {
remoteURL = "https://" + remoteURL
}
@@ -151,3 +151,9 @@ func gitWithinDir(dir string, args ...string) ([]byte, error) {
func git(args ...string) ([]byte, error) {
return exec.Command("git", args...).CombinedOutput()
}
+
+// isGitTransport returns true if the provided str is a git transport by inspecting
+// the prefix of the string for known protocols used in git.
+func isGitTransport(str string) bool {
+ return urlutil.IsURL(str) || strings.HasPrefix(str, "git://") || strings.HasPrefix(str, "git@")
+}
diff --git a/builder/remotecontext/git/gitutils_test.go b/builder/remotecontext/git/gitutils_test.go
index b6523a2..c638a49 100644
--- a/builder/remotecontext/git/gitutils_test.go
+++ b/builder/remotecontext/git/gitutils_test.go
@@ -209,3 +209,30 @@ func TestCheckoutGit(t *testing.T) {
assert.Equal(t, c.exp, string(b))
}
}
+
+func TestValidGitTransport(t *testing.T) {
+ gitUrls := []string{
+ "git://github.com/docker/docker",
+ "[email protected]:docker/docker.git",
+ "[email protected]:atlassianlabs/atlassian-docker.git",
+ "https://github.com/docker/docker.git",
+ "http://github.com/docker/docker.git",
+ "http://github.com/docker/docker.git#branch",
+ "http://github.com/docker/docker.git#:dir",
+ }
+ incompleteGitUrls := []string{
+ "github.com/docker/docker",
+ }
+
+ for _, url := range gitUrls {
+ if !isGitTransport(url) {
+ t.Fatalf("%q should be detected as valid Git prefix", url)
+ }
+ }
+
+ for _, url := range incompleteGitUrls {
+ if isGitTransport(url) {
+ t.Fatalf("%q should not be detected as valid Git prefix", url)
+ }
+ }
+}
diff --git a/pkg/urlutil/urlutil.go b/pkg/urlutil/urlutil.go
index 4415287..cfcd582 100644
--- a/pkg/urlutil/urlutil.go
+++ b/pkg/urlutil/urlutil.go
@@ -29,12 +29,6 @@ func IsGitURL(str string) bool {
return checkURL(str, "git")
}
-// IsGitTransport returns true if the provided str is a git transport by inspecting
-// the prefix of the string for known protocols used in git.
-func IsGitTransport(str string) bool {
- return IsURL(str) || strings.HasPrefix(str, "git://") || strings.HasPrefix(str, "git@")
-}
-
// IsTransportURL returns true if the provided str is a transport (tcp, tcp+tls, udp, unix) URL.
func IsTransportURL(str string) bool {
return checkURL(str, "transport")
diff --git a/pkg/urlutil/urlutil_test.go b/pkg/urlutil/urlutil_test.go
index d84145a..e7579f5 100644
--- a/pkg/urlutil/urlutil_test.go
+++ b/pkg/urlutil/urlutil_test.go
@@ -27,20 +27,6 @@ var (
}
)
-func TestValidGitTransport(t *testing.T) {
- for _, url := range gitUrls {
- if !IsGitTransport(url) {
- t.Fatalf("%q should be detected as valid Git prefix", url)
- }
- }
-
- for _, url := range incompleteGitUrls {
- if IsGitTransport(url) {
- t.Fatalf("%q should not be detected as valid Git prefix", url)
- }
- }
-}
-
func TestIsGIT(t *testing.T) {
for _, url := range gitUrls {
if !IsGitURL(url) {There was a problem hiding this comment.
I did look at
IsGitTransport()and it's only used in this file, so instead, perhaps we should move the function here?
That sounds reasonable (as does the diff snippet above as far as I can tell through the comment rendering).
`docker build` accepts remote repositories using either the `git://` notation, or `git@`. Docker attempted to parse both as an URL, however, `git@` is not an URL, but an argument to `git clone`. Go 1.7 silently ignored this, and managed to extract the needed information from these remotes, however, Go 1.8 does a more strict validation, and invalidated these. This patch adds a different path for `git@` remotes, to prevent them from being handled as URL (and invalidated). A test is also added, because there were no tests for handling of `git@` remotes. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This function was only used inside gitutils, and is written specifically for the requirements there. Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
@ijc pushed the changes as a second commit, PTAL |
|
LGTM |
Includes changes from; - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away moby/moby#33798 (related to docker#236) - Update go-connections dependency moby/moby#33814 (already vendored in docker#238) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker/cli#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 366d3ec971d8007c667e8d7dc8e35a346fb19539 Component: cli
Includes changes from; - Add a LastTagTime for images (moby/moby#31497) - Fix handling of remote "git@" notation (moby/moby#33696) - Move some `api` package functions away (moby/moby#33798) (related to docker#236) - Set ping version even on error (moby/moby#33827) - Do not add duplicate platform information to service spec (moby/moby#33867) - Refactor MountPoint Setup function in volume.go (moby/moby#33890) Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker buildaccepts remote repositories using either thegit://notation, orgit@.Docker attempted to parse both as an URL, however,
git@is not an URL, but an argument togit clone.Go 1.7 silently ignored this, and managed to extract the needed information from these remotes, however, Go 1.8 does a more strict validation, and invalidated these.
This patch adds a different path for
git@remotes, to prevent them from being handled as URL (and invalidated).A test is also added, because there were no tests for handling of
git@remotes.relates to #33686
ping @tonistiigi @ijc PTAL