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

meta: automate description requests when notable change label is added #47078

Merged
merged 1 commit into from Mar 24, 2023

Conversation

danielleadams
Copy link
Member

@danielleadams danielleadams commented Mar 14, 2023

Adds a GH hook to leave a comment to ask for "Notable Change" descriptions.

Fixes: nodejs/Release#821

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@danielleadams danielleadams added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 14, 2023
@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Mar 14, 2023
@danielleadams danielleadams removed the notable-change PRs with changes that should be highlighted in changelogs. label Mar 14, 2023
@danielleadams danielleadams marked this pull request as ready for review March 14, 2023 02:24
@danielleadams danielleadams added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 14, 2023
@danielleadams danielleadams deleted the notable-change-bot branch March 15, 2023 11:42
@danielleadams danielleadams restored the notable-change-bot branch March 15, 2023 17:44
@danielleadams danielleadams reopened this Mar 15, 2023
@danielleadams danielleadams marked this pull request as draft March 15, 2023 17:45
@danielleadams danielleadams marked this pull request as ready for review March 15, 2023 17:46
@danielleadams danielleadams removed the notable-change PRs with changes that should be highlighted in changelogs. label Mar 15, 2023
Copy link
Member

@BethGriggs BethGriggs left a comment

Choose a reason for hiding this comment

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

LGTM, but unsure on how we can test it before landing.

@MoLow
Copy link
Member

MoLow commented Mar 15, 2023

LGTM, but unsure on how we can test it before landing.

that is what https://github.com/nodejs/node-auto-test is for

@danielleadams danielleadams changed the title chore: automate description requests when notable change label is added meta: automate description requests when notable change label is added Mar 15, 2023
@mhdawson
Copy link
Member

I think new actions will not run on a PR as otherwise that would be an attack vector where anybody who submitted a PR could get us to run things. I think past best practice has been do do whatever testing you can in a personal repo and then land the PR and tweak through follow on PRs as necessary.

@danielleadams
Copy link
Member Author

I'm testing it on my fork and will report back

@danielleadams
Copy link
Member Author

danielleadams commented Mar 16, 2023

Tested here: danielleadams#3

Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

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

I am not sure what the intended action item is based on the message. Is it to post a new comment or to edit the one made by the bot?

- name: Add notable change description
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: gh pr comment ${{ github.event.pull_request.number }} --repo ${{ github.repository }} --body "$NOTABLE_CHANGE_MESSAGE"
Copy link
Contributor

@bnb bnb Mar 16, 2023

Choose a reason for hiding this comment

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

using gh pr comment like this is such a good idea. First time I've seen it, this is awesome.

@danielleadams
Copy link
Member Author

I am not sure what the intended action item is based on the message. Is it to post a new comment or to edit the one made by the bot?

@panva you can do either. See nodejs/Release#821 - the change is to have better descriptions of notable changes so that the release notes are more consistent across release lines (and developers get better overall release notes), so how it's communicated is up to the PR author.

@panva
Copy link
Member

panva commented Mar 16, 2023

I am not sure what the intended action item is based on the message. Is it to post a new comment or to edit the one made by the bot?

@panva you can do either. See nodejs/Release#821 - the change is to have better descriptions of notable changes so that the release notes are more consistent across release lines (and developers get better overall release notes), so how it's communicated is up to the PR author.

Let's come up with a better copy then, one that works for both collaborators who can edit the bot's comment and contributors who don't. I'll try to jot something down later today.

Comment on lines 14 to 18
The notable-changes label has been added by @${{ github.actor }}.
Please include your text below if you'd like to include a more detailed summary in the changelog.
```
[Insert here]
```
Copy link
Member

@panva panva Mar 16, 2023

Choose a reason for hiding this comment

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

Suggested change
The notable-changes label has been added by @${{ github.actor }}.
Please include your text below if you'd like to include a more detailed summary in the changelog.
```
[Insert here]
```
The ${{ github.event.label.url }} label has been added by @${{ github.actor }}.
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

Something along these lines. Feel free to play around with it.

Copy link
Member

Choose a reason for hiding this comment

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

The link will make the label appear like so notable-change PRs with changes that should be highlighted in changelogs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@panva when I opened the PR, the event provided a different url (danielleadams#4 (comment)), so going to replace with the hard coded link.

Copy link
Member

Choose a reason for hiding this comment

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

Sure.

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 24, 2023
@nodejs-github-bot nodejs-github-bot merged commit d612613 into nodejs:main Mar 24, 2023
13 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d612613

@panva
Copy link
Member

panva commented Mar 29, 2023

#47078 (comment)

I don't think this was reflected...

@panva panva added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 29, 2023
@panva
Copy link
Member

panva commented Mar 29, 2023

(testing, altho actions has degraded performance at this time so might take a while for the comment to appear, i'll keep tabs on this and follow up)

@panva panva added notable-change PRs with changes that should be highlighted in changelogs. and removed notable-change PRs with changes that should be highlighted in changelogs. labels Mar 29, 2023
@github-actions
Copy link
Contributor

The https://api.github.com/repos/nodejs/node/labels/notable-change label has been added by @panva.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

@panva panva removed the notable-change PRs with changes that should be highlighted in changelogs. label Mar 29, 2023
@panva
Copy link
Member

panva commented Mar 29, 2023

#47300

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47078
Fixes: nodejs/Release#821
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
PR-URL: #47078
Fixes: nodejs/Release#821
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams added a commit that referenced this pull request Jul 6, 2023
PR-URL: #47078
Fixes: nodejs/Release#821
Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: notable-changes bot comment for summaries
9 participants