$GITHUB_PATH support (#566)

* Regression in the .golangci.yml file * This looks like an even better fix to #451 The previous solution only prevented the `starting container process caused "exec: \"bash\"` error when someone added an "extra" path in the workflow using `::add-path` * Add support for >> $GITHUB_PATH * The newRunCommand has too high cyclomatic complexity * Add "linux/arm64" to new test * The cyclop linter was complaining so I extracted some funcs * Close some readers * Fix typo * fix: add missing composite function * Fix regress from merging * Keep the error messages as is * consolidate with master * Close the tar reader on defer * New way to get ContainerWorkdir * Remove arch from runner test * Separate the UpdateFromEnv and UpdateFromPath Co-authored-by: hackercat <me@hackerc.at>
2021-05-06 15:30:12 +02:00 · 2021-05-06 15:30:12 +02:00 · 92eec3a526
commit 92eec3a526
parent 8153dc92e5
8 changed files with 234 additions and 135 deletions
--- a/.golangci.yml
+++ b/.golangci.yml
@ -4,7 +4,7 @@ run:
 linters-settings:
  gocyclo:
    # minimal code complexity to report, 30 by default (but we recommend 10-20)
-    mi-complexity: 15
+    min-complexity: 15
  gocritic:
    disabled-checks:
      - ifElseChain
--- a/cmd/root.go
+++ b/cmd/root.go
@ -145,6 +145,7 @@ func readEnvs(path string, envs map[string]string) bool {
 	return false
 }

+//nolint:gocyclo
 func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []string) error {
 	return func(cmd *cobra.Command, args []string) error {
 		log.Debugf("Loading environment from %s", input.Envfile())
--- a/pkg/container/docker_run.go
+++ b/pkg/container/docker_run.go
@ -70,6 +70,7 @@ type Container interface {
 	Start(attach bool) common.Executor
 	Exec(command []string, env map[string]string) common.Executor
 	UpdateFromEnv(srcPath string, env *map[string]string) common.Executor
+	UpdateFromPath(env *map[string]string) common.Executor
 	Remove() common.Executor
 }

@ -155,6 +156,10 @@ func (cr *containerReference) UpdateFromEnv(srcPath string, env *map[string]stri
 	return cr.extractEnv(srcPath, env).IfNot(common.Dryrun)
 }

+func (cr *containerReference) UpdateFromPath(env *map[string]string) common.Executor {
+	return cr.extractPath(env).IfNot(common.Dryrun)
+}
+
 func (cr *containerReference) Exec(command []string, env map[string]string) common.Executor {
 	return common.NewPipelineExecutor(
 		cr.connect(),
@ -342,6 +347,7 @@ func (cr *containerReference) extractEnv(srcPath string, env *map[string]string)
 		if err != nil {
 			return nil
 		}
+		defer envTar.Close()
 		reader := tar.NewReader(envTar)
 		_, err = reader.Next()
 		if err != nil && err != io.EOF {
@ -376,6 +382,31 @@ func (cr *containerReference) extractEnv(srcPath string, env *map[string]string)
 	}
 }

+func (cr *containerReference) extractPath(env *map[string]string) common.Executor {
+	localEnv := *env
+	return func(ctx context.Context) error {
+		pathTar, _, err := cr.cli.CopyFromContainer(ctx, cr.id, localEnv["GITHUB_PATH"])
+		if err != nil {
+			return errors.WithStack(err)
+		}
+		defer pathTar.Close()
+
+		reader := tar.NewReader(pathTar)
+		_, err = reader.Next()
+		if err != nil && err != io.EOF {
+			return errors.WithStack(err)
+		}
+		s := bufio.NewScanner(reader)
+		for s.Scan() {
+			line := s.Text()
+			localEnv["PATH"] = fmt.Sprintf("%s:%s", localEnv["PATH"], line)
+		}
+
+		env = &localEnv
+		return nil
+	}
+}
+
 func (cr *containerReference) exec(cmd []string, env map[string]string) common.Executor {
 	return func(ctx context.Context) error {
 		logger := common.Logger(ctx)
@ -413,6 +444,8 @@ func (cr *containerReference) exec(cmd []string, env map[string]string) common.E
 		if err != nil {
 			return errors.WithStack(err)
 		}
+		defer resp.Close()
+
 		var outWriter io.Writer
 		outWriter = cr.input.Stdout
 		if outWriter == nil {
--- a/pkg/model/planner.go
+++ b/pkg/model/planner.go
@ -90,6 +90,7 @@ type WorkflowFiles struct {
 }

 // NewWorkflowPlanner will load a specific workflow, all workflows from a directory or all workflows from a directory and its subdirectories
+// nolint: gocyclo
 func NewWorkflowPlanner(path string, noWorkflowRecurse bool) (WorkflowPlanner, error) {
 	path, err := filepath.Abs(path)
 	if err != nil {
--- a/pkg/runner/run_context.go
+++ b/pkg/runner/run_context.go
@ -154,6 +154,10 @@ func (rc *RunContext) startJobContainer() common.Executor {
 				Name: "workflow/envs.txt",
 				Mode: 0644,
 				Body: "",
+			}, &container.FileEntry{
+				Name: "workflow/paths.txt",
+				Mode: 0644,
+				Body: "",
 			}),
 		)(ctx)
 	}
@ -628,6 +632,7 @@ func (rc *RunContext) withGithubEnv(env map[string]string) map[string]string {
 	github := rc.getGithubContext()
 	env["CI"] = "true"
 	env["GITHUB_ENV"] = "/tmp/workflow/envs.txt"
+	env["GITHUB_PATH"] = "/tmp/workflow/paths.txt"
 	env["GITHUB_WORKFLOW"] = github.Workflow
 	env["GITHUB_RUN_ID"] = github.RunID
 	env["GITHUB_RUN_NUMBER"] = github.RunNumber
--- a/pkg/runner/runner_test.go
+++ b/pkg/runner/runner_test.go
@ -107,6 +107,7 @@ func TestRunEvent(t *testing.T) {
 		{"testdata", "uses-composite", "push", "", platforms, ""},
 		{"testdata", "issue-597", "push", "", platforms, ""},
 		{"testdata", "issue-598", "push", "", platforms, ""},
+		{"testdata", "env-and-path", "push", "", platforms, ""},
 		// {"testdata", "issue-228", "push", "", platforms, ""}, // TODO [igni]: Remove this once everything passes

 		// single test for different architecture: linux/arm64
--- a/pkg/runner/step_context.go
+++ b/pkg/runner/step_context.go
@ -112,6 +112,7 @@ func (sc *StepContext) mergeEnv() map[string]string {
 		env = mergeMaps(rc.GetEnv(), step.GetEnv())
 	}

+	env["PATH"] = `/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin`
 	if (rc.ExtraPath != nil) && (len(rc.ExtraPath) > 0) {
 		env["PATH"] = strings.Join(rc.ExtraPath, `:`)
 	}
@ -134,6 +135,10 @@ func (sc *StepContext) setupEnv(ctx context.Context) (ExpressionEvaluator, error
 		if err != nil {
 			return nil, err
 		}
+		err = rc.JobContainer.UpdateFromPath(&sc.Env)(ctx)
+		if err != nil {
+			return nil, err
+		}
 	}
 	evaluator := sc.NewExpressionEvaluator()
 	sc.interpolateEnv(evaluator)
@ -390,14 +395,7 @@ func (sc *StepContext) runAction(actionDir string, actionPath string) common.Exe
 	return func(ctx context.Context) error {
 		action := sc.Action
 		log.Debugf("About to run action %v", action)
-		for inputID, input := range action.Inputs {
-			envKey := regexp.MustCompile("[^A-Z0-9-]").ReplaceAllString(strings.ToUpper(inputID), "_")
-			envKey = fmt.Sprintf("INPUT_%s", envKey)
-			if _, ok := sc.Env[envKey]; !ok {
-				sc.Env[envKey] = rc.ExprEval.Interpolate(input.Default)
-			}
-		}
-
+		sc.populateEnvsFromInput(action, rc)
 		actionLocation := ""
 		if actionPath != "" {
 			actionLocation = path.Join(actionDir, actionPath)
@ -434,133 +432,9 @@ func (sc *StepContext) runAction(actionDir string, actionPath string) common.Exe
 			log.Debugf("executing remote job container: %s", containerArgs)
 			return rc.execJobContainer(containerArgs, sc.Env)(ctx)
 		case model.ActionRunsUsingDocker:
-			var prepImage common.Executor
-			var image string
-			if strings.HasPrefix(action.Runs.Image, "docker://") {
-				image = strings.TrimPrefix(action.Runs.Image, "docker://")
-			} else {
-				image = fmt.Sprintf("%s:%s", regexp.MustCompile("[^a-zA-Z0-9]").ReplaceAllString(actionName, "-"), "latest")
-				image = fmt.Sprintf("act-%s", strings.TrimLeft(image, "-"))
-				image = strings.ToLower(image)
-				contextDir := filepath.Join(actionDir, actionPath, action.Runs.Main)
-
-				anyArchExists, err := container.ImageExistsLocally(ctx, image, "any")
-				if err != nil {
-					return err
-				}
-
-				correctArchExists, err := container.ImageExistsLocally(ctx, image, rc.Config.ContainerArchitecture)
-				if err != nil {
-					return err
-				}
-
-				if anyArchExists && !correctArchExists {
-					wasRemoved, err := container.RemoveImage(ctx, image, true, true)
-					if err != nil {
-						return err
-					}
-					if !wasRemoved {
-						return fmt.Errorf("failed to remove image '%s'", image)
-					}
-				}
-
-				if !correctArchExists {
-					log.Debugf("image '%s' for architecture '%s' will be built from context '%s", image, rc.Config.ContainerArchitecture, contextDir)
-					prepImage = container.NewDockerBuildExecutor(container.NewDockerBuildExecutorInput{
-						ContextDir: contextDir,
-						ImageTag:   image,
-						Platform:   rc.Config.ContainerArchitecture,
-					})
-				} else {
-					log.Debugf("image '%s' for architecture '%s' already exists", image, rc.Config.ContainerArchitecture)
-				}
-			}
-
-			cmd, err := shellquote.Split(step.With["args"])
-			if err != nil {
-				return err
-			}
-			if len(cmd) == 0 {
-				cmd = action.Runs.Args
-			}
-			entrypoint := strings.Fields(step.With["entrypoint"])
-			if len(entrypoint) == 0 {
-				entrypoint = action.Runs.Entrypoint
-			}
-			stepContainer := sc.newStepContainer(ctx, image, cmd, entrypoint)
-			return common.NewPipelineExecutor(
-				prepImage,
-				stepContainer.Pull(rc.Config.ForcePull),
-				stepContainer.Remove().IfBool(!rc.Config.ReuseContainers),
-				stepContainer.Create(),
-				stepContainer.Start(true),
-			).Finally(
-				stepContainer.Remove().IfBool(!rc.Config.ReuseContainers),
-			)(ctx)
+			return sc.execAsDocker(ctx, action, actionName, actionDir, actionPath, rc, step)
 		case model.ActionRunsUsingComposite:
-			err := maybeCopyToActionDir()
-			if err != nil {
-				return err
-			}
-			for outputName, output := range action.Outputs {
-				re := regexp.MustCompile(`\${{ steps\.([a-zA-Z_][a-zA-Z0-9_-]+)\.outputs\.([a-zA-Z_][a-zA-Z0-9_-]+) }}`)
-				matches := re.FindStringSubmatch(output.Value)
-				if len(matches) > 2 {
-					if sc.RunContext.OutputMappings == nil {
-						sc.RunContext.OutputMappings = make(map[MappableOutput]MappableOutput)
-					}
-
-					k := MappableOutput{StepID: matches[1], OutputName: matches[2]}
-					v := MappableOutput{StepID: step.ID, OutputName: outputName}
-					sc.RunContext.OutputMappings[k] = v
-				}
-			}
-
-			var executors []common.Executor
-			stepID := 0
-			for _, compositeStep := range action.Runs.Steps {
-				stepClone := compositeStep
-				// Take a copy of the run context structure (rc is a pointer)
-				// Then take the address of the new structure
-				rcCloneStr := *rc
-				rcClone := &rcCloneStr
-				if stepClone.ID == "" {
-					stepClone.ID = fmt.Sprintf("composite-%d", stepID)
-					stepID++
-				}
-				rcClone.CurrentStep = stepClone.ID
-
-				if err := compositeStep.Validate(); err != nil {
-					return err
-				}
-
-				// Setup the outputs for the composite steps
-				if _, ok := rcClone.StepResults[stepClone.ID]; !ok {
-					rcClone.StepResults[stepClone.ID] = &stepResult{
-						Success: true,
-						Outputs: make(map[string]string),
-					}
-				}
-
-				stepClone.Run = strings.ReplaceAll(stepClone.Run, "${{ github.action_path }}", filepath.Join(containerActionDir, actionName))
-
-				stepContext := StepContext{
-					RunContext: rcClone,
-					Step:       &stepClone,
-					Env:        mergeMaps(sc.Env, stepClone.Env),
-				}
-
-				// Interpolate the outer inputs into the composite step with items
-				exprEval := sc.NewExpressionEvaluator()
-				for k, v := range stepContext.Step.With {
-					if strings.Contains(v, "inputs") {
-						stepContext.Step.With[k] = exprEval.Interpolate(v)
-					}
-				}
-
-				executors = append(executors, stepContext.Executor())
-			}
-			return common.NewPipelineExecutor(executors...)(ctx)
+			return sc.execAsComposite(ctx, step, actionDir, rc, containerActionDir, actionName, actionPath, action, maybeCopyToActionDir)
 		default:
 			return fmt.Errorf(fmt.Sprintf("The runs.using key must be one of: %v, got %s", []string{
 				model.ActionRunsUsingDocker,
@ -571,6 +445,149 @@ func (sc *StepContext) runAction(actionDir string, actionPath string) common.Exe
 	}
 }

+func (sc *StepContext) execAsDocker(ctx context.Context, action *model.Action, actionName string, actionDir string, actionPath string, rc *RunContext, step *model.Step) error {
+	var prepImage common.Executor
+	var image string
+	if strings.HasPrefix(action.Runs.Image, "docker://") {
+		image = strings.TrimPrefix(action.Runs.Image, "docker://")
+	} else {
+		image = fmt.Sprintf("%s:%s", regexp.MustCompile("[^a-zA-Z0-9]").ReplaceAllString(actionName, "-"), "latest")
+		image = fmt.Sprintf("act-%s", strings.TrimLeft(image, "-"))
+		image = strings.ToLower(image)
+		contextDir := filepath.Join(actionDir, actionPath, action.Runs.Main)
+
+		anyArchExists, err := container.ImageExistsLocally(ctx, image, "any")
+		if err != nil {
+			return err
+		}
+
+		correctArchExists, err := container.ImageExistsLocally(ctx, image, rc.Config.ContainerArchitecture)
+		if err != nil {
+			return err
+		}
+
+		if anyArchExists && !correctArchExists {
+			wasRemoved, err := container.RemoveImage(ctx, image, true, true)
+			if err != nil {
+				return err
+			}
+			if !wasRemoved {
+				return fmt.Errorf("failed to remove image '%s'", image)
+			}
+		}
+
+		if !correctArchExists {
+			log.Debugf("image '%s' for architecture '%s' will be built from context '%s", image, rc.Config.ContainerArchitecture, contextDir)
+			prepImage = container.NewDockerBuildExecutor(container.NewDockerBuildExecutorInput{
+				ContextDir: contextDir,
+				ImageTag:   image,
+				Platform:   rc.Config.ContainerArchitecture,
+			})
+		} else {
+			log.Debugf("image '%s' for architecture '%s' already exists", image, rc.Config.ContainerArchitecture)
+		}
+	}
+
+	cmd, err := shellquote.Split(step.With["args"])
+	if err != nil {
+		return err
+	}
+	if len(cmd) == 0 {
+		cmd = action.Runs.Args
+	}
+	entrypoint := strings.Fields(step.With["entrypoint"])
+	if len(entrypoint) == 0 {
+		entrypoint = action.Runs.Entrypoint
+	}
+	stepContainer := sc.newStepContainer(ctx, image, cmd, entrypoint)
+	return common.NewPipelineExecutor(
+		prepImage,
+		stepContainer.Pull(rc.Config.ForcePull),
+		stepContainer.Remove().IfBool(!rc.Config.ReuseContainers),
+		stepContainer.Create(),
+		stepContainer.Start(true),
+	).Finally(
+		stepContainer.Remove().IfBool(!rc.Config.ReuseContainers),
+	)(ctx)
+}
+
+func (sc *StepContext) execAsComposite(ctx context.Context, step *model.Step, _ string, rc *RunContext, containerActionDir string, actionName string, _ string, action *model.Action, maybeCopyToActionDir func() error) error {
+	err := maybeCopyToActionDir()
+
+	if err != nil {
+		return err
+	}
+	for outputName, output := range action.Outputs {
+		re := regexp.MustCompile(`\${{ steps\.([a-zA-Z_][a-zA-Z0-9_-]+)\.outputs\.([a-zA-Z_][a-zA-Z0-9_-]+) }}`)
+		matches := re.FindStringSubmatch(output.Value)
+		if len(matches) > 2 {
+			if sc.RunContext.OutputMappings == nil {
+				sc.RunContext.OutputMappings = make(map[MappableOutput]MappableOutput)
+			}
+
+			k := MappableOutput{StepID: matches[1], OutputName: matches[2]}
+			v := MappableOutput{StepID: step.ID, OutputName: outputName}
+			sc.RunContext.OutputMappings[k] = v
+		}
+	}
+
+	executors := make([]common.Executor, 0, len(action.Runs.Steps))
+	stepID := 0
+	for _, compositeStep := range action.Runs.Steps {
+		stepClone := compositeStep
+		// Take a copy of the run context structure (rc is a pointer)
+		// Then take the address of the new structure
+		rcCloneStr := *rc
+		rcClone := &rcCloneStr
+		if stepClone.ID == "" {
+			stepClone.ID = fmt.Sprintf("composite-%d", stepID)
+			stepID++
+		}
+		rcClone.CurrentStep = stepClone.ID
+
+		if err := compositeStep.Validate(); err != nil {
+			return err
+		}
+
+		// Setup the outputs for the composite steps
+		if _, ok := rcClone.StepResults[stepClone.ID]; !ok {
+			rcClone.StepResults[stepClone.ID] = &stepResult{
+				Success: true,
+				Outputs: make(map[string]string),
+			}
+		}
+
+		stepClone.Run = strings.ReplaceAll(stepClone.Run, "${{ github.action_path }}", filepath.Join(containerActionDir, actionName))
+
+		stepContext := StepContext{
+			RunContext: rcClone,
+			Step:       &stepClone,
+			Env:        mergeMaps(sc.Env, stepClone.Env),
+		}
+
+		// Interpolate the outer inputs into the composite step with items
+		exprEval := sc.NewExpressionEvaluator()
+		for k, v := range stepContext.Step.With {
+			if strings.Contains(v, "inputs") {
+				stepContext.Step.With[k] = exprEval.Interpolate(v)
+			}
+		}
+
+		executors = append(executors, stepContext.Executor())
+	}
+	return common.NewPipelineExecutor(executors...)(ctx)
+}
+
+func (sc *StepContext) populateEnvsFromInput(action *model.Action, rc *RunContext) {
+	for inputID, input := range action.Inputs {
+		envKey := regexp.MustCompile("[^A-Z0-9-]").ReplaceAllString(strings.ToUpper(inputID), "_")
+		envKey = fmt.Sprintf("INPUT_%s", envKey)
+		if _, ok := sc.Env[envKey]; !ok {
+			sc.Env[envKey] = rc.ExprEval.Interpolate(input.Default)
+		}
+	}
+}
+
 type remoteAction struct {
 	URL  string
 	Org  string
--- a/pkg/runner/testdata/env-and-path/push.yaml
+++ b/pkg/runner/testdata/env-and-path/push.yaml
@ -0,0 +1,41 @@
+name: env-and-path
+on: push
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - name: "Append to $GITHUB_PATH"
+        run: |
+          echo "$HOME/someFolder" >> $GITHUB_PATH
+      - name: "Append some more to $GITHUB_PATH"
+        run: |
+          echo "$HOME/someOtherFolder" >> $GITHUB_PATH
+      - name: "Check PATH"
+        run: |
+          if [[ ! "${PATH}" =~ .*"$HOME/"someFolder.*"$HOME/"someOtherFolder ]]; then
+            echo "${PATH} doesn't match .*someFolder.*someOtherFolder"
+            exit 1
+          fi
+      - name: "Write single line env to $GITHUB_ENV"
+        run: |
+          echo "KEY=value" >> $GITHUB_ENV
+      - name: "Check single line env"
+        run: |
+          if [[ "${KEY}" != "value" ]]; then
+            echo "${KEY} dosen't == 'value'"
+            exit 1
+          fi
+      - name: "Write multiline env to $GITHUB_ENV"
+        run: |
+          echo 'KEY2<<EOF' >> $GITHUB_ENV
+                echo value2 >> $GITHUB_ENV
+                echo 'EOF' >> $GITHUB_ENV
+      - name: "Check multiline line env"
+        run: |
+          if [[ "${KEY2}" != "value2" ]]; then
+            echo "${KEY2} dosen't == 'value'"
+            exit 1
+          fi
+
+