CI: check the presence of the changelog line in every pull request

[samueltardieu]

Checking it only in the merge queue leads to deferred failures once the decision to merge has been taken already.

• Testing first without changelog line
• Now that that has failed, adding the changelog line and force-pushing

changelog: none

[samueltardieu]

r? @flip1995

[flip1995]

Oh, I remember why I added this check to the bors run and not the PR now: You can't trigger a workflow on "PR description changed". So if the changelog check fails, you either need to (force) push to the PR, close+reopen it, or go through the GH UI to re-trigger it. The last thing can only be done by maintainers. This can be annoying.

So it shouldn't be a per-requisite for other checks. But even then, a PR might have a failing check and can still be merged, which is a bit weird.

Also a note for future adventurers: The merge queue check must still exist, otherwise it could lead to accidental merges of PRs without a changelog, if the changelog is removed after the check was done.

[flip1995]

I just noticed, that you might be able to use the edited option on a PR to run the changelog check (I think that's "new"). If that works, we don't even need the merge queue check. Can you try that please? (the changelog check would then need to be its own workflow, I think)

[samueltardieu]

I just noticed, that you might be able to use the edited option on a PR to run the changelog check (I think that's "new"). If that works, we don't even need the merge queue check. Can you try that please? (the changelog check would then need to be its own workflow, I think)

I'll do that, looks like the solution is indeed trigerring on the PR [opened, reopened, edited] events. We can even add the enqueued event as an extra check, but that should not be necessary.

[samueltardieu]

@flip1995 It seems to be working fine (I did several edition of the PR body to test it). Once it is merged, "Clippy changelog check / conclusion_changelog (pull_request) " must be made a requested check for putting a PR into the queue.

[samueltardieu]

Also, reopened might not be needed, I added it just in case an old non-merged PR is reopened, to make sure that the test runs.

[samueltardieu]

@flip1995 Do you want me to keep the check in clippy_mq.yaml even if it is useless for new PRs? I was thinking about existing open PRs, which are waiting to be merged, and which might not have run the check.

Edit: I (programmatically) checked every open pull request, and only two of them had no "changelog" line, and both were authored by me… So right now every open pull request has a changelog line, and will be rechecked if this changes, so no need to keep the check in clippy_mq.yaml after all.

[flip1995]

Awesome, thanks for addressing this so quickly and the rigorous checking, that it works!

must be made a requested check for putting a PR into the queue

I'll take care of this 👍

[flip1995]

Let's also run this on

Suggested change

	on:
	on:
	merge_group:

It is a really cheap check. Just to be safe and because I don't know, if you can set a check as "Required" for enqueuing, but not in the merge queue.

[samueltardieu]

Ok. I will have to see if the PR number is available in this case, it should be.

(and I have to remove one unrelated change that went in in another file)

[samueltardieu]

I have renamed the file to clippy_changelog.yaml (instead of clippy_pr_changelog.yaml) since it will check also the merge queue.

[samueltardieu]

@flip1995 Since technically a merge group might aggregate several pull requests, I am not sure we can check the individual PR there, including their body. Would adding the enqueued type on the pull_request event be ok with you? This seems to be what we want.

[samueltardieu]

Or I can make the test be successful if we are in a merge_group event so that the conclusion appears successful.

[marcoieni]

Added this new job to the merge queue requirements after flip asked me to do so in Zulip 👍

[samueltardieu]

@flip1995 I have the merge_group, but I have also configured it so that the PR body is not really checked when in the merge queue because of the aggregate PR there. It should be reported as a success.

What is great is that even if something doesn't work well, this PR will not pass the merge step, so we can amend it if needed, we don't take a large risk. My only remaining question is if the PR_NUMBER: '${{ github.event.number }}' env will kill the merge if no "number" is available at merge time (we won't use it anyway), in which case I'll have to find another way to get it only when needed.

[flip1995]

I think the current version is the way we want to go.

Can you try to skip the test in this PR, to make sure that conclusion_changelog runs, even though the actual check is skipped?

[flip1995]

My only remaining question is if the PR_NUMBER: '${{ github.event.number }}' env will kill the merge if no "number" is available at merge time

I don't think this will be an issues, as the step that requests the PR number will be skipped in the queue.

[samueltardieu]

I think the current version is the way we want to go.

Can you try to skip the test in this PR, to make sure that conclusion_changelog runs, even though the actual check is skipped?

Sure. I'll also try to get a non-existent field from the event in the environment to check how it behaves.

[samueltardieu]

Ok, that went well with a non-step-triggering test, the conclusion was a success.