Redeliver failed webhooks to account for temporary errors #5877
Conversation
|
This is an alternative to #5876 which I found after preparing the draft based on the boilerplate from official GitHub docs, and probably what we would prefer as we don't need to maintain this in the repo plus it already supports multiple hooks out of the box. |
|
Did you have a look at the code this executes? I haven't and thus cannot really approve this PR. As mentioned in the other PR we should probably test what implications this can have for the various webhooks we have. (We don't want things to happen twice or e.g. re-trigger CI checks based on an old state.) |
.github/workflows/redeliver.yaml
Outdated
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: gateixeira/retrigger-webhook-action@main |
There was a problem hiding this comment.
This action doesn't seem to be an official github action.
Also I think we should never use the main version of a third party action we don't control.
And also the usage
https://github.com/gateixeira/retrigger-webhook-action
seems to be totally different. It should not be a scheduled workflow on its own. It says "Add the following snippet to an existing workflow file".
And I think this should really be tested somehow, maybe you can add a dummy workflow in your fork.
There was a problem hiding this comment.
What existing workflow would this be if not this one? I read existing as any workflow where this action is useful, and the schedule is what GitHub docs recommend.
There was a problem hiding this comment.
The alternative is #5876 where we implement it from scratch.
I agree on adding a version, though.
There was a problem hiding this comment.
What existing workflow would this be if not this one? I read existing as any workflow where this action is useful, and the schedule is what GitHub docs recommend.
I don't know. I don't find the documentation of that action very useful and also not very reassuring :)
There was a problem hiding this comment.
I don't find it worse than many of the Perl modules we rely on to be honest. It was a quick proof of concept either way, so no hard feelings if you're not fond of the general idea of the redelivery approach either.
There was a problem hiding this comment.
I never said that I don't like the idea.
And for a proof of concept we still need the proof :)
| token: ${{ secrets.GH_TOKEN_FOR_ACTIONS }} | ||
| owner: ${{ github.repository_owner }} | ||
| repo: ${{ github.event.repository.name }} | ||
| last_redelivery_variable_name: 'LAST_WEBHOOK_REDELIVERY' |
There was a problem hiding this comment.
Where does that come from? I don't really understand the documentation of the action.
There was a problem hiding this comment.
Also, which webhook should it attempt to redeliver? we have more than one and probably need to give it an id, e.g. like documented webhook_id: 'YOUR_HOOK_ID'
There was a problem hiding this comment.
Ah. I forgot to add the comment here. This is to store the last timestamp in a variable.
There was a problem hiding this comment.
I think we would still need the webhook id.
kalikiana
force-pushed
the
redeliver-gateixeira
branch
from
22a075e to
07067a2
Compare
It is based on the officially documented boilerplate and uses the same pattern to avoid retriggering hooks more than once. |
|
With openSUSE/open-build-service#16834 this shouldn't be necessary anymore, right? |
Ack. |
See: https://progress.opensuse.org/issues/165144