fix: rewrite how image env is merged (#828)

* fix: rewrite how image env is merged

* test: add test for extractFromImageEnv

pkg/container/docker_run.go

@@ -17,6 +17,7 @@ import (
 	"github.com/go-git/go-billy/v5/helper/polyfill"
 	"github.com/go-git/go-billy/v5/osfs"
 	"github.com/go-git/go-git/v5/plumbing/format/gitignore"
+	"github.com/joho/godotenv"
 
 	"github.com/docker/cli/cli/connhelper"
 	"github.com/docker/docker/api/types"
@@ -72,6 +73,7 @@ type Container interface {
 	Start(attach bool) common.Executor
 	Exec(command []string, env map[string]string, user, workdir string) common.Executor
 	UpdateFromEnv(srcPath string, env *map[string]string) common.Executor
+	UpdateFromImageEnv(env *map[string]string) common.Executor
 	UpdateFromPath(env *map[string]string) common.Executor
 	Remove() common.Executor
 }
@@ -167,6 +169,10 @@ func (cr *containerReference) UpdateFromEnv(srcPath string, env *map[string]stri
 	return cr.extractEnv(srcPath, env).IfNot(common.Dryrun)
 }
 
+func (cr *containerReference) UpdateFromImageEnv(env *map[string]string) common.Executor {
+	return cr.extractFromImageEnv(env).IfNot(common.Dryrun)
+}
+
 func (cr *containerReference) UpdateFromPath(env *map[string]string) common.Executor {
 	return cr.extractPath(env).IfNot(common.Dryrun)
 }
@@ -296,13 +302,6 @@ func (cr *containerReference) create(capAdd []string, capDrop []string) common.E
 		isTerminal := term.IsTerminal(int(os.Stdout.Fd()))
 		input := cr.input
 
-		insp, _, err := cr.cli.ImageInspectWithRaw(ctx, input.Image)
-		if err != nil {
-			logger.Error(err)
-		}
-
-		input.Env = mergeEnvFromImage(input.Env, insp.Config.Env)
-
 		config := &container.Config{
 			Image:      input.Image,
 			Cmd:        input.Cmd,
@@ -355,39 +354,6 @@ func (cr *containerReference) create(capAdd []string, capDrop []string) common.E
 	}
 }
 
-func mergeEnvFromImage(inputEnv, imageEnv []string) []string {
-	envMap := make(map[string]string)
-	for _, v := range inputEnv {
-		e := strings.Split(v, `=`)
-		if e[1] == "" {
-			envMap[e[0]] = ""
-		} else {
-			envMap[e[0]] = e[1]
-		}
-	}
-
-	for _, v := range imageEnv {
-		e := strings.SplitN(v, `=`, 2)
-		if env, ok := envMap[e[0]]; !ok {
-			if e[1] != "" {
-				envMap[e[0]] = e[1]
-			}
-		} else {
-			if e[0] == "PATH" {
-				if e[1] != "" {
-					envMap[e[0]] = strings.Join([]string{env, e[1]}, `:`)
-				}
-			}
-		}
-	}
-
-	out := make([]string, 0)
-	for k, v := range envMap {
-		out = append(out, strings.Join([]string{k, v}, `=`))
-	}
-	return out
-}
-
 var singleLineEnvPattern, mulitiLineEnvPattern *regexp.Regexp
 
 func (cr *containerReference) extractEnv(srcPath string, env *map[string]string) common.Executor {
@@ -437,6 +403,38 @@ func (cr *containerReference) extractEnv(srcPath string, env *map[string]string)
 	}
 }
 
