Repository navigation
Add CHANGELOG.md modification check and warning comment - #4217
mario-campos wants to merge 48 commits into
Conversation
mbg
left a comment
There was a problem hiding this comment.
Couple of initial comments on this draft. I like that this is taking advantage of the setup that the existing fetch-base provides.
I haven't checked if the changelog-check step behaves as expected yet either. One thing I did note though related to it is that the fetch-base step is skipped if github.event.pull_request.user.login != 'dependabot[bot]'. That isn't new in this PR (and we probably shouldn't address it here), but it might be better to check github.triggering_actor instead of github.event.pull_request.user.login. The justification in the comment is that Dependabot doesn't have permission, but it is possible for us to push commits to Dependabot PR branches or re-run workflows on it.
Also agree that it would be good to not spam the comment and instead replace an existing one. It's fairly easy to do that with gh or in a script by leaving a comment (<!-- some distinct marker -->) in the comment that's posted and then looking for that.
There was a problem hiding this comment.
🟡 Changes recommended
Shell command substitution currently prevents warning creation, and comment lifecycle and exclusion logic need correction.
3 open findings
What changed in this PR
Adds a PR check that warns contributors to use change notes instead of editing CHANGELOG.md directly.
Changes:
- Detects
CHANGELOG.mdmodifications. - Adds a sticky warning comment with automation exclusions.
- Consolidates PR comment posting into one job.
| File | Description |
|---|---|
.github/workflows/pr-checks.yml |
Detects changelog edits and posts PR warnings. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Refactor conditions for PR title checks in workflow. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot Are there unresolved findings? |
Yes—one valid finding remains: the warning step is gated on |
mbg
left a comment
There was a problem hiding this comment.
Thanks for changing this to move away from pulling in another action and also moving the step that posts or updates the comment out of other-checks and into (now) post-pr-comments.
I added a few more comments, but the main one is about avoiding more, lengthy-ish shell scripts. It shouldn't be hard to implement in TS in pr-checks and I have included some relevant pointers in my detailed comment about this.
Co-authored-by: Michael B. Gale <mbg@github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mbg
left a comment
There was a problem hiding this comment.
Thank you for taking the time to simplify the script! That's appreciated!
During my review, I went through the changes in the order of:
- The new version of the script
- The reusable action
- The workflow changes
Aside from some minor comments, I was happy with the script and the reusable action which looked good to approve with just the minor suggestions 👍🏻
Then I looked over the workflow changes. There are a couple of points with the checkout/Node setup that probably should be addressed before we merge this. The most critical point is in my last comment about the changelog comment probably getting posted if there's no change to the file to begin with, which is what prompted me to pick "Request changes" here. Partially my bad for suggesting that we simplify the script this much, but hopefully not a huge deal to add that little bit of functionality back in.
This codebase is a tad coupled; it could end up biting us.
Use a boolean --create-if-missing flag, supplied by the composite action by default and omitted for resolution updates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ing concatenation' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
| The `.github/actions/post-comment` action requires callers to supply this value via its | ||
| `marker` input. | ||
| Creates or updates a pull request comment identified by a marker. The marker | ||
| is supplied via `--marker` (or the `.github/actions/post-comment` action's `marker` input) |
There was a problem hiding this comment.
or the
.github/actions/post-commentaction'smarkerinput
Minor: But that just passes a value to --marker?
`--update-only` is disabled by default, whereas `--create-if-missing` should've been enabled by default but was not.
This PR adds a PR check for non-release/mergeback/update-bundle PRs. The check posts a sticky PR comment if
CHANGELOG.mdhas been modified in the PR, warning the author to instead direct their change to a change-note.Testing
In order to test this Action and its script, I forked this repository at dsp-testing/mario-campos-post-comment-test and modified the
CHANGELOG.mdfile in a PR.In dsp-testing#2, a comment was successfully created.
In dsp-testing#3, I modified
CHANGELOG.mdto invoke the comment, and then subsequently reverted the change to successfully invoke the comment deletion.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist