format-check: add collect errors job #196
Conversation
pillo79
force-pushed
the
pr-fix-format-check-ui
branch
11 times, most recently
from
5f27c2b to
0485300
Compare
|
@iabdalkader @pennam fixed CI one more time, hopefully for good. This change ensures jobs always run and always fail / skip so that the Github rules are followed. See #197 for a failing case. Had to duplicate the list of filters because |
I don't understand the issue. Maybe it would help if you let us know how to reproduce it? Would any PR with broken code or formatting trigger it? |
|
@iabdalkader see #189 merge is blocked due to "wating status to be reported" |
The Github UI for required checks does not integrate with the workflows described in the .github directory. Currently jobs that are never started due to conditional execution are not even reported, so the UI shows "expected" and locks up merges. Rework the format-check workflow to first list the modified files that need to be checked, and then run the check only on those files. This way the test runs on every pull request, but has minimal impact unless one or more source files are actually modified. Requires a branched version of jidicula/clang-format-action 4.15.0 that adds the 'check-files-from' input parameter. Signed-off-by: Luca Burelli <l.burelli@arduino.cc>
Do not set a name for the verify-core job, as this causes the GitHub UI to show a generic name in the required status checks list.
@pennam @pillo79 Does this fix the issue #198? It requires less changes, and no I tested it in two PRs with/without code changes, both pass: #198 No code changes -> pass |
pillo79
force-pushed
the
pr-fix-format-check-ui
branch
from
0485300 to
846dc36
Compare
|
@iabdalkader @pennam please re-review. In addition to fixing CI, this now limits the check to the actual files changed by the PR, so that unrelated files do not cause blocks. It does also show the error in the summary and as a Github note, to simplify locating it. Going to push my changes upstream so that hopefully will be switching back to them. |
| @@ -184,7 +184,6 @@ jobs: | |||
| failOnError: false | |||
|
|
|||
| verify-core: | |||
| name: Collect job errors | |||
There was a problem hiding this comment.
Just an FYI this works too: name: verify-core. I guess you could also quote the name.
There was a problem hiding this comment.
Yes it's the same. I hate that GH matches the actual string generated by the job and not some unmistakable ID. At least in the UI it verify-core looks clearer now as a requirement.
| - name: Run clang-format check | ||
| uses: jidicula/clang-format-action@v4.15.0 | ||
| if: steps.changed-files.outputs.any_changed == 'true' | ||
| uses: pillo79/clang-format-action@05f671e71f0758aba4d3c9dbb0ee81bc5f0137c6 |
There was a problem hiding this comment.
I suppose this is now needed for the action to work with these changes? Kind of an issue if your changes don't make it upstream.
There was a problem hiding this comment.
Yes, it is required for the check-files-from parameter. Very odd that the original action requires a folder and then does a find to list files and runs clang-format on each, but did not support specifying the files directly 🙂
There's also a bug that prevents showing the expected changes on the error annotation as expected, so I added them back.
The project looks active and maintained so I'm pretty sure this can be merged, especially if we show it in use.
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| submodules: false | ||
| persist-credentials: false | ||
|
|
||
| - name: Get changed source files that need format check | ||
| id: changed-files |
There was a problem hiding this comment.
This is okay, but is running the formatter on all PRs unconditionally that bad? Most of the PRs will likely contain code changes anyway.
There was a problem hiding this comment.
I kept the original idea of limiting the execution to PRs that needed it. Not a problem right now, but PRs being blocked due to compliance on unrelated files is so much of an issue in Zephyr proper that they don't enable this kind of filters 🙂
|
This does fix the issue, however kinda adds a dependency on your custom action. I prefer we just keep it simple, at least for now, and run the formatter workflow unconditionally ( as done in #198). Otherwise, I don't really have any other feedback, the choice is yours. |
The Github UI for required checks does not integrate with the workflows described in the .github directory. Currently jobs that are never started due to conditional execution are not even reported, so the UI shows "expected" and locks up merges.
Collect format errors so that the Github UI can be happy to track one job that is always executed. This job will fail if any of the format-check jobs failed or were cancelled, and will be ignored (but reported) otherwise.