Composite Run Steps ADR #554
Conversation
|
TODO: Add more relevant/exciting examples |
There was a problem hiding this comment.
First off, I'd like to say a massive "thank you" for looking to introduce this functionality.
We've actually been using a preprocessor to add support for the concept of "commands" within a workflow, and "subflows" (what are named "composite actions" here) was going to be the next thing to be added.
I hope this review is welcome. I was unsure whether to comment or not, as there's no contribution guidelines.
I'm curious about the name "composite". I know it was called this in the original issue, and from a technical point of view, it's correct.
I do wonder if "subflow" might be a better name from the "human" side of things, where less technical people can quickly identify that a subflow within a workflow is "just another set of steps". This goes hand-in-hand with requests for being able to run a series of steps.
|
|
||
| The immediate if condition holds the most importance and only effects the current if condition. If there is no if condition for a step, it defaults to its parent's if condition (if that parent's if condition is not defined, it takes the grandparent's if condition, and so on) | ||
|
|
||
| In the example above, the `always()` condition in the `foo` step applies to the composite action steps `foo2`, `foo3`, and `foo4`. But since `foo3` has an if condition defined, it overrides its parent condition so `foo3` will have an if condition `success()`. Let's say that the `foo2` step fails, then `foo3` will not run since a past step failed and its if condition is `succcess()`, but `foo4` will run because its if condition is inherited from `foo` which is `always()`. |
There was a problem hiding this comment.
I think this will lead to some unexpected results for end users.
My expectation here would be the if statement on the parent is about whether to run the composite action (subflow?).
The composite flow would then open a clean context, with no success or failure state, except those which it creates itself.
This will reduce ambiguity for the authors of composite actions.
# parent
steps:
- run: exit 1
- uses: user/composite@v1 # <--- this will run, as it's marked as always runing
if: always()
---
# user/composite/action.yml
steps:
- run: echo "just succeeding"
- run: echo "I will run, as my current scope is succeeding"
if: success()
- run: exit 1
- run: echo "I will not run, as my current scope is now failing"I can see where it would be useful for a composite action have access to the parent success state, such as for reporting purposes, or custom cleanup actions.
Perhaps it would be worth exposing certain details, through a parent variable?
Example of reporting:
# user/composite/action.yml
steps:
- run: echo "preparing the slack bot..." # <--- This will run, as nothing has failed within the composite yet
- run: slack.post("All builds passing, ready for a deploy") # <-- this will not run, as the parent fails
if: parent.success()
- run: slack.post("A failure has happened, fix things now", alert=true) # <--- This will run, as the parent fails
if: parent.failure()
---
# parent.yml
steps:
- run: exit 1
- uses: user/composite@v1
if: always()There was a problem hiding this comment.
Thanks for the suggestion - I appreciate the example and reasoning behind your concern.
I'll think about this and insert a more thorough response in this thread soon.
Thanks again!
There was a problem hiding this comment.
@cybojenix I agree with most of your suggestions. I'm a little bit confused on the later part of your example.
...
# user/composite/action.yml
steps:
- run: echo "just succeeding"
- run: echo "I will not run, as my current scope is succeeding"
if: success()
- run: exit 1
- run: echo "I will not run, as my current scope is now failing"
Why would
- run: echo "I will not run, as my current scope is succeeding
if: success()
not run in this situation? The previous composite action step does not error.
Did you mean something like:
- run: echo "I will not run, as my current scope is succeeding
if: failure()
?
There was a problem hiding this comment.
Sorry, I accidentally added the "not". I've updated the example.
It's there to demonstrate that within the composite action, the current status is "success", even though the parent's status is failing.
There was a problem hiding this comment.
@cybojenx
I just incorporated your suggestions in the ADR and cited you: https://github.com/actions/runner/blob/users/ethanchewy/compositeADR/docs/adrs/0549-composite-run-steps.md#if-condition
Thank you again for taking time to make this suggestion. Really appreciate it!
|
Thank you for your suggestion for the name Also, yes we are open to community responses! :) The scope of this feature is simply to allow for running multiple run steps in an action which already has a requires some technical complexity and significant changes to the runner codebase (here are our beginning PRs for reference: #438, #557) Eventually, we will want to create another feature which will allow us to call actions within composite actions (aka supporting nesting) like in #438 with the support of Thanks! |
| - id: job2 | ||
| uses: actions/setup-node@v2 | ||
| - uses: actions/checkout@v2 | ||
| needs: [job1, job2] |
There was a problem hiding this comment.
I don't think we support a step level needs: https://github.com/actions/runner/blob/master/src/Sdk/DTPipelines/workflow-v1.0.json
Is this a new addition as a part of this adr?
This should work today with no modifications. The metadata that the marketplace looks at is present in the actions.yml.
It would be good to do this eventually but I would say it is out of scope for this effort. |
|
|
||
| ``` | ||
| NAME2 test3 | ||
| Server development |
There was a problem hiding this comment.
According to this logic, workflow authors won't be able to override environment variables.
On the one hand, I can see how it would be useful for an action author to want full control over their scope, to avoid collisions (I can't think of an example off-hand).
On the other, I can see it as useful for the action author to set a default, with the intention of it being overwritten (all our deploy scripts use the environment variable STAGE to control which environment to manage).
A good compromise here might be to allow the use of input within the top level env.
There was a problem hiding this comment.
For additional clarity in the example, can we also explicitly say what happens when you specify env on the step itself.
My assumption is that it will behave the same as above, though it could be a bit of a surprise to workflow authors when explicitly setting an env doesn't change what the action has set
|
|
||
| ```yaml | ||
| using: 'composite' | ||
| env: |
There was a problem hiding this comment.
Will env be top level, or nested under runs, as it is when writing a docker action?
Does the behaviour also align with how env works for docker actions? (where the workflow can't override runs.env)
| We plan to use environment variables for Composite Actions similar to the parent/child relationship between nested function calls in programming languages like Python in terms of [lexical scoping](https://inst.eecs.berkeley.edu/~cs61a/fa19/assets/slides/29-Tail_Calls_full.pdf). In Python, let's say you have `functionA` that has local variables called `a` and `b` in this function frame. Let's say we have a `functionB` whose parent frame is `functionA` and has local variable `a` (aka `functionB` is called and defined in `functionA`). `functionB` will have access to its parent input variables that are not overwritten in the local scope (`a`) as well as its own local variable `b`. [Visual Example](http://www.pythontutor.com/visualize.html#code=def%20functionA%28%29%3A%0A%20%20%20%20a%20%3D%201%0A%20%20%20%20b%20%3D%202%0A%20%20%20%20def%20functionB%28%29%3A%0A%20%20%20%20%20%20%20%20b%20%3D%203%0A%20%20%20%20%20%20%20%20print%28%22a%22,%20a%29%0A%20%20%20%20%20%20%20%20print%28%22b%22,%20b%29%0A%20%20%20%20%20%20%20%20return%20b%0A%20%20%20%20return%20functionB%28%29%0A%0A%0A%0AfunctionA%28%29&cumulative=false&curInstr=14&heapPrimitives=nevernest&mode=display&origin=opt-frontend.js&py=3&rawInputLstJSON=%5B%5D&textReferences=false) | ||
|
|
||
| Similar to the above logic, the environment variables will flow from the parent node to its children node. More concretely, whatever workflow/action calls a composite action, that composite action has access to whatever environment variables its caller workflow/action has. Note that the composite action can append its own environment variables or overwrite its parent's environment variables. | ||
|
|
There was a problem hiding this comment.
what about all the commands we have, ex: ::set-env:: will it only scope to the composite action, or still affect steps within the job?
There was a problem hiding this comment.
I think following our logic that the workflow author shouldn't know the internal workings of the composite action, we should say that ::set-env:: will only scope to the composite action.
Update: users can use set-env in composite action that will set env variables for workflow.
There was a problem hiding this comment.
Given that actions can set environment variables for the job, it feel like set-env should also work the same way for composite actions.
An usecase would be if someone wants to configure AWS credentials as part of their boilerplate for preparing a deploy.
This involves setting up environment variables.
The workaround here would be for the action author to output everything using outputs, and have the workflow author do set-env themselves.. which is a fair amount of boilerplate, especially that to do it properly you need to handle escaping.
|
|
||
| A composite action in its entirety is a job. You can set both timeout-minutes for the whole composite action or its steps as long as the the sum of the `timeout-minutes` for each composite action step that has the attribute `timeout-minutes` is less than or equals to `timeout-minutes` for the composite action. There is no default timeout-minutes for each composite action step. | ||
|
|
||
| If the time taken for any of the steps in combination or individually exceed the whole composite action `timeout-minutes` attribute, the whole job will fail. If an individual step exceeds its own `timeout-minutes` attribute but the total time that has been used including this step is below the overall composite action `timeout-minutes`, the individual step will fail but the rest of the steps will run. |
There was a problem hiding this comment.
the individual step will fail but the rest of the steps will run. -> the rest of the steps will run base on condition.
|
A question on this PR that’s important from an Enterprise perspective. Not sure if it’s the right PR though. Is this anything that will be part of this work? |
* start * Inputs + Outputs * Clarify docs * Finish Environment * Add if condition * Clarify language * Update 0549-composite-run-steps.md * timeout-minutes * Finish * add relevant example * Fix syntax * fix env example * fix yaml syntax * Update 0549-composite-run-steps.md * Update file names, add more relevant example if condition * Add note to continue-on-error * Apply changes to If Condition * bolding * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md * Syntax support + spacing * Add guiding principles. * Update 0549-composite-run-steps.md * Reverse order. * Update 0549-composite-run-steps.md * change from job to step * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md * Add Secrets * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md * Fix output example * Fix output example * Fix action examples to use using. * fix output variable name * update workingDir + env * Defaults + continue-on-error * Update Outputs Section * Eliminate Env * Secrets * Update timeout-minutes * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md * Fix example. * Remove TODOs * Update 0549-composite-run-steps.md * Update 0549-composite-run-steps.md
In this ADR, I propose building the first version of Composite Actions which will allow users to run multiple run steps with the support of inputs, outputs, environment, and context for use in any steps as well as the if, timeout-minutes, and the continue-on-error attributes for each Composite Action step.
https://github.com/actions/runner/blob/users/ethanchewy/compositeADR/docs/adrs/0549-composite-run-steps.md
This feature will eventually lead us on our way to develop a full functioning Composite Steps feature that addresses #438 (i.e. allowing actions to compose other actions).