Add GitHub Actions Changelog Bot by ConorMacBride · Pull Request #5811 · sunpy/sunpy

ConorMacBride · 2022-01-11T23:21:47Z

Description

This is a new GitHub Action that commits changelog entries that are in the PR message. This still needs a lot of testing before merging.

Changelog [feature]

Added a new GitHub Action for adding changelog entries.

The bot runs every time the PR message is updated.

Fixes #5415

ConorMacBride · 2022-01-12T00:32:46Z

It's working except for the following error when trying to push the change to the PR branch. It may not be possible to get this working because if it was surely anyone could open a PR with a new GitHub Action to push whatever they want to sunpy/sunpy. Might need to create this as a GitHub App instead.

remote: Permission to ConorMacBride/sunpy.git denied to github-actions[bot].
42
fatal: unable to access 'https://github.com/ConorMacBride/sunpy/': The requested URL returned error: 403

ConorMacBride · 2022-01-12T14:30:59Z

This should probably be added to https://github.com/OpenAstronomy/baldrick instead as an optional plugin. Should be able to implement #4715 in the same plugin also.

ConorMacBride · 2022-01-12T14:59:42Z

There already is a PR! OpenAstronomy/baldrick#95

nabobalis · 2022-02-16T19:25:29Z

There already is a PR! OpenAstronomy/baldrick#95

Does this mean we should close this PR?

ConorMacBride · 2022-02-17T12:44:43Z

They both work in totally different ways and the other PR is a work in progress (and 2 years old). It's worth considering/combining both when expanding the packaging infrastructure.

Signed-off-by: Conor MacBride <conor@macbride.me>

and upgrade some of the actions versions

Cadair · 2022-04-08T14:04:06Z

Wanna merge this and test it here, or you wanna test it on a different repo?

ConorMacBride · 2022-04-08T14:12:22Z

If you like. Are you generally okay with the implementation? Possible downsides: It commits to the PR branch every time a changelog is added, removed or updated, possibly resulting in multiple commits if there are lots of changes. It will also delete any manually committed changelogs for the PR number (unless the no changelog needed tag is added). And it has the issue with people needing to pull the changes back to their local branch.

ConorMacBride · 2022-04-08T14:19:17Z

We could instead create an "Update Changelog" label which, when added, triggers this workflow. (And the workflow would also remove the label, so it can be added again later.) The current behaviour would be very bad for existing PRs as they would get their changelogs deleted when we add labels such as no backport.

Cadair · 2022-04-08T14:22:13Z

+#     markdown heading of "Changelog [category]"
+#     followed by a message in the next ``` code block.
+body = re.sub("(<!--.*?-->)", "", body, flags=re.DOTALL)
+regex = r"[#]+[ ]*Changelog[ ]*\[([a-zA-Z]+)\]*[^`]*```([^`]*)```"


If we don't find anything in body of the PR can we terminate the action from here?

Currently, if you remove the a Changelog from the main PR comment it deletes that Changelog from the PR branch. If we set it to exit here it would make deleting changelogs inconsistent, I.e: if you have two Changelog entries (feature and bugfix) and remove the feature Changelog from the comment it will be deleted in the PR branch, however, if you then remove the bugfix Changelog (so no regex match) it won't be deleted from the PR branch.

Hmmm, that does make sense, but I don't think we should be deleting files people are adding manually either. If we can't delete entries from the PR description because of that, then I think that's a price we would have to pay. Unless we can come up with some other idea.

Cadair · 2022-04-08T14:25:54Z

+    if len(text) > 0:  # ignore empty messages
+        entries += [(filename, text)]
+
+for existing in glob.glob(os.path.join(path, f"{pr}.*.rst")):


I am trying to think of a way we can detect that the action added the file rather than just nuking all changelogs.

Could we use git to pull the commit message of the last commit editing the file and see if we have a unique string in that message? That means that if the last commit wasn't the changelog bot then we don't delete it and instead we error?

Could run e.g. git log -s -n1 --pretty='format:%an %ae%n' changelog/6032.feature.rst on each file before deleting, and only delete if it's the SunPyBot?

