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

Throw warning if CHANGELOG.md file has not been modified #1400

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

phklive
Copy link
Contributor

@phklive phklive commented Jul 16, 2024

In this PR I propose to add a warning if the CHANGELOG.md file has not been modified.

@phklive phklive marked this pull request as draft July 16, 2024 16:31
if git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.sha }} | grep -q "CHANGELOG.md"; then
echo "CHANGELOG.md has been modified. Good job!"
else
echo "::error::CHANGELOG.md has not been modified. Please update the changelog."
Copy link
Contributor

@plafer plafer Jul 16, 2024

Choose a reason for hiding this comment

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

I think a warning is fine, no? Sometimes, we have small PRs (hotfixes or whatnot - such as this one 🙂) that really don't need a changelog entry. It would be nice to not have to merge it with a red CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, let me see if I can make it a little bit less aggressive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobbinth @plafer

SCR-20240716-rdzg

What do you think is the behaviour we should have here?

It seems to me that the warning is too subtle, the job succeeds but throws a warning that most devs wouldn't see.

And plafer says that making the job fail is too aggressive, what do you think is the right thing to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think that's too subtle, and my understanding is that there is no way to show a warning in the PR itself (only failure and success). If so, I think we should show a failure and then the admins can merge the PR anyway if in fact the PR doesn't need the changelog updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I was hoping for some "yellow CI" of some sort, but if this is the only way to do warnings, let's do errors instead

@phklive phklive marked this pull request as ready for review July 16, 2024 19:04
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM! (Assuming we switch back to errors instead of warning)

@phklive phklive merged commit d7955d3 into next Jul 16, 2024
15 of 16 checks passed
@phklive phklive deleted the phklive-changelog-warning branch July 16, 2024 20:16
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.

3 participants