Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork30
format-check: add collect errors job#196
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
5f27c2b to0485300Comparepillo79 commentedSep 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 |
iabdalkader commentedSep 10, 2025
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? |
pennam commentedSep 10, 2025
@iabdalkader see#189 merge is blocked due to "wating status to be reported" |
The Github UI for required checks does not integrate with the workflowsdescribed in the .github directory. Currently jobs that are neverstarted due to conditional execution are not even reported, so the UIshows "expected" and locks up merges.Rework the format-check workflow to first list the modified files thatneed to be checked, and then run the check only on those files. Thisway the test runs on every pull request, but has minimal impact unlessone or more source files are actually modified.Requires a branched version of jidicula/clang-format-action 4.15.0 thatadds 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 UIto show a generic name in the required status checks list.
iabdalkader commentedSep 10, 2025
@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 |
0485300 to846dc36Compare@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. |
| failOnError:false | ||
| verify-core: | ||
| name:Collect job errors |
iabdalkaderSep 10, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just an FYI this works too:name: verify-core. I guess you could also quote the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 itverify-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, it is required for thecheck-files-from parameter. Very odd that the original action requires a folder and then does afind to list files and runsclang-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.
| persist-credentials:false | ||
| -name:Get changed source files that need format check | ||
| id:changed-files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 🙂
iabdalkader commentedSep 10, 2025
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. |
4840a0d intoarduino:mainUh oh!
There was an error while loading.Please reload this page.
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.