Add stop-on-signals and auto-cancellation #149
Conversation
|
👋 ping please. |
| @@ -222,6 +245,12 @@ function githubInputs() { | |||
| const disableGithubEnvVars = | |||
| core.getInput("disable-github-env-vars", { required: false }) === "true"; | |||
|
|
|||
| const stopOnSignals = core | |||
| .getInput("stop-on-signals", { required: false }) | |||
There was a problem hiding this comment.
do we need to set the default value SIGINT here?
There was a problem hiding this comment.
No, because of the default in action.yaml: https://github.com/aws-actions/aws-codebuild-run-build/pull/149/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R46. And it must be this way (vs leaving it required: false in action.yml and doing that defaulting here).
My understanding (correct me if I'm wrong) is:
Putting the default in action.yml means the action is always technically invoked with a value, either the one given by the user or that default. From the perspective of the code here, it will always receive a value.
Confusingly, { required: false } is needed for .getInput to not blow up on signals: '', which is a valid choice the user can make to say "don't do signal handling". Since inputs are always string-typed, getInput would see '' as missing and error if we did { required: true }.
There was a problem hiding this comment.
got it. I asked this question because of the issue I reported in the other comment.
| @@ -118,6 +118,13 @@ The only required input is `project-name`. | |||
| 1. **disable-github-env-vars** (optional) : | |||
| Set to `true` if you want do disable github environment variables in codebuild. | |||
|
|
|||
| 1. **stop-on-signals** (optional) : | |||
There was a problem hiding this comment.
I was trying to test this PR but I couldn't cancel the workflow and the build .
my workflow: https://github.com/taoyong-ty/aws-codebuild-run-build/actions/runs/7731467637/workflow
name: Run CodeBuild Action
on: push
jobs:
codebuild-job:
runs-on: ubuntu-latest
name: CodeBuild Job
steps:
- name: Configure AWS Credentials
uses: aws-actions/configure-aws-credentials@v1
with:
aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
aws-region: us-west-2
- name: Run CodeBuild
id: codebuild
if: always()
uses: freckle/aws-codebuild-run-build@0881951f31dfd749aea98e4dcb1bcc7d8f9dbc05
with:
project-name: aws-codebuild-run-build
buildspec-override: |
version: 0.2
phases:
build:
commands:
- sleep 300
hide-cloudwatch-logs: false
artifacts-type-override: NO_ARTIFACTS
There was a problem hiding this comment.
Can you say a little more about "couldn't cancel the workflow and the build"? The logs seem to have expired for that example. If you do it again, I can take a look.
This has been working well for us since I opened this PR. Here is a recent example,
GitHub cancels the build (it was a fail-fast matrix Job where another Job failed):
You can see the Build did indeed go into Stopped at exactly that time:
And the end of the logs on the CodeBuild side, showing the same:
There was a problem hiding this comment.
I don't now why you have if: always() there, can you try removing it? always() is an anti-pattern anyway because it includes the cancelled status too, which feels important here.
There was a problem hiding this comment.
you are right.
What I saw earlier was that the CodeBuild build was still running, even though the GitHub has acknowledged the cancelling request.
But I am now able to see the expected behavior after removin if: always()!
There was a problem hiding this comment.
Thanks for making the change!
I am ready to approve and merge the change. Could you rebase your commit?
When workflows are cancelled, any started builds are left running. This is not usually desired, for example if using workflow concurrency[^1] to cancel redundant builds. In cases like this, we'd want to also stop any triggered builds. This commit adds a `stop-on-signals` input with a default value of `SIGINT` and updates the action to listen for the listed signal(s) and stop any already-started build in response. This accomplishes the desired behavior in the common case. [^1]: https://docs.github.com/en/actions/using-jobs/using-concurrency#example-using-concurrency-to-cancel-any-in-progress-job-or-run
When workflows are cancelled, any started builds are left running. This is not usually desired, for example if using workflow concurrency1 to cancel redundant builds. In cases like this, we'd want to also stop any triggered builds.
This commit adds a
stop-on-signalsinput with a default value ofSIGINTand updates the action to listen for the listed signal(s) and stop any already-started build in response. This accomplishes the desired behavior in the common case.Footnotes
https://docs.github.com/en/actions/using-jobs/using-concurrency#example-using-concurrency-to-cancel-any-in-progress-job-or-run ↩