Workflow run refactor #969
Conversation
| @@ -180,34 +180,29 @@ func CleanupStepWorkDir() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // GetBuildFailedEnvironments ... | |||
| func GetBuildFailedEnvironments(failed bool) []string { | |||
| @@ -267,6 +267,9 @@ func (r WorkflowRunner) runWorkflows(tracker analytics.Tracker) (models.BuildRun | |||
| if err := os.Setenv("BITRISE_TRIGGERED_WORKFLOW_TITLE", targetWorkflow.Title); err != nil { | |||
| return models.BuildRunResultsModel{}, fmt.Errorf("failed to set BITRISE_TRIGGERED_WORKFLOW_TITLE env: %w", err) | |||
| } | |||
| if err := bitrise.SetBuildFailedEnv(false); err != nil { | |||
There was a problem hiding this comment.
Earlier build status env was set and updated before running a step (and before evaluating the Step's RunIf condition). Because of this we couldn't merge and move all the env var preparations for a step execution into a new function.
With this change, we initialize the build status env before the workflow execution and update it after each step execution.
| stepStartedProperties, | ||
| ) | ||
|
|
||
| *environments = append(*environments, result.OutputEnvironments...) |
There was a problem hiding this comment.
This is the single place where a step run modifies the "build"'s env vars, subsequent functions are just reading the env vars, and that's when they receive environments as a value argument.
| if mergedStep.RunIf != nil && *mergedStep.RunIf != "" { | ||
| buildFailedEnvs := bitrise.BuildFailedEnvs(buildRunResults.IsBuildFailed()) | ||
| runIfEnvs := append(environments, buildFailedEnvs...) | ||
| runIfEnvList, err := envman.ConvertToEnvsJSONModel(runIfEnvs, true, false, &envmanEnv.DefaultEnvironmentSource{}) |
There was a problem hiding this comment.
Previously the env vars were read from the step's Input EnvStore file:
envList, err := tools.EnvmanReadEnvList(configs.InputEnvstorePath)
Because of this, we had to initialize the Step Input EnvStore file and add the current build env list to the file.
With this change, we can do the Step Input EnvStore file init after evaluating the Step's run conditions (RunIf and IsAlwaysRun) and also all the step env var preparation work could be merged and moved to the new prepareEnvsForStepRun function.
cli/step_activator.go
Outdated
| } | ||
| } else if stepIDData.SteplibSource != "" { | ||
| isUpdated := buildRunResults.IsStepLibUpdated(stepIDData.SteplibSource) | ||
| isUpdated := isStepLibUpdated |
There was a problem hiding this comment.
nit: The isUpdated local variable is only used in the function call right below it. We could remove this and just use the isStepLibUpdated as the parameter directly.
cli/step_activator.go
Outdated
| } | ||
| } else if stepIDData.SteplibSource == "_" { | ||
| log.Debugf("[BITRISE_CLI] - Steplib independent step, with direct git uri: (uri:%s) (tag-or-branch:%s)", stepIDData.IDorURI, stepIDData.Version) | ||
|
|
||
| // Steplib independent steps are completly defined in workflow | ||
| // StepLib independent steps are completely defined in workflow |
There was a problem hiding this comment.
nit: This was already like this before your changes but I think a the is missing in front of the workflow word
cli/run_util.go
Outdated
| // and shutting down the last set of containers at the end of the workflow run | ||
| currentStepGroupID := "" | ||
| lastStepGroupID := "" | ||
| defer func() { |
There was a problem hiding this comment.
nit: I was looking at the code and noticed this defer statement here? With this defer it is not easy to see when the stopContainersForStepGroup function is called because we only know that it will be called before exiting this function. And as I was looking at the function I noticed that it has a single exit point at the end.
So my question is that would it make simpler if we kill this defer and move
if lastStepGroupID != "" {
r.stopContainersForStepGroup(lastStepGroupID, plan.WorkflowTitle)
}
on line 140 or 144?
godrei
force-pushed
the
workflow-run-refactor
branch
from
6a63bf5
to
b4448e2
Compare
Checklist
README.mdis updated with the changes (if needed)Version
Requires a NO version update
Context
This PR breaks
WorkflowRunner.activateAndRunStepsinto smaller functions, to prepare the code base for introducing Step Bundles. The activateAndRunSteps function was lengthy (300+ lines), incorporating many aspects of the execution of the workflow steps.The single-step execution is implemented in
activateAndRunStepand the functionality is broken down into new subfunctions likeactivateStepandprepareEnvsForStepRunnext to the existingrunStep.The new hierarchy of the workflow run function in the
run_util.gois:runWorkflowactivateAndRunStepsactivateAndRunStepactivateStepprepareEnvsForStepRunrunStepThe code change is about redistributing existing functionality.