Skip to content
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

CI: add changelog check #2766

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Conversation

cyqsimon
Copy link
Contributor

@cyqsimon cyqsimon commented Nov 3, 2023

This workflow ensures that every PR has a corresponding entry in CHANGELOG.md that includes the PR number and the submitter ID.

@cyqsimon cyqsimon force-pushed the ci-check-changelog branch 4 times, most recently from 4db9716 to 9acb347 Compare November 3, 2023 03:46
@cyqsimon
Copy link
Contributor Author

cyqsimon commented Nov 3, 2023

I apologise for the messy commit log. You may want to use squash merge 😅.

@Enselic
Copy link
Collaborator

Enselic commented Nov 3, 2023

No worries :) Feel free to force-push to fix it! After a first code review it can be good to avoid force push though, to make incremental review easier.

This seems like a good idea, but we need to make an exception for auto-merged dependabot PRs. Example: #2761. Either based on branch (starts with dependabot/...) or based on that the author is dependabot. We handle this by drafting release notes right before a release: https://github.com/sharkdp/bat/blob/master/doc/release-checklist.md#changelogmd-updates. But it's nice if the CHANGELOG is prepared as much as possible anyway.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Nov 3, 2023

Rebased on master.

Fetch the base branch and diff correctly

- See: actions/checkout#160

Improve organisation

Add some echo for easier debugging

Fix bad variable syntax

Clarify the double-print in the success case
@cyqsimon
Copy link
Contributor Author

cyqsimon commented Nov 3, 2023

git rebase -i master and squashed some messy commits.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Nov 3, 2023

Ah shit. I think I messed up. branches-ignore: refers to the target branch, not the source branch.

@cyqsimon cyqsimon force-pushed the ci-check-changelog branch 3 times, most recently from fd311cc to eb09088 Compare November 3, 2023 06:56
@Enselic
Copy link
Collaborator

Enselic commented Nov 3, 2023

This is hard to verify before it's merged I suppose? So I'm tempted to merge it after you fix the comment so we can verify and iterate on it.

@Enselic
Copy link
Collaborator

Enselic commented Nov 3, 2023

Oh wait, right, it runs already (after you fix the bug)

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Nov 3, 2023

This is hard to verify before it's merged I suppose? So I'm tempted to merge it after you fix the comment so we can verify and iterate on it.

I made sure this workflow errors properly before I added the changelog for this PR. So if it passes now (as it should), I think it should be mostly fine.

@Enselic Enselic merged commit bcc2de8 into sharkdp:master Nov 3, 2023
22 checks passed
@cyqsimon cyqsimon deleted the ci-check-changelog branch November 3, 2023 08:15
@Enselic
Copy link
Collaborator

Enselic commented Dec 1, 2023

@cyqsimon Hi! Any chance you could fix a bug in the script? Right now it uses the last commit in a PR to determine author. That will give the wrong author if a maintainer adds a commit to the PR. We likely need to use gh pr view 2779 --json author | jq -r .author.login or something similar. Thanks in advance!

Example of where the check fail when it shouldn't: #2779

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Dec 1, 2023

Arrrrgh. There's always an edge case that slips by. I'll take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants