ci: run update-changelogs on comment for non-release PRs - #10077
ci: run update-changelogs on comment for non-release PRs#10077cryptodev-2s wants to merge 1 commit into
Conversation
f17f696 to
17aea1a
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 17aea1a. Configure here.
| name: Update changelogs | ||
| needs: is-release | ||
| if: ${{ needs.is-release.outputs.is-release == 'true' }} | ||
| if: needs.is-release.outputs.is-release == 'true' || github.event_name == 'issue_comment' |
There was a problem hiding this comment.
Missing commenter write-access gate
High Severity
Commenting @metamaskbot update-changelogs now reaches update-changelogs, which mints a write token and pushes to the PR branch, but is-fork still accepts any commenter. On this public repository, anyone can start privileged writes on a non-fork PR.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 17aea1a. Configure here.
There was a problem hiding this comment.
This is pre existing
There was a problem hiding this comment.
This comment seems correct, we would need to bail out on forks. Which we aren't doing anymore if we allow it on non-releases.
There was a problem hiding this comment.
Yes, but this is pre-existing behavior. It means that on any PR opened from within the repository, a comment from any person could trigger the workflow. Nothing changes here.
The existing fork check only verifies that the workflow is not running from a forked repository. It does not check whether the person commenting on a non-fork PR is a legitimate/authorized user.
There was a problem hiding this comment.
I mean even previously when it was scope to release PRs only. Anyone could have triggered this by a comment.
On the bot's actual point, it's about who typed the comment, not about forks.
There was a problem hiding this comment.
Side-note: Should we consider running this on draft releases too? It seems a bit confusing you have to bring it out of draft to get the bot to run automatically
There was a problem hiding this comment.
We only do this for non-draft PRs. We assume CI could still be failing, or the person may not have prepared the release correctly or fixed all the constraints (for using the correct versions), so the workflow needs to be safe to run more than once. (assuming people are correctly using the draft state)
There was a problem hiding this comment.
Hmm. I can see both sides on this and don't have a strong opinion. That said even if more people are likely to post a comment than wait for the workflow to run then it would act similar to the "update LavaMoat policies" workflow in Extension, which is an established pattern. So maybe the way you have it is okay?
| name: Update changelogs | ||
| needs: is-release | ||
| if: ${{ needs.is-release.outputs.is-release == 'true' }} | ||
| if: needs.is-release.outputs.is-release == 'true' || github.event_name == 'issue_comment' |
There was a problem hiding this comment.
Do we want to allow update-changelogs to run automatically on non-release PRs too? Let's say someone bumps a dependency in a feature branch. If we are encouraging engineers to update the changelog in advance instead of within release PRs, then it seems that we would want the dependency to be logged in the feature branch and not skipped.
| - Split disparate changes from the same pull request into multiple entries if necessary. | ||
| - Omit reverted changes from the changelog. | ||
|
|
||
| ## Updating changelogs automatically |
There was a problem hiding this comment.
To mention what exactly is being updated in changelogs, I wonder if this should be:
| ## Updating changelogs automatically | |
| ## Generating changelog entries for dependency bumps |
| A few things to know: | ||
|
|
||
| - This works on any pull request, not just release pull requests. Release pull requests also get this automatically when they are opened. | ||
| - If the action pushes a commit, remember to pull it before you push again. |
There was a problem hiding this comment.
Nit:
| - If the action pushes a commit, remember to pull it before you push again. | |
| - If the action pushes a commit to your branch, remember to pull it locally before you push again. |
|
|
||
| ## Updating changelogs automatically | ||
|
|
||
| Some changelog entries are mechanical, such as the entries that record dependency bumps across packages. You can get these written for you instead of writing them by hand: |
There was a problem hiding this comment.
Wondering if "mechanical" is the right word here. Maybe we want:
| Some changelog entries are mechanical, such as the entries that record dependency bumps across packages. You can get these written for you instead of writing them by hand: | |
| Recording dependency bumps within package changelogs can be tedious. You can have these entries generated for you instead of writing them by hand: |
3 participants
Explanation
@metamaskbot update-changelogsnever worked outside release PRs. #8443 documented it as working on any non fork PR, but shipped both comment jobs gated onis-release == 'true', so commenting on a normal PR did nothing. This makes the comment trigger actually work.react-to-commentis-release == 'true' && issue_commentissue_commentupdate-changelogsis-release == 'true'is-release == 'true' || issue_commentRelease PR behavior is unchanged. Opening a non release PR still triggers nothing automatically, you have to ask for it. Fork PRs are still skipped.
Also documented the command in
docs/processes/updating-changelogs.md, since it was undiscoverable.References
Related to #8443
Checklist