Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composite Actions Support ADR #1144

Merged
merged 6 commits into from
Jul 27, 2021
Merged

Composite Actions Support ADR #1144

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4d7988e
draft of composite adr
thboop Jun 10, 2021
6fd6b16
adr feedback
thboop Jun 11, 2021
4a98aab
minor updates
thboop Jun 14, 2021
a70380f
Minor update
thboop Jul 27, 2021
bc9f54f
Rename 0000-composite-actions.md to 1144-composite-actions.md
thboop Jul 27, 2021
a80f9c4
Update 1144-composite-actions.md
thboop Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions docs/adrs/0000-composite-actions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
**Date**: 2021-06-10

**Status**: Accepted

## Context

We released [composite run steps](https://github.com/actions/runner/pull/554) last year which started our journey of reusing steps across different workflow files. To continue that journey, we want to expand composite run steps into composite actions.

We want to support the `uses` steps from workflows in composite actions, including:
- Container actions
- javascript actions
- and even other Composite actions (up to a limit of course!)
- The pre and post steps these actions can generate

## Guiding Principles

- Composite Actions should function as a single step or action, no matter how many steps it is composed of or how many levels of recursion it has
thboop marked this conversation as resolved.
Show resolved Hide resolved
- A workflow author should not need to understand the inner workings of a composite action in order to use it
- Composite actions should leverage inputs to get values they need, they will not have full access to the `context` objects. The secrets context will **not** be available to composite actions, users will need to pass these values in as an input.
- Other Actions should **just work** inside a composite action, without any code changes

## Decisions

### Composite Recursion Limit

We will start with supporting a recursion limit of `3`
thboop marked this conversation as resolved.
Show resolved Hide resolved
That means you can have
```
- Composite Action
- Nested Composite Action
- Deep Nested Composite Action
- uses action step
- runs action step
```
- We are free to bump this limit in the future, the code will be written to just require updating a variable. If the graph evaluates beyond the recursion limit, the job will fail in the pre-job phase (The `Set up job` step).
- A composite actions interface is its inputs and outputs, nothing else is carried over when invoking recursively.

### Pre/Post Steps in nested Actions

- We do not plan on adding the ability to configure a customizable pre or post step for composite actions at this time. However, we will execute the pre and post steps of any actions referenced in a composite action.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback from @david-gang regarding this. Could be very helpful for cleanup scenarios. The workaround is writing a JS/container action to reference from your composite action, but that isn't ideal.

The google cli needs a service account json. I have this json stored in a secret. so i want to write a composite action which writes a secret to a file and a post step where the file is deleted. This is not possible without post build step.

- Composite actions will generate a single pre-step and post-step for the entire composite action, even if there are multiple pre-steps and post-steps in the referenced actions.
thboop marked this conversation as resolved.
Show resolved Hide resolved
- These steps will execute following the same ordering rules we have today, first to run has their pre step run first and their post step run last.
- For example, if you had a composite action with two pre steps and two posts steps:

```
- uses: action1
- uses: composite1
- uses: action2
```

The order of execution would be:

```
- prestep-action1
- prestep-composite1
- prestep-composite1-first-action-referenced
- prestep-composite1-second-action-referenced
- prestep-action2
- the job steps
- poststep-action2
- poststep-composite1
- poststep-composite1-the-second-action-referenced
- poststep-composite1-first-action-referenced
- poststep-action1
```

#### Set-state

- While the composite action has an individual combined pre/post action, the `set-state` command will not be shared.
- If the `set-state` command is used during a composite step, only the action that originally called `set-state` will have access to the env variable during the post run step.
- This prevents multible actions that set the same state from interfering with the execution of another action's post step.
thboop marked this conversation as resolved.
Show resolved Hide resolved

### Resolve Action Endpoint changes

- The resolve actions endpoint will now validate policy to ensure that the given workflow run has access to download that action.
- It will return a value indicated security was checked. Older server versions would lack this boolean. For composite actions that try to resolve an action, if the server does not respond with this value, we will fail the job as we could violate access policy if we allowed the composite action to continue.
- Oder GHES/GHAE customers with newer runners will be locked out of composite uses steps until they upgrade their instance.

### If, continue-on-error, timeout-minutes - Not being considered at this time

- `if`, `continue-on-error`, `timeout-minutes` could be supported in composite run/uses steps. These values were not originally supported in our composite run steps implementation.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to prioritize the if keyword sooner rather than later. Feedback from @david-gang

I see that if is not planned to be supported. This could be helpful for us. for example if i have two workflows which are nearly identical but have a big chunk of identical steps. part of the steps contain if clauses. Now i can't unite them in one action and call this action from both workflows. If the if would be supported i could do it. I know that i can use a javascript action but why would i need javascript if it is just a number of bash steps.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be amazing feature https://github.com/prince-chrismc/jwt-cpp/runs/3448042058

- Browsing the community forums and runner repo, there hasn't been a lot of noise asking for these features, so we will hold off on them.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add a usecase, I think some of the early composite actions we'll see will be wrappers around cache + dependency management.

One of the things we do is to conditionally run npm install depending on if we get a cache hit against the os + node + npm versions, which saves us around 1:30m ontop of the standard npm caching.

This is a lot of boilerplate (especially if you adhere to one task per job/workflow), and easy to get wrong, so conditional support would be massive for this usecase, to avoid the complexities of having to write a full action for what should essentially be a composition of actions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more important use cases for if are executing steps only if appropriate in context (i.e. depending on the action's inputs) and capturing details of a failure. I wrote this just now assuming it would work, and was sad when it didn't:

   - name: Build
      shell: bash
      if: inputs.build_targets != 'skip'
      run: make $BUILD_TARGETS
      env:
        CC: ${{ inputs.compiler }}
        BUILD_TARGETS: ${{ inputs.build_targets }}

    - name: Test
      shell: bash
      if: inputs.test_targets != 'skip'
      run: make $TEST_TARGETS
      env:
        CC: ${{ inputs.compiler }}
        TEST_TARGETS: ${{ inputs.test_targets }}

    - name: Failure details
      shell: bash
      if: ${{ failure() }}
      run: |
        dump_log () {
          if [ -s "$1" ]; then
            echo "::group::$1"
            echo '::stop-commands::resume-commands-50YEO1zJ8HSXH4Zy'
            cat "$1"
            echo '::resume-commands-50YEO1zJ8HSXH4Zy::'
            echo '::endgroup::'
          fi
        }
        dump_log config.log
        for ts in $(find . -name 'test-suite*.log' -printf '%P\n'); do
          dump_log "$ts"
        done

It would work (with slight adjustments) in a workflow file, but then I'd need, let's see, at least six copies of it.

- These values passed as input into the composite action will **not** be carried over as input into the individual steps the composite action runs.
Copy link
Collaborator

@ericsciple ericsciple Jun 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will be more interesting after we support all action types. +1 to keep current work scoped to supporting all action types. Then composite will be more useful.

These will be required in the future as we push on composite actions to be a general step-reuse mechanism.


### Defaults - Not being considered at this time

- In actions, we have the idea of [defaults](https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#defaultsrun) , which allow you to specify a shell and working directory in one location, rather then on each step.
- However, `shell` is currently required in composite run steps
- In regular run steps, it is optional, and defaults to a different value based on the OS.
- We could make it optional in composite run steps, and follow the trend of workflow run steps and add the `defaults` functionality
- However, encouraging action authors to explicitly state the `shell` forces them to do the right thing regarding considering cross platform support.
- We can change this in the future as needed, but for now its largely a convenience feature that adds no new functionality, let's wait on more user feedback before we consider this further.
thboop marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a section to discuss the behavior for local actions. Assuming it's supported with the same limitations today (or we could be more strict). Today local actions don't support pre (ignored)

## Consequences

thboop marked this conversation as resolved.
Show resolved Hide resolved
- Workflows are now more reusable across multiple workflow files
- Composite actions implement most of the existing workflow run steps, with room to expand these in the future