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.

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

Allow matching SHA to be canceled #50

Merged
merged 19 commits into from Jan 11, 2021
Merged

Allow matching SHA to be canceled #50

merged 19 commits into from Jan 11, 2021

Conversation

ericmatte
Copy link
Contributor

@ericmatte ericmatte commented Jan 8, 2021

Hi,

I've started using your action and it is really useful!

However, their was one use case for me that was not working:

  • I have a workflow which temporally build and deploy a version of my app on every pull_request.
  • I have a second workflow which cleanup the temporary deployment on pull_request: types: [closed].
    • That second workflow call your action to cancel any build runs still running after the PR is closed.

The problem is that it will not cancel the latest build run if I merge that commit directly; since they both have the same SHA.

So I've added an option allow_matching_sha (default to false) which can disable that check and allow runs with the same sha on other workflows to be cancelled.

So my second workflow looks like this:

on:
  pull_request:
    types: [closed]

jobs:
  cleanup-after-pr-closed:
    name: Cleanup After PR Closed
    runs-on: ubuntu-latest
    steps:
      - name: Cancel build runs
        uses: ericmatte/cancel-workflow-action@match-sha
        with:
          allow_matching_sha: true
          workflow_id: '######'

Cheers

@styfle
Copy link
Owner

styfle commented Jan 10, 2021

Hi @ericmatte

It seems like this is only relevant for pull_request: closed. I can't think of a reason why you would want this for the push event.

I wonder if it would be better to detect the event type and do this automatically for closed PRs rather than add a new option?

@ericmatte
Copy link
Contributor Author

ericmatte commented Jan 11, 2021

@styfle, yes, in my case, it is only useful on pull_request: closed.

I can't think of a reason why you would want this for the push event.

I could think of one reason: Let's say that you have two workflows which are triggered on push that are both doing long jobs.
If a job has failed or is canceled in one of the workflow, maybe you would want to cancel the other one too:

jobs:
  cleanup-after-pr-closed:
    name: Cancel other workflows on failure
    runs-on: ubuntu-latest
    need: [other-jobs]
    if: failure() # or cancelled()
    steps:
        uses: ericmatte/cancel-workflow-action@match-sha
        with:
          allow_matching_sha: true
          workflow_id: 'other-workflow.yml'

If you don't think that this could be a useful case, I could also change the code like you've suggested and detect the event type to do this only on closed PRs.

Cheers

Copy link
Owner

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Lets rename the option ignore_sha and it should be optional, not required

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
ericmatte and others added 5 commits January 11, 2021 13:54
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
dist/index.js Outdated Show resolved Hide resolved
ericmatte and others added 9 commits January 11, 2021 13:57
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Steven <steven@ceriously.com>
dist/index.js Outdated Show resolved Hide resolved
@styfle styfle merged commit 62b179c into styfle:main Jan 11, 2021
@ericmatte ericmatte deleted the match-sha branch January 11, 2021 22:55
styfle added a commit that referenced this pull request Jan 11, 2021
@styfle styfle mentioned this pull request Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants