From b8d7e947cf91e24202326b63821a3ff82a3e2b57 Mon Sep 17 00:00:00 2001 From: ChristopherHX Date: Thu, 15 Dec 2022 18:08:31 +0100 Subject: [PATCH] refactor: fix savestate in pre steps (#1466) * refactor: fix savestate in pre steps * fix pre steps collision * fix tests * remove * enable tests * Update pkg/runner/action.go * Rename InterActionState to IntraActionState Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- pkg/model/step_result.go | 1 - pkg/runner/action.go | 6 +-- pkg/runner/action_test.go | 7 ++-- pkg/runner/command.go | 17 +++++---- pkg/runner/command_test.go | 8 +--- pkg/runner/run_context.go | 1 + pkg/runner/step.go | 41 ++++++++------------ pkg/runner/step_action_local_test.go | 31 ++++----------- pkg/runner/step_action_remote_test.go | 46 +++++++++-------------- pkg/runner/testdata/GITHUB_STATE/push.yml | 34 +++++++++++++++-- 10 files changed, 90 insertions(+), 102 deletions(-) diff --git a/pkg/model/step_result.go b/pkg/model/step_result.go index 49b7705..86e5ebf 100644 --- a/pkg/model/step_result.go +++ b/pkg/model/step_result.go @@ -42,5 +42,4 @@ type StepResult struct { Outputs map[string]string `json:"outputs"` Conclusion stepStatus `json:"conclusion"` Outcome stepStatus `json:"outcome"` - State map[string]string } diff --git a/pkg/runner/action.go b/pkg/runner/action.go index ec7ac7c..5614414 100644 --- a/pkg/runner/action.go +++ b/pkg/runner/action.go @@ -374,9 +374,9 @@ func newStepContainer(ctx context.Context, step step, image string, cmd []string } func populateEnvsFromSavedState(env *map[string]string, step actionStep, rc *RunContext) { - stepResult := rc.StepResults[step.getStepModel().ID] - if stepResult != nil { - for name, value := range stepResult.State { + state, ok := rc.IntraActionState[step.getStepModel().ID] + if ok { + for name, value := range state { envName := fmt.Sprintf("STATE_%s", name) (*env)[envName] = value } diff --git a/pkg/runner/action_test.go b/pkg/runner/action_test.go index 0b23085..36ee14f 100644 --- a/pkg/runner/action_test.go +++ b/pkg/runner/action_test.go @@ -201,10 +201,11 @@ func TestActionRunner(t *testing.T) { }, CurrentStep: "post-step", StepResults: map[string]*model.StepResult{ + "step": {}, + }, + IntraActionState: map[string]map[string]string{ "step": { - State: map[string]string{ - "name": "state value", - }, + "name": "state value", }, }, }, diff --git a/pkg/runner/command.go b/pkg/runner/command.go index 3fa3715..0b0ba2f 100755 --- a/pkg/runner/command.go +++ b/pkg/runner/command.go @@ -153,13 +153,16 @@ func unescapeKvPairs(kvPairs map[string]string) map[string]string { } func (rc *RunContext) saveState(ctx context.Context, kvPairs map[string]string, arg string) { - if rc.CurrentStep != "" { - stepResult := rc.StepResults[rc.CurrentStep] - if stepResult != nil { - if stepResult.State == nil { - stepResult.State = map[string]string{} - } - stepResult.State[kvPairs["name"]] = arg + stepID := rc.CurrentStep + if stepID != "" { + if rc.IntraActionState == nil { + rc.IntraActionState = map[string]map[string]string{} } + state, ok := rc.IntraActionState[stepID] + if !ok { + state = map[string]string{} + rc.IntraActionState[stepID] = state + } + state[kvPairs["name"]] = arg } } diff --git a/pkg/runner/command_test.go b/pkg/runner/command_test.go index 8f71af6..57c7de5 100644 --- a/pkg/runner/command_test.go +++ b/pkg/runner/command_test.go @@ -177,11 +177,7 @@ func TestAddmaskUsemask(t *testing.T) { func TestSaveState(t *testing.T) { rc := &RunContext{ CurrentStep: "step", - StepResults: map[string]*model.StepResult{ - "step": { - State: map[string]string{}, - }, - }, + StepResults: map[string]*model.StepResult{}, } ctx := context.Background() @@ -189,5 +185,5 @@ func TestSaveState(t *testing.T) { handler := rc.commandHandler(ctx) handler("::save-state name=state-name::state-value\n") - assert.Equal(t, "state-value", rc.StepResults["step"].State["state-name"]) + assert.Equal(t, "state-value", rc.IntraActionState["step"]["state-name"]) } diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 854e628..b579e3e 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -38,6 +38,7 @@ type RunContext struct { ExtraPath []string CurrentStep string StepResults map[string]*model.StepResult + IntraActionState map[string]map[string]string ExprEval ExpressionEvaluator JobContainer container.ExecutionsEnvironment OutputMappings map[MappableOutput]MappableOutput diff --git a/pkg/runner/step.go b/pkg/runner/step.go index f8a192f..2e211a3 100644 --- a/pkg/runner/step.go +++ b/pkg/runner/step.go @@ -44,18 +44,6 @@ func (s stepStage) String() string { return "Unknown" } -func (s stepStage) getStepName(stepModel *model.Step) string { - switch s { - case stepStagePre: - return fmt.Sprintf("pre-%s", stepModel.ID) - case stepStageMain: - return stepModel.ID - case stepStagePost: - return fmt.Sprintf("post-%s", stepModel.ID) - } - return "unknown" -} - func runStepExecutor(step step, stage stepStage, executor common.Executor) common.Executor { return func(ctx context.Context) error { logger := common.Logger(ctx) @@ -63,13 +51,16 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo stepModel := step.getStepModel() ifExpression := step.getIfExpression(ctx, stage) - rc.CurrentStep = stage.getStepName(stepModel) + rc.CurrentStep = stepModel.ID - rc.StepResults[rc.CurrentStep] = &model.StepResult{ + stepResult := &model.StepResult{ Outcome: model.StepStatusSuccess, Conclusion: model.StepStatusSuccess, Outputs: make(map[string]string), } + if stage == stepStageMain { + rc.StepResults[rc.CurrentStep] = stepResult + } err := setupEnv(ctx, step) if err != nil { @@ -78,15 +69,15 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo runStep, err := isStepEnabled(ctx, ifExpression, step, stage) if err != nil { - rc.StepResults[rc.CurrentStep].Conclusion = model.StepStatusFailure - rc.StepResults[rc.CurrentStep].Outcome = model.StepStatusFailure + stepResult.Conclusion = model.StepStatusFailure + stepResult.Outcome = model.StepStatusFailure return err } if !runStep { - rc.StepResults[rc.CurrentStep].Conclusion = model.StepStatusSkipped - rc.StepResults[rc.CurrentStep].Outcome = model.StepStatusSkipped - logger.WithField("stepResult", rc.StepResults[rc.CurrentStep].Outcome).Debugf("Skipping step '%s' due to '%s'", stepModel, ifExpression) + stepResult.Conclusion = model.StepStatusSkipped + stepResult.Outcome = model.StepStatusSkipped + logger.WithField("stepResult", stepResult.Outcome).Debugf("Skipping step '%s' due to '%s'", stepModel, ifExpression) return nil } @@ -118,25 +109,25 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo err = executor(ctx) if err == nil { - logger.WithField("stepResult", rc.StepResults[rc.CurrentStep].Outcome).Infof(" \u2705 Success - %s %s", stage, stepString) + logger.WithField("stepResult", stepResult.Outcome).Infof(" \u2705 Success - %s %s", stage, stepString) } else { - rc.StepResults[rc.CurrentStep].Outcome = model.StepStatusFailure + stepResult.Outcome = model.StepStatusFailure continueOnError, parseErr := isContinueOnError(ctx, stepModel.RawContinueOnError, step, stage) if parseErr != nil { - rc.StepResults[rc.CurrentStep].Conclusion = model.StepStatusFailure + stepResult.Conclusion = model.StepStatusFailure return parseErr } if continueOnError { logger.Infof("Failed but continue next step") err = nil - rc.StepResults[rc.CurrentStep].Conclusion = model.StepStatusSuccess + stepResult.Conclusion = model.StepStatusSuccess } else { - rc.StepResults[rc.CurrentStep].Conclusion = model.StepStatusFailure + stepResult.Conclusion = model.StepStatusFailure } - logger.WithField("stepResult", rc.StepResults[rc.CurrentStep].Outcome).Errorf(" \u274C Failure - %s %s", stage, stepString) + logger.WithField("stepResult", stepResult.Outcome).Errorf(" \u274C Failure - %s %s", stage, stepString) } // Process Runner File Commands orgerr := err diff --git a/pkg/runner/step_action_local_test.go b/pkg/runner/step_action_local_test.go index 9ece06a..023f701 100644 --- a/pkg/runner/step_action_local_test.go +++ b/pkg/runner/step_action_local_test.go @@ -107,13 +107,12 @@ func TestStepActionLocalTest(t *testing.T) { func TestStepActionLocalPost(t *testing.T) { table := []struct { - name string - stepModel *model.Step - actionModel *model.Action - initialStepResults map[string]*model.StepResult - expectedPostStepResult *model.StepResult - err error - mocks struct { + name string + stepModel *model.Step + actionModel *model.Action + initialStepResults map[string]*model.StepResult + err error + mocks struct { env bool exec bool } @@ -138,11 +137,6 @@ func TestStepActionLocalPost(t *testing.T) { Outputs: map[string]string{}, }, }, - expectedPostStepResult: &model.StepResult{ - Conclusion: model.StepStatusSuccess, - Outcome: model.StepStatusSuccess, - Outputs: map[string]string{}, - }, mocks: struct { env bool exec bool @@ -171,11 +165,6 @@ func TestStepActionLocalPost(t *testing.T) { Outputs: map[string]string{}, }, }, - expectedPostStepResult: &model.StepResult{ - Conclusion: model.StepStatusSuccess, - Outcome: model.StepStatusSuccess, - Outputs: map[string]string{}, - }, mocks: struct { env bool exec bool @@ -204,11 +193,6 @@ func TestStepActionLocalPost(t *testing.T) { Outputs: map[string]string{}, }, }, - expectedPostStepResult: &model.StepResult{ - Conclusion: model.StepStatusSkipped, - Outcome: model.StepStatusSkipped, - Outputs: map[string]string{}, - }, mocks: struct { env bool exec bool @@ -238,7 +222,6 @@ func TestStepActionLocalPost(t *testing.T) { Outputs: map[string]string{}, }, }, - expectedPostStepResult: nil, mocks: struct { env bool exec bool @@ -307,7 +290,7 @@ func TestStepActionLocalPost(t *testing.T) { err := sal.post()(ctx) assert.Equal(t, tt.err, err) - assert.Equal(t, tt.expectedPostStepResult, sal.RunContext.StepResults["post-step"]) + assert.Equal(t, sal.RunContext.StepResults["post-step"], (*model.StepResult)(nil)) cm.AssertExpectations(t) }) } diff --git a/pkg/runner/step_action_remote_test.go b/pkg/runner/step_action_remote_test.go index 829e386..a6653f7 100644 --- a/pkg/runner/step_action_remote_test.go +++ b/pkg/runner/step_action_remote_test.go @@ -415,14 +415,14 @@ func TestStepActionRemotePreThroughActionToken(t *testing.T) { func TestStepActionRemotePost(t *testing.T) { table := []struct { - name string - stepModel *model.Step - actionModel *model.Action - initialStepResults map[string]*model.StepResult - expectedEnv map[string]string - expectedPostStepResult *model.StepResult - err error - mocks struct { + name string + stepModel *model.Step + actionModel *model.Action + initialStepResults map[string]*model.StepResult + IntraActionState map[string]map[string]string + expectedEnv map[string]string + err error + mocks struct { env bool exec bool } @@ -445,19 +445,16 @@ func TestStepActionRemotePost(t *testing.T) { Conclusion: model.StepStatusSuccess, Outcome: model.StepStatusSuccess, Outputs: map[string]string{}, - State: map[string]string{ - "key": "value", - }, + }, + }, + IntraActionState: map[string]map[string]string{ + "step": { + "key": "value", }, }, expectedEnv: map[string]string{ "STATE_key": "value", }, - expectedPostStepResult: &model.StepResult{ - Conclusion: model.StepStatusSuccess, - Outcome: model.StepStatusSuccess, - Outputs: map[string]string{}, - }, mocks: struct { env bool exec bool @@ -486,11 +483,6 @@ func TestStepActionRemotePost(t *testing.T) { Outputs: map[string]string{}, }, }, - expectedPostStepResult: &model.StepResult{ - Conclusion: model.StepStatusSuccess, - Outcome: model.StepStatusSuccess, - Outputs: map[string]string{}, - }, mocks: struct { env bool exec bool @@ -519,11 +511,6 @@ func TestStepActionRemotePost(t *testing.T) { Outputs: map[string]string{}, }, }, - expectedPostStepResult: &model.StepResult{ - Conclusion: model.StepStatusSkipped, - Outcome: model.StepStatusSkipped, - Outputs: map[string]string{}, - }, mocks: struct { env bool exec bool @@ -553,7 +540,6 @@ func TestStepActionRemotePost(t *testing.T) { Outputs: map[string]string{}, }, }, - expectedPostStepResult: nil, mocks: struct { env bool exec bool @@ -585,7 +571,8 @@ func TestStepActionRemotePost(t *testing.T) { }, }, }, - StepResults: tt.initialStepResults, + StepResults: tt.initialStepResults, + IntraActionState: tt.IntraActionState, }, Step: tt.stepModel, action: tt.actionModel, @@ -622,7 +609,8 @@ func TestStepActionRemotePost(t *testing.T) { assert.Equal(t, value, sar.env[key]) } } - assert.Equal(t, tt.expectedPostStepResult, sar.RunContext.StepResults["post-step"]) + // Enshure that StepResults is nil in this test + assert.Equal(t, sar.RunContext.StepResults["post-step"], (*model.StepResult)(nil)) cm.AssertExpectations(t) }) } diff --git a/pkg/runner/testdata/GITHUB_STATE/push.yml b/pkg/runner/testdata/GITHUB_STATE/push.yml index 179c5a7..61afc07 100644 --- a/pkg/runner/testdata/GITHUB_STATE/push.yml +++ b/pkg/runner/testdata/GITHUB_STATE/push.yml @@ -15,8 +15,34 @@ jobs: echo "::save-state name=mystate3::mystateval" post: | env - # Enable once https://github.com/nektos/act/issues/1459 is fixed - # [ "$STATE_mystate0" = "mystateval" ] - # [ "$STATE_mystate1" = "mystateval" ] + [ "$STATE_mystate0" = "mystateval" ] + [ "$STATE_mystate1" = "mystateval" ] [ "$STATE_mystate2" = "mystateval" ] - [ "$STATE_mystate3" = "mystateval" ] \ No newline at end of file + [ "$STATE_mystate3" = "mystateval" ] + test-id-collision-bug: + runs-on: ubuntu-latest + steps: + - uses: nektos/act-test-actions/script@main + id: script + with: + pre: | + env + echo mystate0=mystateval > $GITHUB_STATE + echo "::save-state name=mystate1::mystateval" + main: | + env + echo mystate2=mystateval > $GITHUB_STATE + echo "::save-state name=mystate3::mystateval" + post: | + env + [ "$STATE_mystate0" = "mystateval" ] + [ "$STATE_mystate1" = "mystateval" ] + [ "$STATE_mystate2" = "mystateval" ] + [ "$STATE_mystate3" = "mystateval" ] + - uses: nektos/act-test-actions/script@main + id: pre-script + with: + main: | + env + echo mystate0=mystateerror > $GITHUB_STATE + echo "::save-state name=mystate1::mystateerror" \ No newline at end of file