Merge pull request #768 from N4rm0/fix-envar-replacement-for-url

Fix #691 - ADD does not understand ENV variables
GoogleContainerTools · Oct 4, 2019 · 2218859 · 2218859
2 parents e87304d + 019b26e
commit 2218859
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 11 deletions.
diff --git a/integration/dockerfiles/Dockerfile_test_add b/integration/dockerfiles/Dockerfile_test_add
@@ -28,3 +28,7 @@ ADD https://github.com/GoogleCloudPlatform/docker-credential-gcr/releases/downlo
 # Test environment replacement in the URL
 ENV VERSION=v1.4.3
 ADD https://github.com/GoogleCloudPlatform/docker-credential-gcr/releases/download/${VERSION}-static/docker-credential-gcr_linux_amd64-1.4.3.tar.gz /destination
+
+# Test full url replacement
+ENV URL=https://github.com/GoogleCloudPlatform/docker-credential-gcr/releases/download/v1.4.3/docker-credential-gcr_linux_386-1.4.3.tar.gz
+ADD $URL /otherdestination
diff --git a/integration/integration_test.go b/integration/integration_test.go
@@ -323,7 +323,7 @@ func TestGitBuildContextWithBranch(t *testing.T) {
 
 func TestLayers(t *testing.T) {
 	offset := map[string]int{
-		"Dockerfile_test_add":     11,
+		"Dockerfile_test_add":     12,
 		"Dockerfile_test_scratch": 3,
 	}
 	for dockerfile := range imageBuilder.FilesBuilt {

diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go
@@ -37,13 +37,7 @@ import (
 func ResolveEnvironmentReplacementList(values, envs []string, isFilepath bool) ([]string, error) {
 	var resolvedValues []string
 	for _, value := range values {
-		var resolved string
-		var err error
-		if IsSrcRemoteFileURL(value) {
-			resolved, err = ResolveEnvironmentReplacement(value, envs, false)
-		} else {
-			resolved, err = ResolveEnvironmentReplacement(value, envs, isFilepath)
-		}
+		resolved, err := ResolveEnvironmentReplacement(value, envs, isFilepath)
 		logrus.Debugf("Resolved %s to %s", value, resolved)
 		if err != nil {
 			return nil, err
@@ -65,7 +59,8 @@ func ResolveEnvironmentReplacementList(values, envs []string, isFilepath bool) (
 func ResolveEnvironmentReplacement(value string, envs []string, isFilepath bool) (string, error) {
 	shlex := shell.NewLex(parser.DefaultEscapeToken)
 	fp, err := shlex.ProcessWord(value, envs)
-	if !isFilepath {
+	// Check after replacement if value is a remote URL
+	if !isFilepath || IsSrcRemoteFileURL(fp) {
 		return fp, err
 	}
 	if err != nil {

diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go
@@ -97,6 +97,22 @@ var testEnvReplacement = []struct {
 		},
 		expectedPath: "8080/udp",
 	},
+	{
+		path: "$url",
+		envs: []string{
+			"url=http://example.com",
+		},
+		isFilepath:   true,
+		expectedPath: "http://example.com",
+	},
+	{
+		path: "$url",
+		envs: []string{
+			"url=http://example.com",
+		},
+		isFilepath:   false,
+		expectedPath: "http://example.com",
+	},
 }
 
 func Test_EnvReplacement(t *testing.T) {
@@ -472,14 +488,15 @@ func TestResolveEnvironmentReplacementList(t *testing.T) {
 			name: "url",
 			args: args{
 				values: []string{
-					"https://google.com/$foo", "$bar",
+					"https://google.com/$foo", "$bar", "$url",
 				},
 				envs: []string{
 					"foo=baz",
 					"bar=bat",
+					"url=https://google.com",
 				},
 			},
-			want: []string{"https://google.com/baz", "bat"},
+			want: []string{"https://google.com/baz", "bat", "https://google.com"},
 		},
 		{
 			name: "mixed",