workflows: issue_comment triggers refactoring

[nbusseneau]

Please review per commit :)

[twpayne]

LGTM

We also need to add docs, but that can be in a follow-up PR.

[nbusseneau]

We also need to add docs, but that can be in a follow-up PR.

Too late, I was already doing it :D

[pchaigno]

One concern below, the other is a nit. Also, where was this tested?

[pchaigno]

@nbusseneau Did you validate this with a test commit already?

[nbusseneau]

@nbusseneau Did you validate this with a test commit already?

I looked at the PR history and was like "hmmm, no I did not, why didn't I?" 🤔 So I added a DO NOT MERGE commit and then remembered why I did not: this PR is from my fork, and cannot trigger test runs.

I am opening a separate PR for testing.

[nbusseneau]

Anybody knows why https://github.com/cilium/cilium/actions/runs/1270095584 is failing? I suppose it is due to this:

{[3/5] update MLH config with new `/test` trigger}
  Running on 417c7794d8797687c82bfe31abbe3a1cba8f8258
  Warning: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one
  
  
  NOTE: For some of the reported defects, checkpatch may be able to
        mechanically convert to the typical style using --fix or --fix-inplace.
  
  "[PATCH] update MLH config with new `/test` trigger" has style problems, please review.
  
  NOTE: Ignored message types: C99_COMMENT_TOLERANCE COMMIT_LOG_LONG_LINE COMPLEX_MACRO CONSTANT_CONVERSION CONST_STRUCT EMAIL_SUBJECT FILE_PATH_CHANGES FROM_SIGN_OFF_MISMATCH GIT_COMMIT_ID JIFFIES_COMPARISON LEADING_SPACE LONG_LINE_COMMENT MACRO_WITH_FLOW_CONTROL MULTISTATEMENT_MACRO_USE_DO_WHILE NOT_UNIFIED_DIFF PRINTK_WITHOUT_KERN_LEVEL TRAILING_SEMICOLON TRAILING_STATEMENTS VOLATILE
  
  NOTE: If any of the errors are false positives, please report
        them to the maintainer, see CHECKPATCH in MAINTAINERS.

Do we not allow empty commit descriptions anymore?

[nbusseneau]

All tests in #17465 successfully triggered, however please note that the PR tests the pull_request path and not the issue_comment triggers themselves, so this test only guarantees that the PR changes are syntactically correct and won't break workflows after merge.

[pchaigno]

Do we not allow empty commit descriptions anymore?

cc @qmonnet

[pchaigno]

Reviews are in and required tests are passing. Marking ready to merge.

[qmonnet]

Do we not allow empty commit descriptions anymore?

No.
There was a Slack discussion about that some time ago, and I made checkpatch complain on empty commit logs as a result. Only my checkpatch change introduced a bug and checkpatch would not make the GitHub action fail when detecting errors 😞. Fixed now, so normal checkpatch activity should resume, with complaints on empty commit logs in addition to the previous reports.

[pchaigno]

There was a Slack discussion about that some time ago, and I made checkpatch complain on empty commit logs as a result.

IIRC, the conclusion of that discussion was that empty commits are sometimes useful, as illustrated here. I think we should revert.