Fix #2363. Add /pre- and /post-entrypoint handling

[AndriiChyrva]

Fix #2363. Add /pre- and /post-entrypoint handling

[ChristopherHX]

Thank you for wanting to implement this, I think that I should provide you some pointer where these pre/post steps should be run to not cause some unexpected execution orders

Those pre/pre steps are managed by the following:

act/pkg/runner/action.go

Lines 488 to 651 in 935e4c3

	func hasPreStep(step actionStep) common.Conditional {
	return func(ctx context.Context) bool {
	action := step.getActionModel()
	return action.Runs.Using == model.ActionRunsUsingComposite ||
	((action.Runs.Using == model.ActionRunsUsingNode12 ||
	action.Runs.Using == model.ActionRunsUsingNode16 ||
	action.Runs.Using == model.ActionRunsUsingNode20) &&
	action.Runs.Pre != "")
	}
	}
	func runPreStep(step actionStep) common.Executor {
	return func(ctx context.Context) error {
	logger := common.Logger(ctx)
	logger.Debugf("run pre step for '%s'", step.getStepModel())
	rc := step.getRunContext()
	stepModel := step.getStepModel()
	action := step.getActionModel()
	switch action.Runs.Using {
	case model.ActionRunsUsingNode12, model.ActionRunsUsingNode16, model.ActionRunsUsingNode20:
	// defaults in pre steps were missing, however provided inputs are available
	populateEnvsFromInput(ctx, step.getEnv(), action, rc)
	// todo: refactor into step
	var actionDir string
	var actionPath string
	if _, ok := step.(*stepActionRemote); ok {
	actionPath = newRemoteAction(stepModel.Uses).Path
	actionDir = fmt.Sprintf("%s/%s", rc.ActionCacheDir(), safeFilename(stepModel.Uses))
	} else {
	actionDir = filepath.Join(rc.Config.Workdir, stepModel.Uses)
	actionPath = ""
	}
	actionLocation := ""
	if actionPath != "" {
	actionLocation = path.Join(actionDir, actionPath)
	} else {
	actionLocation = actionDir
	}
	_, containerActionDir := getContainerActionPaths(stepModel, actionLocation, rc)
	if err := maybeCopyToActionDir(ctx, step, actionDir, actionPath, containerActionDir); err != nil {
	return err
	}
	containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Pre)}
	logger.Debugf("executing remote job container: %s", containerArgs)
	rc.ApplyExtraPath(ctx, step.getEnv())
	return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
	case model.ActionRunsUsingComposite:
	if step.getCompositeSteps() == nil {
	step.getCompositeRunContext(ctx)
	}
	if steps := step.getCompositeSteps(); steps != nil && steps.pre != nil {
	return steps.pre(ctx)
	}
	return fmt.Errorf("missing steps in composite action")
	default:
	return nil
	}
	}
	}
	func shouldRunPostStep(step actionStep) common.Conditional {
	return func(ctx context.Context) bool {
	log := common.Logger(ctx)
	stepResults := step.getRunContext().getStepsContext()
	stepResult := stepResults[step.getStepModel().ID]
	if stepResult == nil {
	log.WithField("stepResult", model.StepStatusSkipped).Debugf("skipping post step for '%s'; step was not executed", step.getStepModel())
	return false
	}
	if stepResult.Conclusion == model.StepStatusSkipped {
	log.WithField("stepResult", model.StepStatusSkipped).Debugf("skipping post step for '%s'; main step was skipped", step.getStepModel())
	return false
	}
	if step.getActionModel() == nil {
	log.WithField("stepResult", model.StepStatusSkipped).Debugf("skipping post step for '%s': no action model available", step.getStepModel())
	return false
	}
	return true
	}
	}
	func hasPostStep(step actionStep) common.Conditional {
	return func(ctx context.Context) bool {
	action := step.getActionModel()
	return action.Runs.Using == model.ActionRunsUsingComposite ||
	((action.Runs.Using == model.ActionRunsUsingNode12 ||
	action.Runs.Using == model.ActionRunsUsingNode16 ||
	action.Runs.Using == model.ActionRunsUsingNode20) &&
	action.Runs.Post != "")
	}
	}
	func runPostStep(step actionStep) common.Executor {
	return func(ctx context.Context) error {
	logger := common.Logger(ctx)
	logger.Debugf("run post step for '%s'", step.getStepModel())
	rc := step.getRunContext()
	stepModel := step.getStepModel()
	action := step.getActionModel()
	// todo: refactor into step
	var actionDir string
	var actionPath string
	if _, ok := step.(*stepActionRemote); ok {
	actionPath = newRemoteAction(stepModel.Uses).Path
	actionDir = fmt.Sprintf("%s/%s", rc.ActionCacheDir(), safeFilename(stepModel.Uses))
	} else {
	actionDir = filepath.Join(rc.Config.Workdir, stepModel.Uses)
	actionPath = ""
	}
	actionLocation := ""
	if actionPath != "" {
	actionLocation = path.Join(actionDir, actionPath)
	} else {
	actionLocation = actionDir
	}
	_, containerActionDir := getContainerActionPaths(stepModel, actionLocation, rc)
	switch action.Runs.Using {
	case model.ActionRunsUsingNode12, model.ActionRunsUsingNode16, model.ActionRunsUsingNode20:
	populateEnvsFromSavedState(step.getEnv(), step, rc)
	populateEnvsFromInput(ctx, step.getEnv(), step.getActionModel(), rc)
	containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Post)}
	logger.Debugf("executing remote job container: %s", containerArgs)
	rc.ApplyExtraPath(ctx, step.getEnv())
	return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
	case model.ActionRunsUsingComposite:
	if err := maybeCopyToActionDir(ctx, step, actionDir, actionPath, containerActionDir); err != nil {
	return err
	}
	if steps := step.getCompositeSteps(); steps != nil && steps.post != nil {
	return steps.post(ctx)
	}
	return fmt.Errorf("missing steps in composite action")
	default:
	return nil
	}
	}
	}

hasPreStep should return true if your preentrypoint field is non empty and the type is docker action, similar to post in the later methods.

You could pass the entrypoint to the execasdocker and call it also for pre/post sections of the referenced code

What you have done so far reads like you do

• pre docker
• main docker
• post docker

If you have more than a single step it could look like

• pre checkout
• main checkout
• pre docker
• main docker
• post docker
• main run step
• post checkout

I would expect, that is enshured by the advanced pre post logic written by former maintainers

• pre checkout
• pre docker
• main checkout
• main docker
• main run step
• post docker
• post checkout

[codecov]

Codecov Report

Attention: Patch coverage is 81.39535% with 8 lines in your changes missing coverage. Please review.

Project coverage is 76.37%. Comparing base (5a80a04) to head (4204ed8).
Report is 87 commits behind head on master.

Files	Patch %	Lines
pkg/runner/action.go	81.39%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2377       +/-   ##
===========================================
+ Coverage   61.56%   76.37%   +14.81%     
===========================================
  Files          53       61        +8     
  Lines        9002     7856     -1146     
===========================================
+ Hits         5542     6000      +458     
+ Misses       3020     1300     -1720     
- Partials      440      556      +116     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AndriiChyrva]

Thank you for wanting to implement this, I think that I should provide you some pointer where these pre/post steps should be run to not cause some unexpected execution orders

Those pre/pre steps are managed by the following:

act/pkg/runner/action.go

