Skip to content

Commit

Permalink
refactor: remove composite action runcontext workaround
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@KnisterPeter
KnisterPeter authored and github-actions committed Apr 27, 2022
1 parent 5ca8ab9 commit 6b56e0d
Show file tree
Hide file tree
Showing 6 changed files with 123 additions and 148 deletions.
11 changes: 11 additions & 0 deletions pkg/container/docker_run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
169 changes: 107 additions & 62 deletions pkg/runner/action.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()

Expand All @@ -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)
}
Expand All @@ -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)
}
}
}
Expand Down
20 changes: 1 addition & 19 deletions pkg/runner/action_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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{
Expand Down Expand Up @@ -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)
})
}
Expand Down
14 changes: 2 additions & 12 deletions pkg/runner/expression.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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,
Expand Down Expand Up @@ -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(),
Expand All @@ -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,
Expand Down
Loading

0 comments on commit 6b56e0d

Please sign in to comment.