search

re-actors / alls-green

A check for whether the dependency jobs are all green.
https://github.com/marketplace/actions/alls-green
BSD 3-Clause "New" or "Revised" License
114 stars 12 forks source link

Cancelled jobs result in a failure being reported #10

Closed pradyunsg closed 2 years ago

pradyunsg commented 2 years ago

https://github.com/pypa/pip/actions/runs/3210435710

This triggers a notification if the jobs were auto-cancelled, for example due to concurrency protections.

webknjaz commented 2 years ago

There's nothing to be done about this. If cancelations aren't turned into failures, you'd be getting a lot of surprise branch protection breaches. I don't think there's a way to detect a cancelation happening because of the cancelation group and it'd still be fragile (improperly set up cancelation groups can lead to workflows running against the last PR branch being canceled — skipping them would allow bypassing the branch protection). The only thing that I can advise is turning off the notifications.

webknjaz commented 2 years ago

Here's how cancelation happens, by the way: https://docs.github.com/en/actions/managing-workflow-runs/canceling-a-workflow#steps-github-takes-to-cancel-a-workflow-run

webknjaz commented 2 years ago

I've got a (terrible) idea — you could try adding an always succeeding job with if: always() (or inspect the previous jobs for cancelations) and the workflow (probably) would be successful but the check job would still report the right status in the branch protection. But again... That would be a terrible hack with a lot of possibile surprise effects.

webknjaz commented 2 years ago

By the way, there was a PR https://github.com/re-actors/alls-green/pull/7 that suggested treating cancelled as skipped. I had to reject that implementation because it was breaking some guarantees (the new behavior was unconditional) but stated that it'd be okay to maybe make it optional. Still, it feels dangerous to use this so I'm not sure if we should be enabling this.

pradyunsg commented 2 years ago

I guess the solution to this is to do not cancelled() somewhere, or possibly pass that information through to the action (i.e. explore some way of skipping the all-greens step if the workflow is cancelled).

webknjaz commented 2 years ago

Today, I saw if: ${{ !cancelled() }} in one workflow. Although, I'm pretty sure that it has the same problems.

webknjaz commented 2 years ago

@pradyunsg let's chat about this on the tomorrow's call. I'm not sure if we understand the implications of this the same way.

pradyunsg commented 2 years ago

Nvm, we get notifications for cancelled jobs anyway. 🤦🏽