Add CHANGELOG.md modification check and warning comment - #4217
mario-campos wants to merge 19 commits into
Conversation
Sorry, something went wrong.
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.
Sorry, something went wrong.
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 findingsStale warning persists when changelog changes are reverted · New
Adds a PR check that warns contributors to use change notes instead of editing CHANGELOG.md directly.
Changes:
- Detects CHANGELOG.md modifications.
- 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.
Sorry, something went wrong.
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? |
Sorry, something went wrong.
Yes—one valid finding remains: the warning step is gated on changelog-changed == 'true', so if a later commit reverts the CHANGELOG.md change, the step never runs and the existing warning comment stays stale. It should also run when the file is unchanged and remove any existing marker comment. The shell backticks are escaped, and the automation exclusion checks now include branch criteria. |
Sorry, something went wrong.
There was a problem hiding this comment.
🟢 Approval recommended
The workflow correctly handles comment creation, deduplication, cleanup, and automation exemptions.
0 open findings
3 resolved since last review
🧠 Review effort: Balanced
Sorry, something went wrong.
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.
Sorry, something went wrong.
Co-authored-by: Michael B. Gale <mbg@github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Input-validation, test-coverage, link, and workflow-bootstrap issues remain unresolved.
5 open findingsAvoid resolving the local action from the PR base SHA · New
Use a repository-qualified URL for change-note instructions · New
🧠 Review effort: Balanced
Sorry, something went wrong.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Update link in PR checks workflow to point to the correct README. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Note: the failed check is expected, because it's a consequence of introducing an Action and simoultanously changing the workflow to call said Action in the PR's base ref. For this PR, it's a not possible to do both, but it shan't be a problem once merged. |
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
I have added some detailed comments with some major points of feedback, but they are all broadly aligned with what we discussed elsewhere already about this script/action being too over-engineered.
Sorry, something went wrong.
| action-condition: ${{ needs.other-checks.outputs.changelog-changed }} | ||
| action-if-true: upsert | ||
| action-if-false: delete |
There was a problem hiding this comment.
Rather than making this the responsibility of the post-comment action and thus adding complexity there, this could be achieved with a single action input and if conditions on two post-comment steps in the workflow. The step with the delete action wouldn't need the body so there'd be no meaningful duplication.
Sorry, something went wrong.
| # Derived from the workflow name, job id, and step id/sequence-number so that repeated | ||
| # runs of the same step find the same comment (for upsert/delete), while distinct steps | ||
| # (even within the same job) get distinct markers. | ||
| MARKER: <!-- post-comment:${{ github.workflow }}:${{ github.job }}:${{ github.action }} --> |
There was a problem hiding this comment.
It would probably make sense to make this an input rather than generate it automatically. Although this might be convenient, it leads to issues if we e.g. wanted to have two post-comment steps in one job or it might not have the expected behaviour if we want post-comment steps across different workflows/jobs that should edit the same comment.
Sorry, something went wrong.
| action-condition: | ||
| description: "The business condition ('true' or 'false') that selects which action to perform." | ||
| required: true | ||
| action-if-true: | ||
| description: "Action to perform when action-condition is 'true'. One of: none, insert, upsert, delete." | ||
| required: true | ||
| action-if-false: | ||
| description: "Action to perform when action-condition is 'false'. One of: none, insert, upsert, delete." | ||
| required: true |
There was a problem hiding this comment.
See my other comment, but this seems to be shifting functionality that already exists in workflows into this action and thus adds unnecessary complexity.
Sorry, something went wrong.
| inputs: | ||
| body: | ||
| description: "Full comment markdown." | ||
| required: true |
There was a problem hiding this comment.
Optional for the delete action, if you keep it.
Sorry, something went wrong.
| import { type ApiClient, getApiClient } from "./api-client"; | ||
|
|
||
| /** The set of valid actions that can be performed against a (possibly pre-existing) marked comment. */ | ||
| export const ACTIONS = ["none", "insert", "upsert", "delete"] as const; |
There was a problem hiding this comment.
See my other comments where I advocate for a single action input: I'd remove none, make insert and upsert the same (depending on whether a tag was specified or not), and possibly remove delete. I was quite surprised to find that, on your second test PR, there was no trace of the comment. Rather than deleting it, it might be better to edit it to a message that e.g. acknowledges that there's no problem anymore.
That simplifies this script (there's only one action) and makes it more visible that a problem was resolved.
Sorry, something went wrong.
| /** | ||
| * Validates and parses a required CLI flag's raw string `value`, delegating the actual | ||
| * value-specific validation/conversion to `parse`. | ||
| */ | ||
| function parseFlag<T>( | ||
| flag: string, | ||
| value: string | undefined, | ||
| parse: (value: string) => T, | ||
| ): T { | ||
| if (value === undefined) { | ||
| throw new Error(`Missing required flag --${flag}.`); | ||
| } | ||
| try { | ||
| return parse(value); | ||
| } catch (err) { | ||
| const reason = err instanceof Error ? err.message : String(err); | ||
| throw new Error(`Invalid value '${value}' for --${flag}. ${reason}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
We are currently using Node's parseArgs module (which this script also uses). The validation options there are quite limited, but I am not sure that it makes sense to implement this little argument parsing/validation library in this script. At a minimum, make this something that the pr-checks scripts can share, but really if you see a need for this, we can add a dependency on a more feature-rich library for this kind of thing.
Sorry, something went wrong.
This PR adds a PR check for non-release/mergeback/update-bundle PRs. The check posts a sticky PR comment if CHANGELOG.md has 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.md file in a PR.
In dsp-testing#2, a comment was successfully created.
In dsp-testing#3, I modified CHANGELOG.md to 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