refactor: fix add-path / GITHUB_PATH commands (nektos#1472)

* fix: add-path / GITHUB_PATH commands * Disable old code * fix: missing mock * Update tests * fix tests * UpdateExtraPath skip on dryrun * patch test Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
xing · Dec 9, 2022 · d281230 · d281230
1 parent 7d99512
commit d281230
Show file tree

Hide file tree

Showing 12 changed files with 95 additions and 46 deletions.
diff --git a/pkg/runner/action.go b/pkg/runner/action.go
@@ -154,6 +154,8 @@ func runActionImpl(step actionStep, actionDir string, remoteAction *remoteAction
 			containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Main)}
 			logger.Debugf("executing remote job container: %s", containerArgs)
 
+			rc.ApplyExtraPath(step.getEnv())
+
 			return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
 		case model.ActionRunsUsingDocker:
 			location := actionLocation
@@ -486,6 +488,8 @@ func runPreStep(step actionStep) common.Executor {
 			containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Pre)}
 			logger.Debugf("executing remote job container: %s", containerArgs)
 
+			rc.ApplyExtraPath(step.getEnv())
+
 			return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
 
 		case model.ActionRunsUsingComposite:
@@ -573,6 +577,8 @@ func runPostStep(step actionStep) common.Executor {
 			containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Post)}
 			logger.Debugf("executing remote job container: %s", containerArgs)
 
+			rc.ApplyExtraPath(step.getEnv())
+
 			return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
 
 		case model.ActionRunsUsingComposite:

diff --git a/pkg/runner/command.go b/pkg/runner/command.go
@@ -101,7 +101,13 @@ func (rc *RunContext) setOutput(ctx context.Context, kvPairs map[string]string,
 }
 func (rc *RunContext) addPath(ctx context.Context, arg string) {
 	common.Logger(ctx).Infof("  \U00002699  ::add-path:: %s", arg)
-	rc.ExtraPath = append(rc.ExtraPath, arg)
+	extraPath := []string{arg}
+	for _, v := range rc.ExtraPath {
+		if v != arg {
+			extraPath = append(extraPath, v)
+		}
+	}
+	rc.ExtraPath = extraPath
 }
 
 func parseKeyValuePairs(kvPairs string, separator string) map[string]string {

diff --git a/pkg/runner/command_test.go b/pkg/runner/command_test.go
@@ -64,7 +64,7 @@ func TestAddpath(t *testing.T) {
 	a.Equal("/zoo", rc.ExtraPath[0])
 
 	handler("::add-path::/boo\n")
-	a.Equal("/boo", rc.ExtraPath[1])
+	a.Equal("/boo", rc.ExtraPath[0])
 }
 
 func TestStopCommands(t *testing.T) {
@@ -102,7 +102,7 @@ func TestAddpathADO(t *testing.T) {
 	a.Equal("/zoo", rc.ExtraPath[0])
 
 	handler("##[add-path]/boo\n")
-	a.Equal("/boo", rc.ExtraPath[1])
+	a.Equal("/boo", rc.ExtraPath[0])
 }
 
 func TestAddmask(t *testing.T) {

diff --git a/pkg/runner/container_mock_test.go b/pkg/runner/container_mock_test.go
@@ -2,6 +2,7 @@ package runner
 
 import (
 	"context"
+	"io"
 
 	"github.com/nektos/act/pkg/common"
 	"github.com/nektos/act/pkg/container"
@@ -63,7 +64,17 @@ func (cm *containerMock) CopyDir(destPath string, srcPath string, useGitIgnore b
 	args := cm.Called(destPath, srcPath, useGitIgnore)
 	return args.Get(0).(func(context.Context) error)
 }
+
 func (cm *containerMock) Exec(command []string, env map[string]string, user, workdir string) common.Executor {
 	args := cm.Called(command, env, user, workdir)
 	return args.Get(0).(func(context.Context) error)
 }
+
+func (cm *containerMock) GetContainerArchive(ctx context.Context, srcPath string) (io.ReadCloser, error) {
+	args := cm.Called(ctx, srcPath)
+	err, hasErr := args.Get(1).(error)
+	if !hasErr {
+		err = nil
+	}
+	return args.Get(0).(io.ReadCloser), err
+}
diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go
@@ -1,12 +1,15 @@
 package runner
 
 import (
+	"archive/tar"
+	"bufio"
 	"context"
 	"crypto/rand"
 	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -188,10 +191,6 @@ func (rc *RunContext) startHostEnvironment() common.Executor {
 				Name: "workflow/envs.txt",
 				Mode: 0666,
 				Body: "",
-			}, &container.FileEntry{
-				Name: "workflow/paths.txt",
-				Mode: 0666,
-				Body: "",
 			}),
 		)(ctx)
 	}
@@ -277,10 +276,6 @@ func (rc *RunContext) startJobContainer() common.Executor {
 				Name: "workflow/envs.txt",
 				Mode: 0666,
 				Body: "",
-			}, &container.FileEntry{
-				Name: "workflow/paths.txt",
-				Mode: 0666,
-				Body: "",
 			}),
 		)(ctx)
 	}
@@ -292,6 +287,41 @@ func (rc *RunContext) execJobContainer(cmd []string, env map[string]string, user
 	}
 }
 
+func (rc *RunContext) ApplyExtraPath(env *map[string]string) {
+	if rc.ExtraPath != nil && len(rc.ExtraPath) > 0 {
+		path := rc.JobContainer.GetPathVariableName()
+		if (*env)[path] == "" {
+			(*env)[path] = rc.JobContainer.DefaultPathVariable()
+		}
+		(*env)[path] = rc.JobContainer.JoinPathVariable(append(rc.ExtraPath, (*env)[path])...)
+	}
+}
+
+func (rc *RunContext) UpdateExtraPath(ctx context.Context, githubEnvPath string) error {
+	if common.Dryrun(ctx) {
+		return nil
+	}
+	pathTar, err := rc.JobContainer.GetContainerArchive(ctx, githubEnvPath)
+	if err != nil {
+		return err
+	}
+	defer pathTar.Close()
+
+	reader := tar.NewReader(pathTar)
+	_, err = reader.Next()
+	if err != nil && err != io.EOF {
+		return err
+	}
+	s := bufio.NewScanner(reader)
+	for s.Scan() {
+		line := s.Text()
+		if len(line) > 0 {
+			rc.addPath(ctx, line)
+		}
+	}
+	return nil
+}
+
 // stopJobContainer removes the job container (if it exists) and its volume (if it exists) if !rc.Config.ReuseContainers
 func (rc *RunContext) stopJobContainer() common.Executor {
 	return func(ctx context.Context) error {
@@ -639,7 +669,6 @@ func nestedMapLookup(m map[string]interface{}, ks ...string) (rval interface{})
 func (rc *RunContext) withGithubEnv(ctx context.Context, github *model.GithubContext, env map[string]string) map[string]string {
 	env["CI"] = "true"
 	env["GITHUB_ENV"] = rc.JobContainer.GetActPath() + "/workflow/envs.txt"
-	env["GITHUB_PATH"] = rc.JobContainer.GetActPath() + "/workflow/paths.txt"
 	env["GITHUB_WORKFLOW"] = github.Workflow
 	env["GITHUB_RUN_ID"] = github.RunID
 	env["GITHUB_RUN_NUMBER"] = github.RunNumber

diff --git a/pkg/runner/step.go b/pkg/runner/step.go
@@ -100,14 +100,19 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo
 		actPath := rc.JobContainer.GetActPath()
 		outputFileCommand := path.Join("workflow", "outputcmd.txt")
 		stateFileCommand := path.Join("workflow", "statecmd.txt")
+		pathFileCommand := path.Join("workflow", "pathcmd.txt")
 		(*step.getEnv())["GITHUB_OUTPUT"] = path.Join(actPath, outputFileCommand)
 		(*step.getEnv())["GITHUB_STATE"] = path.Join(actPath, stateFileCommand)
+		(*step.getEnv())["GITHUB_PATH"] = path.Join(actPath, pathFileCommand)
 		_ = rc.JobContainer.Copy(actPath, &container.FileEntry{
 			Name: outputFileCommand,
 			Mode: 0666,
 		}, &container.FileEntry{
 			Name: stateFileCommand,
 			Mode: 0666,
+		}, &container.FileEntry{
+			Name: pathFileCommand,
+			Mode: 0666,
 		})(ctx)
 
 		err = executor(ctx)
@@ -151,6 +156,10 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo
 		for k, v := range output {
 			rc.setOutput(ctx, map[string]string{"name": k}, v)
 		}
+		err = rc.UpdateExtraPath(ctx, path.Join(actPath, pathFileCommand))
+		if err != nil {
+			return err
+		}
 		if orgerr != nil {
 			return orgerr
 		}
@@ -170,10 +179,6 @@ func setupEnv(ctx context.Context, step step) error {
 	if err != nil {
 		return err
 	}
-	err = rc.JobContainer.UpdateFromPath(step.getEnv())(ctx)
-	if err != nil {
-		return err
-	}
 	// merge step env last, since it should not be overwritten
 	mergeIntoMap(step.getEnv(), step.getStepModel().GetEnv())
 
@@ -209,14 +214,6 @@ func mergeEnv(ctx context.Context, step step) {
 		mergeIntoMap(env, rc.GetEnv())
 	}
 
-	path := rc.JobContainer.GetPathVariableName()
-	if (*env)[path] == "" {
-		(*env)[path] = rc.JobContainer.DefaultPathVariable()
-	}
-	if rc.ExtraPath != nil && len(rc.ExtraPath) > 0 {
-		(*env)[path] = rc.JobContainer.JoinPathVariable(append(rc.ExtraPath, (*env)[path])...)
-	}
-
 	rc.withGithubEnv(ctx, step.getGithubContext(ctx), *env)
 }
 

diff --git a/pkg/runner/step_action_local_test.go b/pkg/runner/step_action_local_test.go
@@ -1,7 +1,9 @@
 package runner
 
 import (
+	"bytes"
 	"context"
+	"io"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -75,10 +77,6 @@ func TestStepActionLocalTest(t *testing.T) {
 		return nil
 	})
 
-	cm.On("UpdateFromPath", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
-		return nil
-	})
-
 	cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error {
 		return nil
 	})
@@ -91,6 +89,8 @@ func TestStepActionLocalTest(t *testing.T) {
 		return nil
 	})
 
