From 78987a6f82743b371be5d3925041ec0989d574cb Mon Sep 17 00:00:00 2001 From: Markus Wolf Date: Tue, 29 Mar 2022 19:42:11 +0200 Subject: [PATCH] refactor: simplify action function signatures (#1083) This change reduces the interfaces by removing obsolete parameters from functions. Obsolete parameters does not means unused ones, but parameters which could be retrieved from other parameters instead. This should simplify logic and maintainability for these functions. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- act/runner/action.go | 184 ++++++++++++++------------ act/runner/action_test.go | 4 +- act/runner/run_context.go | 2 +- act/runner/step_action_local.go | 2 +- act/runner/step_action_local_test.go | 6 +- act/runner/step_action_remote.go | 2 +- act/runner/step_action_remote_test.go | 8 +- 7 files changed, 109 insertions(+), 99 deletions(-) diff --git a/act/runner/action.go b/act/runner/action.go index a50efddf..6a8d31ad 100644 --- a/act/runner/action.go +++ b/act/runner/action.go @@ -31,7 +31,7 @@ type readAction func(step *model.Step, actionDir string, actionPath string, read type actionYamlReader func(filename string) (io.Reader, io.Closer, error) type fileWriter func(filename string, data []byte, perm fs.FileMode) error -type runAction func(step actionStep, actionDir string, actionPath string, actionRepository string, actionRef string, localAction bool) common.Executor +type runAction func(step actionStep, actionDir string, remoteAction *remoteAction) common.Executor //go:embed res/trampoline.js var trampoline embed.FS @@ -98,7 +98,7 @@ func readActionImpl(step *model.Step, actionDir string, actionPath string, readF return action, err } -func runActionImpl(step actionStep, actionDir string, actionPath string, actionRepository string, actionRef string, localAction bool) common.Executor { +func runActionImpl(step actionStep, actionDir string, remoteAction *remoteAction) common.Executor { rc := step.getRunContext() stepModel := step.getStepModel() return func(ctx context.Context) error { @@ -111,19 +111,24 @@ func runActionImpl(step actionStep, actionDir string, actionPath string, actionR rc.ActionRef = parentActionRef rc.ActionRepository = parentActionRepository }() - rc.ActionRef = actionRef - rc.ActionRepository = actionRepository + + actionPath := "" + if remoteAction != nil { + rc.ActionRef = remoteAction.Ref + rc.ActionRepository = remoteAction.Repo + if remoteAction.Path != "" { + actionPath = remoteAction.Path + } + } else { + rc.ActionRef = "" + rc.ActionRepository = "" + } action := step.getActionModel() log.Debugf("About to run action %v", action) populateEnvsFromInput(step.getEnv(), action, rc) - actionLocation := "" - if actionPath != "" { - actionLocation = path.Join(actionDir, actionPath) - } else { - actionLocation = actionDir - } + actionLocation := path.Join(actionDir, actionPath) actionName, containerActionDir := getContainerActionPaths(stepModel, actionLocation, rc) 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) @@ -156,9 +161,16 @@ func runActionImpl(step actionStep, actionDir string, actionPath string, actionR log.Debugf("executing remote job container: %s", containerArgs) return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx) case model.ActionRunsUsingDocker: - return execAsDocker(ctx, action, actionName, containerActionDir, actionLocation, rc, step, localAction) + location := actionLocation + if remoteAction == nil { + location = containerActionDir + } + return execAsDocker(ctx, step, actionName, location, remoteAction == nil) case model.ActionRunsUsingComposite: - return execAsComposite(ctx, step, actionDir, rc, containerActionDir, actionName, actionPath, action, maybeCopyToActionDir) + if err := maybeCopyToActionDir(); err != nil { + return err + } + return execAsComposite(step)(ctx) default: return fmt.Errorf(fmt.Sprintf("The runs.using key must be one of: %v, got %s", []string{ model.ActionRunsUsingDocker, @@ -189,7 +201,10 @@ func removeGitIgnore(directory string) error { // TODO: break out parts of function to reduce complexicity // nolint:gocyclo -func execAsDocker(ctx context.Context, action *model.Action, actionName string, containerLocation string, actionLocation string, rc *RunContext, step step, localAction bool) error { +func execAsDocker(ctx context.Context, step actionStep, actionName string, basedir string, localAction bool) error { + rc := step.getRunContext() + action := step.getActionModel() + var prepImage common.Executor var image string if strings.HasPrefix(action.Runs.Image, "docker://") { @@ -199,10 +214,6 @@ func execAsDocker(ctx context.Context, action *model.Action, actionName string, image = fmt.Sprintf("%s-dockeraction:%s", regexp.MustCompile("[^a-zA-Z0-9]").ReplaceAllString(actionName, "-"), "latest") image = fmt.Sprintf("act-%s", strings.TrimLeft(image, "-")) image = strings.ToLower(image) - basedir := actionLocation - if localAction { - basedir = containerLocation - } contextDir := filepath.Join(basedir, action.Runs.Main) anyArchExists, err := container.ImageExistsLocally(ctx, image, "any") @@ -229,7 +240,7 @@ func execAsDocker(ctx context.Context, action *model.Action, actionName string, log.Debugf("image '%s' for architecture '%s' will be built from context '%s", image, rc.Config.ContainerArchitecture, contextDir) var actionContainer container.Container if localAction { - actionContainer = step.getRunContext().JobContainer + actionContainer = rc.JobContainer } prepImage = container.NewDockerBuildExecutor(container.NewDockerBuildExecutorInput{ ContextDir: contextDir, @@ -241,7 +252,7 @@ func execAsDocker(ctx context.Context, action *model.Action, actionName string, log.Debugf("image '%s' for architecture '%s' already exists", image, rc.Config.ContainerArchitecture) } } - eval := step.getRunContext().NewStepExpressionEvaluator(step) + eval := rc.NewStepExpressionEvaluator(step) cmd, err := shellquote.Split(eval.Interpolate(step.getStepModel().With["args"])) if err != nil { return err @@ -348,76 +359,77 @@ func newStepContainer(ctx context.Context, step step, image string, cmd []string return stepContainer } -func execAsComposite(ctx context.Context, step step, _ string, rc *RunContext, containerActionDir string, actionName string, _ string, action *model.Action, maybeCopyToActionDir func() error) error { - err := maybeCopyToActionDir() +func execAsComposite(step actionStep) common.Executor { + rc := step.getRunContext() + action := step.getActionModel() - if err != nil { + return func(ctx context.Context) error { + // Disable some features of composite actions, only for feature parity with github + for _, compositeStep := range action.Runs.Steps { + if err := compositeStep.Validate(rc.Config.CompositeRestrictions); err != nil { + return err + } + } + inputs := make(map[string]interface{}) + eval := step.getRunContext().NewExpressionEvaluator() + // Set Defaults + for k, input := range action.Inputs { + inputs[k] = eval.Interpolate(input.Default) + } + if step.getStepModel().With != nil { + for k, v := range step.getStepModel().With { + 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) + } + compositerc := rc + compositerc.Parent = &RunContext{ + CurrentStep: scriptName, + } + // 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 + } + compositerc.Inputs = inputs + compositerc.ExprEval = compositerc.NewExpressionEvaluator() + + err := compositerc.compositeExecutor()(ctx) + + // Map outputs to parent rc + eval = compositerc.NewStepExpressionEvaluator(step) + for outputName, output := range action.Outputs { + backup.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 + } + } return err } - // Disable some features of composite actions, only for feature parity with github - for _, compositeStep := range action.Runs.Steps { - if err := compositeStep.Validate(rc.Config.CompositeRestrictions); err != nil { - return err - } - } - inputs := make(map[string]interface{}) - eval := step.getRunContext().NewExpressionEvaluator() - // Set Defaults - for k, input := range action.Inputs { - inputs[k] = eval.Interpolate(input.Default) - } - if step.getStepModel().With != nil { - for k, v := range step.getStepModel().With { - 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) - } - compositerc := rc - compositerc.Parent = &RunContext{ - CurrentStep: scriptName, - } - // 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 - } - compositerc.Inputs = inputs - compositerc.ExprEval = compositerc.NewExpressionEvaluator() - err = compositerc.CompositeExecutor()(ctx) - - // Map outputs to parent rc - eval = compositerc.NewStepExpressionEvaluator(step) - for outputName, output := range action.Outputs { - backup.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 - } - } - return err } func populateEnvsFromInput(env *map[string]string, action *model.Action, rc *RunContext) { diff --git a/act/runner/action_test.go b/act/runner/action_test.go index 2a47dc0e..833e8b74 100644 --- a/act/runner/action_test.go +++ b/act/runner/action_test.go @@ -201,9 +201,7 @@ func TestActionRunner(t *testing.T) { ee.On("Interpolate", "default value").Return("default value") tt.step.getRunContext().ExprEval = ee - _, localAction := tt.step.(*stepActionRemote) - - err := runActionImpl(tt.step, "dir", "path", "repo", "ref", localAction)(ctx) + err := runActionImpl(tt.step, "dir", newRemoteAction("org/repo/path@ref"))(ctx) assert.Nil(t, err) ee.AssertExpectations(t) diff --git a/act/runner/run_context.go b/act/runner/run_context.go index d5eab92e..8b1d3e1b 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -295,7 +295,7 @@ func (rc *RunContext) Executor() common.Executor { } // Executor returns a pipeline executor for all the steps in the job -func (rc *RunContext) CompositeExecutor() common.Executor { +func (rc *RunContext) compositeExecutor() common.Executor { steps := make([]common.Executor, 0) sf := &stepFactoryImpl{} diff --git a/act/runner/step_action_local.go b/act/runner/step_action_local.go index 9eb6fa48..de3c9cc3 100644 --- a/act/runner/step_action_local.go +++ b/act/runner/step_action_local.go @@ -58,7 +58,7 @@ func (sal *stepActionLocal) main() common.Executor { sal.action = actionModel log.Debugf("Read action %v from '%s'", sal.action, "Unknown") - return sal.runAction(sal, actionDir, "", "", "", true)(ctx) + return sal.runAction(sal, actionDir, nil)(ctx) }) } diff --git a/act/runner/step_action_local_test.go b/act/runner/step_action_local_test.go index 235fae3d..58607299 100644 --- a/act/runner/step_action_local_test.go +++ b/act/runner/step_action_local_test.go @@ -14,8 +14,8 @@ type stepActionLocalMocks struct { mock.Mock } -func (salm *stepActionLocalMocks) runAction(step actionStep, actionDir string, actionPath string, actionRepository string, actionRef string, localAction bool) common.Executor { - args := salm.Called(step, actionDir, actionPath, actionRepository, actionRef, localAction) +func (salm *stepActionLocalMocks) runAction(step actionStep, actionDir string, remoteAction *remoteAction) common.Executor { + args := salm.Called(step, actionDir, remoteAction) return args.Get(0).(func(context.Context) error) } @@ -76,7 +76,7 @@ func TestStepActionLocalTest(t *testing.T) { return nil }) - salm.On("runAction", sal, "/tmp/path/to/action", "", "", "", true).Return(func(ctx context.Context) error { + salm.On("runAction", sal, "/tmp/path/to/action", (*remoteAction)(nil)).Return(func(ctx context.Context) error { return nil }) diff --git a/act/runner/step_action_remote.go b/act/runner/step_action_remote.go index 03895b86..f1b532a8 100644 --- a/act/runner/step_action_remote.go +++ b/act/runner/step_action_remote.go @@ -86,7 +86,7 @@ func (sar *stepActionRemote) main() common.Executor { log.Debugf("Read action %v from '%s'", sar.action, "Unknown") return err }, - sar.runAction(sar, actionDir, remoteAction.Path, remoteAction.Repo, remoteAction.Ref, false), + sar.runAction(sar, actionDir, remoteAction), )(ctx) }) } diff --git a/act/runner/step_action_remote_test.go b/act/runner/step_action_remote_test.go index d41109f6..fd5ebcd8 100644 --- a/act/runner/step_action_remote_test.go +++ b/act/runner/step_action_remote_test.go @@ -20,8 +20,8 @@ func (sarm *stepActionRemoteMocks) readAction(step *model.Step, actionDir string return args.Get(0).(*model.Action), args.Error(1) } -func (sarm *stepActionRemoteMocks) runAction(step actionStep, actionDir string, actionPath string, actionRepository string, actionRef string, localAction bool) common.Executor { - args := sarm.Called(step, actionDir, actionPath, actionRepository, actionRef, localAction) +func (sarm *stepActionRemoteMocks) runAction(step actionStep, actionDir string, remoteAction *remoteAction) common.Executor { + args := sarm.Called(step, actionDir, remoteAction) return args.Get(0).(func(context.Context) error) } @@ -48,7 +48,7 @@ func TestStepActionRemoteTest(t *testing.T) { sar := &stepActionRemote{ RunContext: &RunContext{ Config: &Config{ - GitHubInstance: "https://github.com", + GitHubInstance: "github.com", }, Run: &model.Run{ JobID: "1", @@ -79,7 +79,7 @@ func TestStepActionRemoteTest(t *testing.T) { cm.On("UpdateFromPath", &sar.env).Return(func(ctx context.Context) error { return nil }) sarm.On("readAction", sar.Step, suffixMatcher("act/remote-action@v1"), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) - sarm.On("runAction", sar, suffixMatcher("act/remote-action@v1"), "", "action", "v1", false).Return(func(ctx context.Context) error { return nil }) + sarm.On("runAction", sar, suffixMatcher("act/remote-action@v1"), newRemoteAction(sar.Step.Uses)).Return(func(ctx context.Context) error { return nil }) err := sar.main()(ctx)