Fix: Always run annotation check for PRs#87
Conversation
|
Your description lacks a bit of motivation.. why do you want to run this check if the build obviously already failed? Worded differently: What value does it add? Not saying it is a bad idea, but I am not seeing why the current setup is an issue, and what this PR solves :D |
| needs: | ||
| - build | ||
|
|
||
| if: always() && github.event_name == 'pull_request' |
There was a problem hiding this comment.
This would now only run on PR, but currently it also runs after merge. Is that change intended?
There was a problem hiding this comment.
rw-annotation-check.yml also use if: always() && github.event_name == 'pull_request', so using if: always() here should be enough.
Anyway that means it already only run on PR.
There was a problem hiding this comment.
Okay, so mostly the rw-annotation-check.yml is outdated, and that if condition should be moved up one. So that if condition doesn't actually do what we hope/think it does :) At least, the always() part there is pointless, if I get it right.
Maybe good to just remove the if statement in that workflow. Makes this PR more clear in its intentions, and also forces us to cleanup some other repos that wrongly don't have the if statement before using the rw-annotation-check.yml. If that makes sense :P (please check if it makes sense)
PS: it means these two if statements should remain as they are :) At least, I think :P
There was a problem hiding this comment.
Scratch that: my suggestion, for this PR: replace the rw-annotation-check.yml below with the actual action directly (so the annotation-check). And leave the rw-annotation-check.yml alone for now.
There are three users of that rw-. I think we should fix them to work how they are intended (they miss the if statement), and switch to the annotation-check directly too, much like the other repos like OpenTTD, catcodec, ...
That way, all repos are the same again, with the least amount of effort :P My suggestion, feel free to look around yourself what you think is best. But this did get kinda messy :D
|
If annotation check is always executed, it's possible to only set this check as required for a given reusable workflow. |
Something for a description next time :D Okay, I was missing some context to actually be able to review this PR. You might already gathered this, but for me it was hidden away in how GitHub executes these workflows. It seems, from what I can gather, that reusable workflows only execute their This is a bit unexpected, as I expected the It also means a bunch of other repositories have their workflows wrong. The intention is that the annotation check only runs on Pull Requests, no matter what, if I get you right. But a lot of repos only Took me longer than I would like to admit to understand what was going on; hopefully you already knew and just didn't write it down :P |
b18793a to
af393b1
Compare
Annotation check is skipped if any of the needed tasks fails.
And the required status is waiting for report.
Always run the check for PRs.