-
-
Notifications
You must be signed in to change notification settings - Fork 266
ci: Add action to validate changelog diffs after merging #7525
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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 a plain JS file so we can just call node check-changelog-diff.cjs without installing dependencies etc.
.github/actions/check-merge-queue-changelogs/check-changelog-diff.cjs
Outdated
Show resolved
Hide resolved
| id: changelog-check | ||
| shell: bash | ||
| env: | ||
| HEAD_REF: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }} |
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 seems to be unused
| HEAD_REF: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }} |
| HEAD_REF: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }} | ||
| BASE_REF: ${{ github.event.pull_request.base.ref || github.event.merge_group.base_ref }} | ||
| PR_BRANCH: ${{ steps.pr-branch.outputs.pr-branch }} | ||
| REPOSITORY: ${{ github.repository }} |
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 seems to be unused
| REPOSITORY: ${{ github.repository }} |
| BASE_REF="${BASH_REMATCH[1]}" | ||
| fi | ||
|
|
||
| TARGET_REF=$(git merge-base "origin/$BASE_REF" "origin/$PR_BRANCH") |
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'm not following how this ref will help us find changelog updates. It looks like we're always getting the same diff we see in the PR here, so it would miss changes due to updates on the base branch.
e.g. if I'm fixing a bug in the network-controller, then a release happens and moves that under a release header, we won't see that here. The TARGET_REF will be the commit on main before the release, and the PR_BRANCH won't have the release on it either (the branch wasn't updated since the release; if it was, the pre-existing changelog linter would have caught this!), so we're comparing two commits here that are both too old to catch the problem.
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.
It seems like what we want here is to compare the changelog in the PR with what it would be after merge.
The "before" case here would be PR_BRANCH, and the "after" would be the commit that triggered the workflow (which is post-merge for both merge_group and pull_request)
| } | ||
|
|
||
| /* eslint-disable n/no-sync */ | ||
| // The type of these is inferred as `Buffer` when using "utf-8" directly instead |
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.
Is this comment from an earlier draft when this was written in TypeScript?
Explanation
This fixes an issue in changelogs where changelog entries may be incorrectly moved after merging into the main branch. Given the following (partial) changelog for example:
After a release, this changelog may look like this:
## [Unreleased] + ## [1.0.0] ### Changed - Changelog entry A ([#1234](...)) - Changelog entry B ([#5678](...)) - Changelog entry C ([#1337](...))But if another PR based on the original changelog also adds an entry to the unreleased section like this:
## [Unreleased] ### Changed - Changelog entry A ([#1234](...)) - Changelog entry B ([#5678](...)) - Changelog entry C ([#1337](...)) + - Changelog entry D ([#0000](...))The final changelog will look like this, since Git can merge the entry based on the other entries around it:
Here it appears that the release included "entry D", but in reality that was meant to be under the unreleased section.
To address this issue, I've added a simple workflow that checks for changelog diffs. It uses a minimal algorithm to parse the markdown files, find entries that are included in unreleased originally, and checks if they still are after merging. If not, the workflow will fail and show an error such as this:
Long term we should solve this problem by using changesets instead, in which case the unreleased changes will never be merged or conflict with each other, but until we figure out how to implement those in our release workflows, we can use this workflow.
References
Checklist
Note
Adds a CI action and job that verify PR-added changelog entries remain in the Unreleased section after merge (including merge queue).
./.github/actions/check-merge-queue-changelogs:pull_requestandmerge_group).CHANGELOG.mdfiles.check-changelog-diff.cjsto ensure PR-added lines stay in[Unreleased]after merge../.github/workflows/lint-build-test.yml:validate-changelog-diffsjob (PRs and merge groups) that uses the new action.Written by Cursor Bugbot for commit 04224ca. This will update automatically on new commits. Configure here.