Lines 488 to 651 in 935e4c3

	func hasPreStep(step actionStep) common.Conditional {
	return func(ctx context.Context) bool {
	action := step.getActionModel()
	return action.Runs.Using == model.ActionRunsUsingComposite ||
	((action.Runs.Using == model.ActionRunsUsingNode12 ||
	action.Runs.Using == model.ActionRunsUsingNode16 ||
	action.Runs.Using == model.ActionRunsUsingNode20) &&
	action.Runs.Pre != "")
	}
	}
	func runPreStep(step actionStep) common.Executor {
	return func(ctx context.Context) error {
	logger := common.Logger(ctx)
	logger.Debugf("run pre step for '%s'", step.getStepModel())
	rc := step.getRunContext()
	stepModel := step.getStepModel()
	action := step.getActionModel()
	switch action.Runs.Using {
	case model.ActionRunsUsingNode12, model.ActionRunsUsingNode16, model.ActionRunsUsingNode20:
	// defaults in pre steps were missing, however provided inputs are available
	populateEnvsFromInput(ctx, step.getEnv(), action, rc)
	// todo: refactor into step
	var actionDir string
	var actionPath string
	if _, ok := step.(*stepActionRemote); ok {
	actionPath = newRemoteAction(stepModel.Uses).Path
	actionDir = fmt.Sprintf("%s/%s", rc.ActionCacheDir(), safeFilename(stepModel.Uses))
	} else {
	actionDir = filepath.Join(rc.Config.Workdir, stepModel.Uses)
	actionPath = ""
	}
	actionLocation := ""
	if actionPath != "" {
	actionLocation = path.Join(actionDir, actionPath)
	} else {
	actionLocation = actionDir
	}
	_, containerActionDir := getContainerActionPaths(stepModel, actionLocation, rc)
	if err := maybeCopyToActionDir(ctx, step, actionDir, actionPath, containerActionDir); err != nil {
	return err
	}
	containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Pre)}
	logger.Debugf("executing remote job container: %s", containerArgs)
	rc.ApplyExtraPath(ctx, step.getEnv())
	return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
	case model.ActionRunsUsingComposite:
	if step.getCompositeSteps() == nil {
	step.getCompositeRunContext(ctx)
	}
	if steps := step.getCompositeSteps(); steps != nil && steps.pre != nil {
	return steps.pre(ctx)
	}
	return fmt.Errorf("missing steps in composite action")
	default:
	return nil
	}
	}
	}
	func shouldRunPostStep(step actionStep) common.Conditional {
	return func(ctx context.Context) bool {
	log := common.Logger(ctx)
	stepResults := step.getRunContext().getStepsContext()
	stepResult := stepResults[step.getStepModel().ID]
	if stepResult == nil {
	log.WithField("stepResult", model.StepStatusSkipped).Debugf("skipping post step for '%s'; step was not executed", step.getStepModel())
	return false
	}
	if stepResult.Conclusion == model.StepStatusSkipped {
	log.WithField("stepResult", model.StepStatusSkipped).Debugf("skipping post step for '%s'; main step was skipped", step.getStepModel())
	return false
	}
	if step.getActionModel() == nil {
	log.WithField("stepResult", model.StepStatusSkipped).Debugf("skipping post step for '%s': no action model available", step.getStepModel())
	return false
	}
	return true
	}
	}
	func hasPostStep(step actionStep) common.Conditional {
	return func(ctx context.Context) bool {
	action := step.getActionModel()
	return action.Runs.Using == model.ActionRunsUsingComposite ||
	((action.Runs.Using == model.ActionRunsUsingNode12 ||
	action.Runs.Using == model.ActionRunsUsingNode16 ||
	action.Runs.Using == model.ActionRunsUsingNode20) &&
	action.Runs.Post != "")
	}
	}
	func runPostStep(step actionStep) common.Executor {
	return func(ctx context.Context) error {
	logger := common.Logger(ctx)
	logger.Debugf("run post step for '%s'", step.getStepModel())
	rc := step.getRunContext()
	stepModel := step.getStepModel()
	action := step.getActionModel()
	// todo: refactor into step
	var actionDir string
	var actionPath string
	if _, ok := step.(*stepActionRemote); ok {
	actionPath = newRemoteAction(stepModel.Uses).Path
	actionDir = fmt.Sprintf("%s/%s", rc.ActionCacheDir(), safeFilename(stepModel.Uses))
	} else {
	actionDir = filepath.Join(rc.Config.Workdir, stepModel.Uses)
	actionPath = ""
	}
	actionLocation := ""
	if actionPath != "" {
	actionLocation = path.Join(actionDir, actionPath)
	} else {
	actionLocation = actionDir
	}
	_, containerActionDir := getContainerActionPaths(stepModel, actionLocation, rc)
	switch action.Runs.Using {
	case model.ActionRunsUsingNode12, model.ActionRunsUsingNode16, model.ActionRunsUsingNode20:
	populateEnvsFromSavedState(step.getEnv(), step, rc)
	populateEnvsFromInput(ctx, step.getEnv(), step.getActionModel(), rc)
	containerArgs := []string{"node", path.Join(containerActionDir, action.Runs.Post)}
	logger.Debugf("executing remote job container: %s", containerArgs)
	rc.ApplyExtraPath(ctx, step.getEnv())
	return rc.execJobContainer(containerArgs, *step.getEnv(), "", "")(ctx)
	case model.ActionRunsUsingComposite:
	if err := maybeCopyToActionDir(ctx, step, actionDir, actionPath, containerActionDir); err != nil {
	return err
	}
	if steps := step.getCompositeSteps(); steps != nil && steps.post != nil {
	return steps.post(ctx)
	}
	return fmt.Errorf("missing steps in composite action")
	default:
	return nil
	}
	}
	}