+	cm.On("GetContainerArchive", ctx, "/var/run/act/workflow/pathcmd.txt").Return(io.NopCloser(&bytes.Buffer{}), nil)
+
 	salm.On("runAction", sal, filepath.Clean("/tmp/path/to/action"), (*remoteAction)(nil)).Return(func(ctx context.Context) error {
 		return nil
 	})
@@ -280,7 +280,6 @@ func TestStepActionLocalPost(t *testing.T) {
 			if tt.mocks.env {
 				cm.On("UpdateFromImageEnv", &sal.env).Return(func(ctx context.Context) error { return nil })
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", &sal.env).Return(func(ctx context.Context) error { return nil })
-				cm.On("UpdateFromPath", &sal.env).Return(func(ctx context.Context) error { return nil })
 			}
 			if tt.mocks.exec {
 				suffixMatcher := func(suffix string) interface{} {
@@ -301,6 +300,8 @@ func TestStepActionLocalPost(t *testing.T) {
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/outputcmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 					return nil
 				})
+
+				cm.On("GetContainerArchive", ctx, "/var/run/act/workflow/pathcmd.txt").Return(io.NopCloser(&bytes.Buffer{}), nil)
 			}
 
 			err := sal.post()(ctx)

diff --git a/pkg/runner/step_action_remote_test.go b/pkg/runner/step_action_remote_test.go
@@ -1,8 +1,10 @@
 package runner
 
 import (
+	"bytes"
 	"context"
 	"errors"
+	"io"
 	"strings"
 	"testing"
 
@@ -166,7 +168,6 @@ func TestStepActionRemote(t *testing.T) {
 			if tt.mocks.env {
 				cm.On("UpdateFromImageEnv", &sar.env).Return(func(ctx context.Context) error { return nil })
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", &sar.env).Return(func(ctx context.Context) error { return nil })
-				cm.On("UpdateFromPath", &sar.env).Return(func(ctx context.Context) error { return nil })
 			}
 			if tt.mocks.read {
 				sarm.On("readAction", sar.Step, suffixMatcher("act/remote-action@v1"), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil)
@@ -185,6 +186,8 @@ func TestStepActionRemote(t *testing.T) {
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/outputcmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 					return nil
 				})
+
+				cm.On("GetContainerArchive", ctx, "/var/run/act/workflow/pathcmd.txt").Return(io.NopCloser(&bytes.Buffer{}), nil)
 			}
 
 			err := sar.pre()(ctx)
@@ -592,7 +595,6 @@ func TestStepActionRemotePost(t *testing.T) {
 			if tt.mocks.env {
 				cm.On("UpdateFromImageEnv", &sar.env).Return(func(ctx context.Context) error { return nil })
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/envs.txt", &sar.env).Return(func(ctx context.Context) error { return nil })
-				cm.On("UpdateFromPath", &sar.env).Return(func(ctx context.Context) error { return nil })
 			}
 			if tt.mocks.exec {
 				cm.On("Exec", []string{"node", "/var/run/act/actions/remote-action@v1/post.js"}, sar.env, "", "").Return(func(ctx context.Context) error { return tt.err })
@@ -608,6 +610,8 @@ func TestStepActionRemotePost(t *testing.T) {
 				cm.On("UpdateFromEnv", "/var/run/act/workflow/outputcmd.txt", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
 					return nil
 				})
+
+				cm.On("GetContainerArchive", ctx, "/var/run/act/workflow/pathcmd.txt").Return(io.NopCloser(&bytes.Buffer{}), nil)
 			}
 
 			err := sar.post()(ctx)

diff --git a/pkg/runner/step_docker_test.go b/pkg/runner/step_docker_test.go
@@ -1,7 +1,9 @@
 package runner
 
 import (
+	"bytes"
 	"context"
+	"io"
 	"testing"
 
 	"github.com/nektos/act/pkg/container"
@@ -63,10 +65,6 @@ func TestStepDockerMain(t *testing.T) {
 		return nil
 	})
 
-	cm.On("UpdateFromPath", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
-		return nil
-	})
-
 	cm.On("Pull", false).Return(func(ctx context.Context) error {
 		return nil
 	})
@@ -99,6 +97,8 @@ func TestStepDockerMain(t *testing.T) {
 		return nil
 	})
 
+	cm.On("GetContainerArchive", ctx, "/var/run/act/workflow/pathcmd.txt").Return(io.NopCloser(&bytes.Buffer{}), nil)
+
 	err := sd.main()(ctx)
 	assert.Nil(t, err)
 

diff --git a/pkg/runner/step_run.go b/pkg/runner/step_run.go
@@ -30,6 +30,7 @@ func (sr *stepRun) main() common.Executor {
 	return runStepExecutor(sr, stepStageMain, common.NewPipelineExecutor(
 		sr.setupShellCommandExecutor(),
 		func(ctx context.Context) error {
+			sr.getRunContext().ApplyExtraPath(&sr.env)
 			return sr.getRunContext().JobContainer.Exec(sr.cmd, sr.env, "", sr.Step.WorkingDirectory)(ctx)
 		},
 	))

diff --git a/pkg/runner/step_run_test.go b/pkg/runner/step_run_test.go
@@ -1,7 +1,9 @@
 package runner
 
 import (
+	"bytes"
 	"context"
+	"io"
 	"testing"
 
 	"github.com/nektos/act/pkg/container"
@@ -61,10 +63,6 @@ func TestStepRun(t *testing.T) {
 		return nil
 	})
 
-	cm.On("UpdateFromPath", mock.AnythingOfType("*map[string]string")).Return(func(ctx context.Context) error {
-		return nil
-	})
-
 	cm.On("Copy", "/var/run/act", mock.AnythingOfType("[]*container.FileEntry")).Return(func(ctx context.Context) error {
 		return nil
 	})
@@ -79,6 +77,8 @@ func TestStepRun(t *testing.T) {
 
 	ctx := context.Background()
 
+	cm.On("GetContainerArchive", ctx, "/var/run/act/workflow/pathcmd.txt").Return(io.NopCloser(&bytes.Buffer{}), nil)
+
 	err := sr.main()(ctx)
 	assert.Nil(t, err)