Skip to content

dist/tools/check_pr: also error on Github commits#22358

Open
mguetschow wants to merge 4 commits into
RIOT-OS:masterfrom
mguetschow:tools-commit-msg-fixup
Open

dist/tools/check_pr: also error on Github commits#22358
mguetschow wants to merge 4 commits into
RIOT-OS:masterfrom
mguetschow:tools-commit-msg-fixup

Conversation

@mguetschow

@mguetschow mguetschow commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Contribution description

Right now, fixup commits usually just error out because they go over the commit message length limit. Automatic Github commits ("Apply suggestions from code review") just fail because they are missing the colon. Now we show the typical "squash needed" message instead.

Testing procedure

CI should fail on this PR until I remove the fixup commit.

Declaration of AI-Tools / LLMs usage:

AI-Tools / LLMs that were used are:

  • none

@github-actions github-actions Bot added the Area: tools Area: Supplementary tools label Jun 8, 2026
@mguetschow mguetschow added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Jun 8, 2026
@mguetschow mguetschow marked this pull request as draft June 8, 2026 15:01
@mguetschow

Copy link
Copy Markdown
Contributor Author

Ah, overlooked dist/tools/pr_check

@AnnsAnns AnnsAnns left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds solid but as far as I understand your test should not work since it checks out master for the commit-msg CI workflow and not the PR itself 😄

@crasbe crasbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnnsAnns

AnnsAnns commented Jun 8, 2026

Copy link
Copy Markdown
Member

Ah, overlooked dist/tools/pr_check

I think there is still some value to making the commit-msg error clearer though

@crasbe

crasbe commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Ah, overlooked dist/tools/pr_check

I think there is still some value to making the commit-msg error clearer though

image

Clearer?

@mguetschow mguetschow force-pushed the tools-commit-msg-fixup branch from f191401 to 7a16b12 Compare June 8, 2026 15:06
@AnnsAnns

AnnsAnns commented Jun 8, 2026

Copy link
Copy Markdown
Member

Clearer?

In the sense that https://github.com/RIOT-OS/RIOT/actions/runs/27146516631/job/80125660302?pr=22358 would also show a message about it being a fixup.

I'd argue that it might actually make more sense to make commit-msg wait for pr_check though so that it doesn't throw like 3 CI errors for a single issue

@mguetschow mguetschow changed the title dist/tools/commit-msg: Error out on fixup commits dist/tools/check_pr: also error on Github commits Jun 8, 2026
@mguetschow

Copy link
Copy Markdown
Contributor Author

I've changed the proposal now to what I initially wanted to cover: Erroring out on automatic "Github code review application" commits.

@riot-ci

riot-ci commented Jun 8, 2026

Copy link
Copy Markdown

Murdock results

✔️ PASSED

ca292bf Apply suggestions from code review

Success Failures Total Runtime
1 0 1 01m:18s

Artifacts

@mguetschow mguetschow marked this pull request as ready for review June 9, 2026 12:33
@mguetschow mguetschow requested a review from crasbe June 9, 2026 12:33
@crasbe

crasbe commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Can you test that in this PR real quick? Add a bogus commit with "Apply suggestions from code review"?

Comment thread dist/tools/pr_check/no_merge_keywords
Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
@mguetschow

Copy link
Copy Markdown
Contributor Author

Actually I just noted that Github would suggest "Apply suggestion from " for single apply instead of batch-apply. See the last commit to catch both.

Comment thread dist/tools/pr_check/no_merge_keywords Outdated
Comment thread dist/tools/pr_check/no_merge_keywords Outdated
Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
@crasbe crasbe added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants