-
Notifications
You must be signed in to change notification settings - Fork 921
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.
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
Conversation
docs/adrs/0000-composite-actions.md
Outdated
|
||
- `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. | ||
- 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. | ||
- These values passed as input into the composite action will **not** be carried over as input into the individual steps the composite action runs. |
There was a problem hiding this comment.
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.
docs/adrs/0000-composite-actions.md
Outdated
- 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. | ||
|
There was a problem hiding this comment.
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)
As per your comment, adding a use case: I'm working on an action that regularly pulls data from a Slack workspace, processes it and posts some graphs and statistics. Optionally, it can also send a Telegram message, for which I use another existing action. I have to describe how to use this other action and have my own action produce output that can be used with it so users can set up a workflow – if I were able to just integrate the other action, and it looks I will be, then I could hide all the complexity and achieve the same result with just my action having to be set up. In other words: this look great, looking forward to using it 🙂 |
adding a use case: same story for linting rules + actions (identical in every go repo, for example). currently using a fork of https://github.com/varunsridharan/action-github-workflow-sync to distribute actions to each repo. effective secret handling is a must, as well as caching and log visibility. TBH the current setup is not bad, just clunky and has no versions/releases |
docs/adrs/0000-composite-actions.md
Outdated
|
||
### 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
docs/adrs/0000-composite-actions.md
Outdated
|
||
### 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. |
There was a problem hiding this comment.
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.
I have a use case that really wants an explicit post step: caching. I have a fairly complicated gradle cache setup, with post-steps requiring |
Very exciting! Our engineers have been waiting for this for a while now. We would be happy to test any early releases on our self-hosted runners and provide feedback if needed. Thanks again! |
This seems like an area rife with potential for supply chain attacks. Today, if my workflow depends on action However, if action Before actions gain the capability to include other actions, please spend some time considering the potential for attacks through indirect dependencies, and please outline steps GitHub would take to mitigate that potential. For example, here's an idea that could help: Require workflows to opt-in to indirect dependencies, or to allow-list specific indirect dependencies and versions. This would help by keeping workflow maintainers aware of what they are consuming. |
@joeyparrish isn't that a general concern with consuming any tool, since most software tools nowadays have dependencies? I can't see why composite actions increase the attack surface compared to consuming Actions. In fact, people who want to provide the features of composite actions can already achieve it if they use the Actions without the |
What if instead of requiring opt-in for indirect dependencies you required an opt-in for non-static (i.e. non-tag) versions? Think |
@joeyparrish As far as I can tell that is not true in practice. A lot of actions re-tag their smaller updates based on semver so you don't even notice the action has changed. And if the I agree with your points, but the problem actually starts with the first action you use. |
To build on what @kuhnroyal said, having "moving tags" is the officially recommended approach by Github (Actions). A lof of actions re-tag their smaller updates because of a lack of proper semver support. This is also related to the recommendation of adding
Many of us would be really happy with semver being understood by GitHub Action's parser/interpreter. Having the same syntax as npm or yarn seems sensible, since the ecosystem is mainly based on JS. |
You are all correct. Supply chain attacks can occur through node module deps and tag rewrites just as well as the scenario I suggested. Perhaps I had too narrow a view and chose a poor venue. Is there a better place to discuss these potential security issues? |
@joeyparrish what about asking the feature to be integrated into dependabot and the regular security updates that GitHub suggests? Currently, it scans |
For third party actions, we recommend pinning to a sha. We will update this guidance to recommend you do that for actions inside a composite action as well. This mitigates the supply-chain attack.
Good news on this front! Composite actions will also respect the actions policies, so you can state which actions you want to be runnable in your org/repo/enterprise. This allows you to specify specific tags or sha's.
Javascript actions have the same issue when pinning to a major version tag, if a node module updates and is compromised, you would end up having your action compromised. As posted above pinning to a sha mitigates this.
We have a bug bounty program, please submit security vulnerabilities there!
Great idea, Let me dig and see if we can get dependabot to do automatic updates for the
That is correct, but we hope to improve this solution in the future. However, it's out of scope for this feature. |
docs/adrs/0000-composite-actions.md
Outdated
### 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. | ||
- 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I just want to say that this is awesome and I am very excited to use this feature! 💖 |
|
What does this mean? I thought it meant it was ready for use, but I'm still seeing
when I try to do things like this
Edit: never mind, I got it working. Turns out it was a different step that was missing the |
Rendered
We welcome community feedback, please feel free to comment on this ADR!
This ADR outlines the next steps we are taking to change composite run steps into composite actions, and specifically mentions how certain scenarios will work.