hasPreStep should return true if your preentrypoint field is non empty and the type is docker action, similar to post in the later methods.

You could pass the entrypoint to the execasdocker and call it also for pre/post sections of the referenced code

What you have done so far reads like you do

• pre docker
• main docker
• post docker

If you have more than a single step it could look like

• pre checkout
• main checkout
• pre docker
• main docker
• post docker
• main run step
• post checkout

I would expect, that is enshured by the advanced pre post logic written by former maintainers

• pre checkout
• pre docker
• main checkout
• main docker
• main run step
• post docker
• post checkout

as far as I understand, execAsDocker(...) is a simple function that is just designed to execute an entry point and that's it.

execJobContainer(...) is calculated to start the nodeis process inside the docker container.

what is happening with composite actions, I did not understand at all. something terrible seems.

if I understand correctly, I need to implement something like execJobContainer(...), docker specific and push it into the main pipeline?

[ChristopherHX]

Feel free to ask me if something is unclear, as English is one of my foreign languages

as far as I understand, execAsDocker(...) is a simple function that is just designed to execute an entry point and that's it.

yes, basically execJobContainer for docker actions

_You have already written the code that can execute each pre, main and post stage for docker actions, so I think it's not much work to select one of the three via an additional parameter to execasdocker.

execJobContainer(...) is calculated to start the nodeis process inside the docker container.

Yes, but only for nodejs actions. execAsDocker should be called there for docker actions
like here for the main stage (this is mirrored for pre and post steps in runPreStep/hasPostStep

act/pkg/runner/action.go

Lines 188 to 193 in 935e4c3

	case model.ActionRunsUsingDocker:
	location := actionLocation
	if remoteAction == nil {
	location = containerActionDir
	}
	return execAsDocker(ctx, step, actionName, location, remoteAction == nil)

what is happening with composite actions, I did not understand at all. something terrible seems.

That is handled by the existing code, therefore not your problem if you would put the change to runPreStep/hasPostStep

I need to implement something like execJobContainer(...), docker specific

yes, probably just varying the following entrypoint is enough

act/pkg/runner/action.go

Lines 324 to 329 in 935e4c3

	if action.Runs.Entrypoint != "" {
	entrypoint, err = shellquote.Split(action.Runs.Entrypoint)
	if err != nil {
	return err
	}
	} else {

push it into the main pipeline?

I don't understand what you mean by this, actions have 3 entrypoints (those are not called sequencially per action except if your job is basically a single docker action) this is implemented only for nodejs and composite actions while docker actions only execute in the main stage at this time