From 5f673cbb6db5569334cc806cf0536a97ad6b899b 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> --- pkg/runner/action.go | 184 ++++++++++++++------------ pkg/runner/action_test.go | 4 +- pkg/runner/run_context.go | 2 +- pkg/runner/step_action_local.go | 2 +- pkg/runner/step_action_local_test.go | 6 +- pkg/runner/step_action_remote.go | 2 +- pkg/runner/step_action_remote_test.go | 8 +- 7 files changed, 109 insertions(+), 99 deletions(-) diff --git a/pkg/runner/action.go b/pkg/runner/action.go index a50efdd..6a8d31a 100644 --- a/pkg/runner/action.go +++ b/pkg/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/pkg/runner/action_test.go b/pkg/runner/action_test.go index 2a47dc0..833e8b7 100644 --- a/pkg/runner/action_test.go +++ b/pkg/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/pkg/runner/run_context.go b/pkg/runner/run_context.go index d5eab92..8b1d3e1 100644 --- a/pkg/runner/run_context.go +++ b/pkg/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/pkg/runner/step_action_local.go b/pkg/runner/step_action_local.go index 9eb6fa4..de3c9cc 100644 --- a/pkg/runner/step_action_local.go +++ b/pkg/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/pkg/runner/step_action_local_test.go b/pkg/runner/step_action_local_test.go index 235fae3..5860729 100644 --- a/pkg/runner/step_action_local_test.go +++ b/pkg/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/pkg/runner/step_action_remote.go b/pkg/runner/step_action_remote.go index 03895b8..f1b532a 100644 --- a/pkg/runner/step_action_remote.go +++ b/pkg/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/pkg/runner/step_action_remote_test.go b/pkg/runner/step_action_remote_test.go index d41109f..fd5ebcd 100644 --- a/pkg/runner/step_action_remote_test.go +++ b/pkg/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)