Actually, it would be much simpler (and with less surprises) if this workflow can only commit if all the existing changelog entries for the PR were last edited by the bot. (Because otherwise it's complicated to handle multiple changelogs of the same type where some are controlled by the bot and some are manually controlled.)

That would be fine with me as well.

Which would you prefer? Bot command or top comment? Top comment would be easiest to implement (as that's what we have already), although happy to switch to bot command if you would prefer.

I can't find a reliable way to check if the comment author should have permission to push to the PR branch (as the workflow will always have permission). So, if you don't mind, I'll just keep it as updating the top comment so permissions can be enforced. I'll add the check mentioned above.

Cadair · 2022-04-08T14:26:27Z

+if len(entries) == 0:
+    raise ValueError("No new changelog entries found.")


If we did this above the deletion then we wouldn't remove entries in PRs without the magic comment?

Cadair · 2022-04-08T14:27:36Z

The other option is rather than using a magic heading in the top comment we could require you to @ a bot in a comment to make a change to the log entry? i.e.

@SunPyBot add changelog bugfix

adlkjaskljdklajklj

i.e. use https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment

ConorMacBride · 2022-04-08T15:00:17Z

I like the magic heading in the top comment because it makes it easier to update existing entries (rather than copying and pasting) and edit all the changelogs in the one place. Maybe the bot comment is more intuitive though?

Cadair · 2022-04-08T15:23:14Z

I like the idea of the top comment, but it's a stateful interface i.e. handling deletes and edits etc, when the nature of actions is stateless, it's kinda hard to graft the stateful UI onto the stateless actions. Where as responding to specific commands in a comment means that the UI is also stateless.

ConorMacBride · 2022-04-08T15:45:52Z

That makes sense. The comment should work well then. For the API design:

@SunPyBot add changelog <changelog type>

<changelog entry>

If no existing files match <pr number>.<changelog type>(.(0-9)+)?.rst, a new file <pr number>.<changelog type>.rst containing <changelog entry> will be created.

If <pr number>.<changelog type>.rst already exists, it will be renamed <pr number>.<changelog type>.1.rst. The changelog will be created as a new <pr number>.<changelog type>.<n>.rst file containing <changelog entry> where <n> is the lowest integer that doesn't already exist as a filename.

@SunPyBot update changelog <changelog type> [<n>]

<changelog entry>

This will update the contents of <pr number>.<changelog type>[.<n>].rst to be <changelog entry>. If <pr number>.<changelog type>[.<n>].rst does not exist it will error. If multiple changelogs of <changelog type> exist, <n> must be specified.

@SunPyBot delete changelog <changelog type> [<n>]

This will delete the file <pr number>.<changelog type>[.<n>].rst. If <pr number>.<changelog type>[.<n>].rst does not exist it will error. If multiple changelogs of <changelog type> exist, <n> must be specified.

The workflow will react to the comment with 👍 on success and 👎 on error.

Cadair · 2022-04-08T16:12:55Z

Sounds great!

ConorMacBride · 2022-04-08T16:37:14Z

Need a way to ignore comments from people who do not have write access to the PR branch...

ConorMacBride · 2022-04-08T18:35:18Z

The git-auto-commit-action documentation says that a pull_request_target event will run on the fork so the fork needs to have GitHub Actions enabled for their workaround to work. However, this is not what the GitHub documentation says and for example this workflow on a PR from a fork does run on the PR target repo and not on the fork. The difference is, the version of the workflow that is run is the version in the main branch and not the PR branch.

How about we merge this so we can test it?

nabobalis · 2022-04-08T18:36:29Z

How about we merge this so we can test it?

I am pro this.

ConorMacBride mentioned this pull request Apr 6, 2022

gh workflow run not working in scheduled_builds.yml #6044

Closed

ConorMacBride added 6 commits April 8, 2022 14:10

[skip azp] Add Changelog Bot GitHub Actions

9fd7630

Signed-off-by: Conor MacBride <conor@macbride.me>

[skip azp] Apply fixes

20a53b9

Signed-off-by: Conor MacBride <conor@macbride.me>

[skip azp] path to fork

0769ac2

Signed-off-by: Conor MacBride <conor@macbride.me>

[skip azp] Fix file_pattern

42c8853

Signed-off-by: Conor MacBride <conor@macbride.me>

[skip azp] remove add_options

2dfc3ce

Signed-off-by: Conor MacBride <conor@macbride.me>

[skip ci] Update event trigger from pull_request to pull_request_target

0c4bb9c

and upgrade some of the actions versions

ConorMacBride force-pushed the changelog-bot branch from 743b370 to 0c4bb9c Compare April 8, 2022 13:25

Cadair added the No Changelog Entry Needed Skip all changelog checks. label Apr 8, 2022

Cadair reviewed Apr 8, 2022

View reviewed changes

ConorMacBride removed the No Changelog Entry Needed Skip all changelog checks. label Apr 8, 2022

Add check for bot author

ba1e766

ConorMacBride force-pushed the changelog-bot branch from 77998ee to ba1e766 Compare April 8, 2022 18:15

ConorMacBride added the No Changelog Entry Needed Skip all changelog checks. label Apr 8, 2022

ConorMacBride marked this pull request as ready for review April 8, 2022 18:35

nabobalis approved these changes Apr 8, 2022

View reviewed changes

ConorMacBride added the No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) label Apr 8, 2022

nabobalis merged commit 1c315d6 into sunpy:main Apr 8, 2022

ConorMacBride deleted the changelog-bot branch April 9, 2022 18:59

		if len(entries) == 0:
		raise ValueError("No new changelog entries found.")

Uh oh!

Conversation

ConorMacBride commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog [feature]

Uh oh!

ConorMacBride commented Jan 12, 2022

Uh oh!

ConorMacBride commented Jan 12, 2022

Uh oh!

ConorMacBride commented Jan 12, 2022

Uh oh!

nabobalis commented Feb 16, 2022

Uh oh!

ConorMacBride commented Feb 17, 2022

Uh oh!

Cadair commented Apr 8, 2022

Uh oh!

ConorMacBride commented Apr 8, 2022

Uh oh!

ConorMacBride commented Apr 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cadair commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ConorMacBride commented Apr 8, 2022

Uh oh!

Cadair commented Apr 8, 2022

Uh oh!

ConorMacBride commented Apr 8, 2022

Uh oh!

Cadair commented Apr 8, 2022

Uh oh!

ConorMacBride commented Apr 8, 2022

Uh oh!

ConorMacBride commented Apr 8, 2022

Uh oh!

nabobalis commented Apr 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ConorMacBride commented Jan 11, 2022 •

edited

Loading

Cadair commented Apr 8, 2022 •

edited

Loading