From 6b56e0d84973ed1fda7b6e31f6403cae31aa5032 Mon Sep 17 00:00:00 2001 From: Markus Wolf Date: Tue, 29 Mar 2022 15:08:05 +0200 Subject: [PATCH] refactor: remove composite action runcontext workaround The RunContext is cloned to execute a composite action with all its steps in a similar context. This required some workaround, since the command handler has kept a reference to the original RunContext. This is solved now, by replacing the docker LogWriter with a proper scoped LogWriter. This prepares for a simpler setup of composite actions to be able to create and re-create the composite RunContext for pre/main/post action steps. --- pkg/container/docker_run.go | 11 +++ pkg/runner/action.go | 169 +++++++++++++++++++++++------------- pkg/runner/action_test.go | 20 +---- pkg/runner/expression.go | 14 +-- pkg/runner/run_context.go | 53 ----------- pkg/runner/step_run.go | 4 +- 6 files changed, 123 insertions(+), 148 deletions(-) diff --git a/pkg/container/docker_run.go b/pkg/container/docker_run.go index 5334bfe0683..1948c31719b 100644 --- a/pkg/container/docker_run.go +++ b/pkg/container/docker_run.go @@ -77,6 +77,7 @@ type Container interface { UpdateFromPath(env *map[string]string) common.Executor Remove() common.Executor Close() common.Executor + ReplaceLogWriter(io.Writer, io.Writer) (io.Writer, io.Writer) } // NewContainer creates a reference to a container @@ -195,6 +196,16 @@ func (cr *containerReference) Remove() common.Executor { ).IfNot(common.Dryrun) } +func (cr *containerReference) ReplaceLogWriter(stdout io.Writer, stderr io.Writer) (io.Writer, io.Writer) { + out := cr.input.Stdout + err := cr.input.Stderr + + cr.input.Stdout = stdout + cr.input.Stderr = stderr + + return out, err +} + type containerReference struct { cli *client.Client id string diff --git a/pkg/runner/action.go b/pkg/runner/action.go index 6a8d31ad07f..7001adf7dec 100644 --- a/pkg/runner/action.go +++ b/pkg/runner/action.go @@ -102,26 +102,9 @@ func runActionImpl(step actionStep, actionDir string, remoteAction *remoteAction rc := step.getRunContext() stepModel := step.getStepModel() return func(ctx context.Context) error { - // Backup the parent composite action path and restore it on continue - parentActionPath := rc.ActionPath - parentActionRepository := rc.ActionRepository - parentActionRef := rc.ActionRef - defer func() { - rc.ActionPath = parentActionPath - rc.ActionRef = parentActionRef - rc.ActionRepository = parentActionRepository - }() - actionPath := "" - if remoteAction != nil { - rc.ActionRef = remoteAction.Ref - rc.ActionRepository = remoteAction.Repo - if remoteAction.Path != "" { - actionPath = remoteAction.Path - } - } else { - rc.ActionRef = "" - rc.ActionRepository = "" + if remoteAction != nil && remoteAction.Path != "" { + actionPath = remoteAction.Path } action := step.getActionModel() log.Debugf("About to run action %v", action) @@ -134,7 +117,6 @@ func runActionImpl(step actionStep, actionDir string, remoteAction *remoteAction log.Debugf("type=%v actionDir=%s actionPath=%s workdir=%s actionCacheDir=%s actionName=%s containerActionDir=%s", stepModel.Type(), actionDir, actionPath, rc.Config.Workdir, rc.ActionCacheDir(), actionName, containerActionDir) maybeCopyToActionDir := func() error { - rc.ActionPath = containerActionDir if stepModel.Type() != model.StepTypeUsesActionRemote { return nil } @@ -170,7 +152,7 @@ func runActionImpl(step actionStep, actionDir string, remoteAction *remoteAction if err := maybeCopyToActionDir(); err != nil { return err } - return execAsComposite(step)(ctx) + return execAsComposite(step, containerActionDir)(ctx) default: return fmt.Errorf(fmt.Sprintf("The runs.using key must be one of: %v, got %s", []string{ model.ActionRunsUsingDocker, @@ -359,7 +341,7 @@ func newStepContainer(ctx context.Context, step step, image string, cmd []string return stepContainer } -func execAsComposite(step actionStep) common.Executor { +func execAsComposite(step actionStep, containerActionDir string) common.Executor { rc := step.getRunContext() action := step.getActionModel() @@ -370,9 +352,10 @@ func execAsComposite(step actionStep) common.Executor { return err } } + + eval := rc.NewExpressionEvaluator() + inputs := make(map[string]interface{}) - eval := step.getRunContext().NewExpressionEvaluator() - // Set Defaults for k, input := range action.Inputs { inputs[k] = eval.Interpolate(input.Default) } @@ -381,63 +364,125 @@ func execAsComposite(step actionStep) common.Executor { inputs[k] = eval.Interpolate(v) } } - // Doesn't work with the command processor has a pointer to the original rc - // compositerc := rc.Clone() - // Workaround start - backup := *rc - defer func() { *rc = backup }() - *rc = *rc.Clone() - scriptName := backup.CurrentStep - for rcs := &backup; rcs.Parent != nil; rcs = rcs.Parent { - scriptName = fmt.Sprintf("%s-composite-%s", rcs.Parent.CurrentStep, scriptName) + + env := make(map[string]string) + for k, v := range step.getStepModel().Environment() { + env[k] = eval.Interpolate(v) } - compositerc := rc - compositerc.Parent = &RunContext{ - CurrentStep: scriptName, + + var actionPath string + var actionRepository string + var actionRef string + if step.getStepModel().Type() == model.StepTypeUsesActionRemote { + ra := newRemoteAction(step.getStepModel().Uses) + actionPath = containerActionDir + actionRepository = ra.Repo + actionRef = ra.Ref + } else { + actionPath = containerActionDir + actionRepository = "" + actionRef = "" } - // Workaround end - compositerc.Composite = action - envToEvaluate := mergeMaps(compositerc.Env, step.getStepModel().Environment()) - compositerc.Env = make(map[string]string) - // origEnvMap: is used to pass env changes back to parent runcontext - origEnvMap := make(map[string]string) - for k, v := range envToEvaluate { - ev := eval.Interpolate(v) - origEnvMap[k] = ev - compositerc.Env[k] = ev + + // run with the global config but without secrets + configCopy := *rc.Config + configCopy.Secrets = nil + + // create a run context for the composite action to run in + compositerc := &RunContext{ + Name: rc.Name, + Run: &model.Run{ + JobID: "composite-job", + Workflow: &model.Workflow{ + Name: rc.Run.Workflow.Name, + Jobs: map[string]*model.Job{ + "composite-job": {}, + }, + }, + }, + Config: &configCopy, + StepResults: map[string]*model.StepResult{}, + JobContainer: rc.JobContainer, + Inputs: inputs, + ActionPath: actionPath, + ActionRepository: actionRepository, + ActionRef: actionRef, + Env: env, } - compositerc.Inputs = inputs - compositerc.ExprEval = compositerc.NewExpressionEvaluator() - err := compositerc.compositeExecutor()(ctx) + // We need to inject a composite RunContext related command + // handler into the current running job container + // We need this, to support scoping commands to the composite action + // executing. + logWriter := common.NewLineWriter(compositerc.commandHandler(ctx)) + oldout, olderr := compositerc.JobContainer.ReplaceLogWriter(logWriter, logWriter) + defer (func() { + rc.JobContainer.ReplaceLogWriter(oldout, olderr) + })() - // Map outputs to parent rc - eval = compositerc.NewStepExpressionEvaluator(step) + err := compositerc.compositeExecutor(action)(ctx) + + // Map outputs from composite RunContext to job RunContext + eval = compositerc.NewExpressionEvaluator() for outputName, output := range action.Outputs { - backup.setOutput(ctx, map[string]string{ + rc.setOutput(ctx, map[string]string{ "name": outputName, }, eval.Interpolate(output.Value)) } - backup.Masks = append(backup.Masks, compositerc.Masks...) - // Test if evaluated parent env was altered by this composite step - // Known Issues: - // - you try to set an env variable to the same value as a scoped step env, will be discared - for k, v := range compositerc.Env { - if ov, ok := origEnvMap[k]; !ok || ov != v { - backup.Env[k] = v - } - } + rc.Masks = append(rc.Masks, compositerc.Masks...) + return err } } +// Executor returns a pipeline executor for all the steps in the job +func (rc *RunContext) compositeExecutor(action *model.Action) common.Executor { + steps := make([]common.Executor, 0) + + sf := &stepFactoryImpl{} + + for i, step := range action.Runs.Steps { + if step.ID == "" { + step.ID = fmt.Sprintf("%d", i) + } + + // create a copy of the step, since this composite action could + // run multiple times and we might modify the instance + stepcopy := step + + step, err := sf.newStep(&stepcopy, rc) + if err != nil { + return common.NewErrorExecutor(err) + } + stepExec := common.NewPipelineExecutor(step.pre(), step.main(), step.post()) + + steps = append(steps, func(ctx context.Context) error { + err := stepExec(ctx) + if err != nil { + common.Logger(ctx).Errorf("%v", err) + common.SetJobError(ctx, err) + } else if ctx.Err() != nil { + common.Logger(ctx).Errorf("%v", ctx.Err()) + common.SetJobError(ctx, ctx.Err()) + } + return nil + }) + } + + steps = append(steps, common.JobError) + return func(ctx context.Context) error { + return common.NewPipelineExecutor(steps...)(common.WithJobErrorContainer(ctx)) + } +} + func populateEnvsFromInput(env *map[string]string, action *model.Action, rc *RunContext) { + eval := rc.NewExpressionEvaluator() for inputID, input := range action.Inputs { envKey := regexp.MustCompile("[^A-Z0-9-]").ReplaceAllString(strings.ToUpper(inputID), "_") envKey = fmt.Sprintf("INPUT_%s", envKey) if _, ok := (*env)[envKey]; !ok { - (*env)[envKey] = rc.ExprEval.Interpolate(input.Default) + (*env)[envKey] = eval.Interpolate(input.Default) } } } diff --git a/pkg/runner/action_test.go b/pkg/runner/action_test.go index 833e8b74bb7..39bd98ad35e 100644 --- a/pkg/runner/action_test.go +++ b/pkg/runner/action_test.go @@ -136,16 +136,6 @@ runs: } } -type exprEvalMock struct { - ExpressionEvaluator - mock.Mock -} - -func (e *exprEvalMock) Interpolate(expr string) string { - args := e.Called(expr) - return args.String(0) -} - func TestActionRunner(t *testing.T) { table := []struct { name string @@ -158,10 +148,7 @@ func TestActionRunner(t *testing.T) { Uses: "repo@ref", }, RunContext: &RunContext{ - ActionRepository: "repo", - ActionPath: "path", - ActionRef: "ref", - Config: &Config{}, + Config: &Config{}, Run: &model.Run{ JobID: "job", Workflow: &model.Workflow{ @@ -197,14 +184,9 @@ func TestActionRunner(t *testing.T) { cm.On("Exec", []string{"node", "/var/run/act/actions/dir/path"}, map[string]string{"INPUT_KEY": "default value"}, "", "").Return(func(ctx context.Context) error { return nil }) tt.step.getRunContext().JobContainer = cm - ee := &exprEvalMock{} - ee.On("Interpolate", "default value").Return("default value") - tt.step.getRunContext().ExprEval = ee - err := runActionImpl(tt.step, "dir", newRemoteAction("org/repo/path@ref"))(ctx) assert.Nil(t, err) - ee.AssertExpectations(t) cm.AssertExpectations(t) }) } diff --git a/pkg/runner/expression.go b/pkg/runner/expression.go index 9dbb16fe175..5ff311d68f1 100644 --- a/pkg/runner/expression.go +++ b/pkg/runner/expression.go @@ -37,11 +37,6 @@ func (rc *RunContext) NewExpressionEvaluator() ExpressionEvaluator { } } - secrets := rc.Config.Secrets - if rc.Composite != nil { - secrets = nil - } - ee := &exprparser.EvaluationEnvironment{ Github: rc.getGithubContext(), Env: rc.GetEnv(), @@ -54,7 +49,7 @@ func (rc *RunContext) NewExpressionEvaluator() ExpressionEvaluator { "temp": "/tmp", "tool_cache": "/opt/hostedtoolcache", }, - Secrets: secrets, + Secrets: rc.Config.Secrets, Strategy: strategy, Matrix: rc.Matrix, Needs: using, @@ -89,11 +84,6 @@ func (rc *RunContext) NewStepExpressionEvaluator(step step) ExpressionEvaluator } } - secrets := rc.Config.Secrets - if rc.Composite != nil { - secrets = nil - } - ee := &exprparser.EvaluationEnvironment{ Github: rc.getGithubContext(), Env: *step.getEnv(), @@ -104,7 +94,7 @@ func (rc *RunContext) NewStepExpressionEvaluator(step step) ExpressionEvaluator "temp": "/tmp", "tool_cache": "/opt/hostedtoolcache", }, - Secrets: secrets, + Secrets: rc.Config.Secrets, Strategy: strategy, Matrix: rc.Matrix, Needs: using, diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 06c074039f1..6c59357828f 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -43,7 +43,6 @@ type RunContext struct { ActionPath string ActionRef string ActionRepository string - Composite *model.Action Inputs map[string]interface{} Parent *RunContext Masks []string @@ -53,16 +52,6 @@ func (rc *RunContext) AddMask(mask string) { rc.Masks = append(rc.Masks, mask) } -func (rc *RunContext) Clone() *RunContext { - clone := *rc - clone.CurrentStep = "" - clone.Composite = nil - clone.Inputs = nil - clone.StepResults = make(map[string]*model.StepResult) - clone.Parent = rc - return &clone -} - type MappableOutput struct { StepID string OutputName string @@ -309,46 +298,6 @@ func (rc *RunContext) Executor() common.Executor { } } -// Executor returns a pipeline executor for all the steps in the job -func (rc *RunContext) compositeExecutor() common.Executor { - steps := make([]common.Executor, 0) - - sf := &stepFactoryImpl{} - - for i, step := range rc.Composite.Runs.Steps { - if step.ID == "" { - step.ID = fmt.Sprintf("%d", i) - } - - // create a copy of the step, since this composite action could - // run multiple times and we might modify the instance - stepcopy := step - - step, err := sf.newStep(&stepcopy, rc) - if err != nil { - return common.NewErrorExecutor(err) - } - stepExec := common.NewPipelineExecutor(step.pre(), step.main(), step.post()) - - steps = append(steps, func(ctx context.Context) error { - err := stepExec(ctx) - if err != nil { - common.Logger(ctx).Errorf("%v", err) - common.SetJobError(ctx, err) - } else if ctx.Err() != nil { - common.Logger(ctx).Errorf("%v", ctx.Err()) - common.SetJobError(ctx, ctx.Err()) - } - return nil - }) - } - - steps = append(steps, common.JobError) - return func(ctx context.Context) error { - return common.NewPipelineExecutor(steps...)(common.WithJobErrorContainer(ctx)) - } -} - func (rc *RunContext) platformImage() string { job := rc.Run.Job() @@ -628,8 +577,6 @@ func (rc *RunContext) withGithubEnv(env map[string]string) map[string]string { env["GITHUB_SERVER_URL"] = "https://github.com" env["GITHUB_API_URL"] = "https://api.github.com" env["GITHUB_GRAPHQL_URL"] = "https://api.github.com/graphql" - env["GITHUB_ACTION_REF"] = github.ActionRef - env["GITHUB_ACTION_REPOSITORY"] = github.ActionRepository env["GITHUB_BASE_REF"] = github.BaseRef env["GITHUB_HEAD_REF"] = github.HeadRef env["GITHUB_JOB"] = rc.JobName diff --git a/pkg/runner/step_run.go b/pkg/runner/step_run.go index 68ebd90767d..a67d390e8d1 100644 --- a/pkg/runner/step_run.go +++ b/pkg/runner/step_run.go @@ -130,7 +130,7 @@ func (sr *stepRun) setupShell() { step.Shell = rc.Run.Job().Defaults.Run.Shell } - step.Shell = rc.ExprEval.Interpolate(step.Shell) + step.Shell = rc.NewExpressionEvaluator().Interpolate(step.Shell) if step.Shell == "" { step.Shell = rc.Run.Workflow.Defaults.Run.Shell @@ -157,7 +157,7 @@ func (sr *stepRun) setupWorkingDirectory() { } // jobs can receive context values, so we interpolate - step.WorkingDirectory = rc.ExprEval.Interpolate(step.WorkingDirectory) + step.WorkingDirectory = rc.NewExpressionEvaluator().Interpolate(step.WorkingDirectory) // but top level keys in workflow file like `defaults` or `env` can't if step.WorkingDirectory == "" {