+func (cr *containerReference) extractFromImageEnv(env *map[string]string) common.Executor {
+	envMap := *env
+	return func(ctx context.Context) error {
+		logger := common.Logger(ctx)
+
+		inspect, _, err := cr.cli.ImageInspectWithRaw(ctx, cr.input.Image)
+		if err != nil {
+			logger.Error(err)
+		}
+
+		imageEnv, err := godotenv.Unmarshal(strings.Join(inspect.Config.Env, "\n"))
+		if err != nil {
+			logger.Error(err)
+		}
+
+		for k, v := range imageEnv {
+			if k == "PATH" {
+				if envMap[k] == "" {
+					envMap[k] = v
+				} else {
+					envMap[k] += `:` + v
+				}
+			} else if envMap[k] == "" {
+				envMap[k] = v
+			}
+		}
+
+		env = &envMap
+		return nil
+	}
+}
+
 func (cr *containerReference) extractPath(env *map[string]string) common.Executor {
 	localEnv := *env
 	return func(ctx context.Context) error {

pkg/container/docker_run_test.go

@@ -1,34 +1,47 @@
 package container
 
 import (
-	"sort"
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func TestMergeEnvFromImage(t *testing.T) {
+func TestDocker(t *testing.T) {
-	inputEnv := []string{
+	ctx := context.Background()
-		"PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/sbin",
+	client, err := GetDockerClient(ctx)
-		"GOPATH=/root/go",
+	assert.NoError(t, err)
-		"GOOS=linux",
+
+	dockerBuild := NewDockerBuildExecutor(NewDockerBuildExecutorInput{
+		ContextDir: "testdata",
+		ImageTag:   "envmergetest",
+	})
+
+	err = dockerBuild(ctx)
+	assert.NoError(t, err)
+
+	cr := &containerReference{
+		cli: client,
+		input: &NewContainerInput{
+			Image: "envmergetest",
+		},
 	}
-	imageEnv := []string{
+	env := map[string]string{
-		"PATH=/root/go/bin",
+		"PATH":         "/usr/local/bin:/usr/bin:/usr/sbin:/bin:/sbin",
-		"GOPATH=/tmp",
+		"RANDOM_VAR":   "WITH_VALUE",
-		"GOARCH=amd64",
+		"ANOTHER_VAR":  "",
+		"CONFLICT_VAR": "I_EXIST_IN_MULTIPLE_PLACES",
 	}
 
-	merged := mergeEnvFromImage(inputEnv, imageEnv)
+	envExecutor := cr.extractFromImageEnv(&env)
-	sort.Strings(merged)
+	err = envExecutor(ctx)
-
+	assert.NoError(t, err)
-	expected := []string{
+	assert.Equal(t, map[string]string{
-		"PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/sbin:/root/go/bin",
+		"PATH":            "/usr/local/bin:/usr/bin:/usr/sbin:/bin:/sbin:/this/path/does/not/exists/anywhere:/this/either",
-		"GOPATH=/root/go",
+		"RANDOM_VAR":      "WITH_VALUE",
-		"GOOS=linux",
+		"ANOTHER_VAR":     "",
-		"GOARCH=amd64",
+		"SOME_RANDOM_VAR": "",
-	}
+		"ANOTHER_ONE":     "BUT_I_HAVE_VALUE",
-	sort.Strings(expected)
+		"CONFLICT_VAR":    "I_EXIST_IN_MULTIPLE_PLACES",
-
+	}, env)
-	assert.Equal(t, expected, merged)
 }

pkg/container/testdata/Dockerfile

@@ -0,0 +1,5 @@
+FROM scratch
+ENV PATH="/this/path/does/not/exists/anywhere:/this/either"
+ENV SOME_RANDOM_VAR=""
+ENV ANOTHER_ONE="BUT_I_HAVE_VALUE"
+ENV CONFLICT_VAR="I_EXIST_ONLY_HERE"

pkg/runner/run_context.go

@@ -156,6 +156,7 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			rc.stopJobContainer(),
 			rc.JobContainer.Create(rc.Config.ContainerCapAdd, rc.Config.ContainerCapDrop),
 			rc.JobContainer.Start(false),
+			rc.JobContainer.UpdateFromImageEnv(&rc.Env),
 			rc.JobContainer.UpdateFromEnv("/etc/environment", &rc.Env),
 			rc.JobContainer.Exec([]string{"mkdir", "-m", "0777", "-p", ActPath}, rc.Env, "root", ""),
 			rc.JobContainer.CopyDir(copyToPath, rc.Config.Workdir+string(filepath.Separator)+".", rc.Config.UseGitIgnore).IfBool(copyWorkspace),

pkg/runner/step_context.go

@@ -163,7 +163,11 @@ func (sc *StepContext) setupEnv(ctx context.Context) (ExpressionEvaluator, error
 	rc := sc.RunContext
 	sc.Env = sc.mergeEnv()
 	if sc.Env != nil {
-		err := rc.JobContainer.UpdateFromEnv(sc.Env["GITHUB_ENV"], &sc.Env)(ctx)
+		err := rc.JobContainer.UpdateFromImageEnv(&sc.Env)(ctx)
+		if err != nil {
+			return nil, err
+		}
+		err = rc.JobContainer.UpdateFromEnv(sc.Env["GITHUB_ENV"], &sc.Env)(ctx)
 		if err != nil {
 			return nil, err
 		}