Skip to content

Conversation

nbusseneau
Copy link
Member

Please review per commit :)

@nbusseneau nbusseneau added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. labels Sep 16, 2021
@nbusseneau nbusseneau requested review from a team as code owners September 16, 2021 15:57
@nbusseneau nbusseneau force-pushed the pr/workflows-update-triggers branch from de436c8 to f523e99 Compare September 16, 2021 16:25
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

LGTM

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

@nbusseneau
Copy link
Member Author

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

Too late, I was already doing it :D

@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 20, 2021
@pchaigno pchaigno removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 20, 2021
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

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

@nbusseneau nbusseneau force-pushed the pr/workflows-update-triggers branch from f8baa22 to e44285b Compare September 22, 2021 13:40
@nbusseneau nbusseneau requested a review from pchaigno September 22, 2021 13:40
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

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

Ensure all checks use a consistent format.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
For now keep the previous and new triggers as a transition phase.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau nbusseneau force-pushed the pr/workflows-update-triggers branch from cf56912 to 64b4617 Compare September 24, 2021 13:41
This will avoid triggering both regular and 1.10 CI when commenting
`/test-backport-1.10` on a backport PR, while still allowing additional
comments or newlines after the initial test trigger phrase.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
- `test-me-please` replaced with `/test`.
- `build-me-please` replace with `/build`.
- Other triggers as-is but prefixed with `/`.
- Unified race detection trigger phrases with the rest of the document:
  singular race detection jobs will expose their unique trigger phrases
  directly from the PR checks.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau nbusseneau force-pushed the pr/workflows-update-triggers branch from 64b4617 to 3d0bba0 Compare September 24, 2021 13:45
@nbusseneau
Copy link
Member Author

@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 nbusseneau mentioned this pull request Sep 24, 2021
@nbusseneau nbusseneau force-pushed the pr/workflows-update-triggers branch from e501d51 to 3d0bba0 Compare September 24, 2021 13:56
@nbusseneau
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

Do we not allow empty commit descriptions anymore?

cc @qmonnet

@pchaigno
Copy link
Member

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

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 24, 2021
@qmonnet
Copy link
Member

qmonnet commented Sep 24, 2021

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.

@jibi jibi merged commit 0e1613c into cilium:master Sep 27, 2021
@pchaigno
Copy link
Member

pchaigno commented Sep 27, 2021

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.

@nbusseneau nbusseneau deleted the pr/workflows-update-triggers branch July 11, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants