Automate changelog updates #547

KshitijThareja · 2024-02-15T12:15:16Z

Description

This PR introduces a new workflow to automate the updates to CHANGELOG.md whenever a PR is merged into KDS. It also adds a modified version of the existing changelog workflow, which now checks for the presence of Changelog section in PRs.

Issue addressed

Addresses #533

Changelog

Description: Introduce a new workflow to automate the updates to CHANGELOG.md whenever a PR is merged into KDS. Also add a modified version of the existing changelog workflow, which checks for the presence of Changelog section in PRs.
Products impact: -
Addresses: -
Components: -
Breaking: no
Impacts a11y: no
Guidance: -

Steps to test

Create a new test PR.
Make sure to include the Changelog section, otherwise the changelog action won't be successful.
When the PR is merged, the Changelog section is automatically appended to the CHANGELOG.md file.

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

After review

The changelog item has been pasted to the CHANGELOG.md

Comments

MisRob · 2024-02-16T06:28:45Z

Thanks @KshitijThareja, amazing! I will take some time to try out the workflow in my test repo t at first and then ask my teammates who are much more skilled in GH actions than me to review in more detail. I'd like to get to it by the beginning of next week, but as usual, it could take up to two weeks.

MisRob · 2024-02-22T18:31:25Z

Hi @KshitijThareja, I tested the main flows like opening/closing/merging PR in my test repository. Both check for the presence of PR section in the PR description and updating the changelog after merging the PR worked smoothly! This will be such an important help for our workflow.

I will try to review code as best as I can some time next week and will invite @rtibbles to give the final approval as my GH actions and bash skills are not great.

MisRob · 2024-02-22T18:34:42Z

I am not sure how to best confirm the dependabot part right now as I didn't find time yet to configure it in my test repo. But if the code looks well, I'd be fine with trying out after merge since @KshitijThareja apparently tested ;)

MisRob · 2024-02-22T18:37:40Z

.github/workflows/update_changelog.yml

+          echo "${pr_link_ref}" >> pr_info.txt
+          echo >> pr_info.txt
+
+          sed -i "/<!-- Put all new updates into this section, please -->/r pr_info.txt" CHANGELOG.md


We will need to use something else to identify the correct place to paste the new item because "Put all new updates into this section, please" was just a temporary comment. I will think about it more and come back. Perhaps we could add some kind of special comment to the changelog that would be obviously intended for this action to prevent ourselves from deleting it accidentally.

Yeah sure. I can make the changes once we've got that part figured out. We just need something to serve as a marker for the workflow.

Thank you, I just posted a suggestion down there

One other thing is that this gets a little more complex when we have a release branch and the main branch in play. At the moment, this action will be pushing all updates to the default (main) branch, but we actually want to check out the target branch of the closed PR instead to make the CHANGELOG updates. I see you are taking that into account below, but not for the checkout, so this will attempt to push to the branch that the PR was based on, but with the default branch, so a release branch will get pushed to be main instead.

MisRob · 2024-02-22T18:49:03Z

.github/workflows/changelog.yml

+        id: check_description
+        run: |
+          description=$(jq -r ".pull_request.body" "$GITHUB_EVENT_PATH")
+          if [[ "$description" == *"## Changelog"* ]]; then


Not required, would just be nice-to-have. People sometimes open a PR and even though they don't delete the Changelog, they leave it unfilled, like this

Right now, the action check will pass. If we could check that for example "Description" was changed from its default value, that would help from merging such items accidentally. Or we could do something else along this line if you have any better idea.

I agree to that. Description is one field which we expect every contributor to fill out. So it makes sense to check if it has been updated or not to further verify the presence of changelog section.
Also, while going through everything, I realized that I should have included a check to see if the PR is made by dependabot in the check-changelog workflow. If we don't have that, the action will keep failing for dependabot PRs.
If you agree to the above changes, I can start working on them.

Yes, that makes sense to me, thank you

while going through everything, I realized that I should have included a check to see if the PR is made by dependabot in the check-changelog workflow

We're okay with merging with checks failing as long as there's a good reason for it, so it wouldn't be anything blocking. Nice to have for sure, as dependabot PRs will be a bit more clear.

Just a note that even when we can have more changelog items in the PR description as I've just mentioned below, it's absolutely okay to only check for the first "Description" being changed since we basically just want to detect that someone changed something.

Got it. I'll try to get these changes done soon 👍🏻

KshitijThareja · 2024-02-23T08:09:24Z

Hi @MisRob. Glad to know that you were able to test the workflows successfully. This PR required me to test the workflows locally multiple times, I guess more than any code I've ever written before 😅... made me realize how important of a step it is.

MisRob · 2024-02-23T11:53:11Z

@KshitijThareja I realized that we can have more changelog items in a PR description (example PR) so to summarize our conversations about recognizing where to copy/paste from/to, I think it'd be reasonable to

Mark beginning and end of changelog section in the PR template

<!-- [DO NOT REMOVE-USED BY GH ACTION] CHANGELOG START -->

<!-- [DO NOT REMOVE-USED BY GH ACTION] CHANGELOG END -->

Mark place below which changelog items should be pasted in the CHANGELOG.md

<!-- [DO NOT REMOVE-USED BY GH ACTION] PASTE CHANGELOG -->

These comments can then safely be used by the action.

rtibbles

A couple of things that need to be considered!

rtibbles · 2024-02-24T01:45:54Z

.github/workflows/update_changelog.yml

+          echo "${pr_link_ref}" >> pr_info.txt
+          echo >> pr_info.txt
+
+          sed -i "/<!-- Put all new updates into this section, please -->/r pr_info.txt" CHANGELOG.md


One other thing is that this gets a little more complex when we have a release branch and the main branch in play. At the moment, this action will be pushing all updates to the default (main) branch, but we actually want to check out the target branch of the closed PR instead to make the CHANGELOG updates. I see you are taking that into account below, but not for the checkout, so this will attempt to push to the branch that the PR was based on, but with the default branch, so a release branch will get pushed to be main instead.

rtibbles · 2024-02-24T01:47:24Z

.github/workflows/update_changelog.yml

+
+on:
+  workflow_run:
+    workflows: ["Check PR for Changelog"]


I don't think we need this trigger here? Only need to react to the pull_request closed event below. This trigger will never pass the github.event.pull_request.merged check (and may even fail as it won't have a pull_request attribute).

Hi @rtibbles. I understood your concern regarding taking the target branch into consideration for checkout.
I'll make the required changes within some days time.

KshitijThareja · 2024-03-02T17:28:41Z

@rtibbles @MisRob I've updated the workflows as requested. Can you please review the changes once?
Thanks :)

KshitijThareja · 2024-03-03T06:24:22Z

@MisRob I had a question. You had earlier provided an example for multiple changelog entries in a single PR. But now that we are removing the PR number and link from the PR template, how is the multiple changelogs scenario going to look like? A PR author won't just start typing the Description for the next changelog entry just below the previous one I presume?

MisRob · 2024-03-04T13:47:40Z

Hi @KshitijThareja

A PR author won't just start typing the Description for the next changelog entry just below the previous one I presume?

I think so, yes

KshitijThareja · 2024-03-08T18:31:26Z

Hi @MisRob!
What should be the next steps here? Is there any workaround for the multiple changelog entries issue?

MisRob · 2024-03-12T11:28:08Z

Hi @KshitijThareja

What should be the next steps here? Is there any workaround for the multiple changelog entries issue?

Do you think that multiple changelog items can be recognized in the action, for example by searching for the first row ("Description") and last row ("Guidance")? I think that "Description" and "Guidance" should always be there. If yes, I think we could preserve the current changelog format with a PR link for each item, right?

If you don't think it's feasible, we can also think about tweaking the changelog format in regards to where PR links are and you're welcome to make suggestions based on what you think would work well with the automation.

KshitijThareja · 2024-03-14T19:24:41Z

Hi @MisRob
Sorry I forgot to follow up here. I was working on correcting the script to identify separate changelog entries. I've added the required changes. The action will now be able to identify and add PR links to each changelog entry in a PR. One modification that I think would be helpful is asking the users to manually separate each changelog entry by mentioning it explicitly in the PR, for example:

Changelog 1:
< CONTENT >

Changelog 2:
< CONTENT >

---and so on.

It would be helpful in increasing the readability in the case of multiple changelogs. What are your views on this?

MisRob · 2024-03-15T18:05:01Z

Thank you @KshitijThareja!

Yes, I think it'd be nice if people could separate changelog entries in a clear manner and we can have a recommendation in comments. I don't think we can rely that it will be present though, but that shouldn't be a problem, is that right? Will the script pick it up in either of those cases?

rtibbles · 2024-03-15T18:06:32Z

I think the reviewer will have to take responsibility for making sure this is formatted properly before merge - the other possibility is that there could be a separate PR action that validates the changelog section (and maybe comments on the PR with what the changelog change will look like).

KshitijThareja · 2024-03-15T18:20:00Z

Will the script pick it up in either of those cases?

Yeah it will be able to pick up separate changelog items in either of those cases.

KshitijThareja · 2024-03-15T18:29:38Z

Hi @rtibbles!
I don't really think there's a need for a separate PR action for this right now, though it may be good to have. We are already providing them with the PR template that shows how the changelog should be written by default. And I assume most PR authors would try to stick to the template.
We can add clear instructions on how the changelog section should be formatted in the PR template itself. What's your opinion on this?

rtibbles · 2024-03-15T18:30:33Z

I don't review a lot of KDS PRs - so of course I am happy to say "the reviewer should do it"! We could also file that automation idea as a follow up issue :)

KshitijThareja · 2024-03-15T18:37:38Z

Sure @rtibbles. I guess it would be better to finalize the current changes first and then have a follow up issue for the automation part. I'd be happy to work on it once this is finalized :)

MisRob · 2024-03-15T18:54:10Z

There's already separate action that checks for the presence of at least one changelog item in the PR description before merging it so we could gradually build on top of that if needed.

Hearing that it the script can pick up more changelog items, no matter whether we title them or no, I feel confident about this version.

Let me give it a test try and then we can proceed to final review and hopefully start using soon :)

Thanks both

MisRob · 2024-03-20T11:55:37Z

Hi @KshitijThareja, I've just started testing in detail the first action that's run on a PR. I copied all code and changelog to this test repo where I opened four PRs

I have observed two issues

On this PR Test PR 2 test-actions#2, the check has passed even it's expected to fail
Updating a PR description in one PR triggers the check to be re-run on all four PRs and with result from the PR that triggered it. For example, updating "Test PR 3" template triggers the check correctly and it is correctly evaluated to "fail". But now not only "Test PR 3" but also 1,2, and 4 are failed. I realized this was caused by me opening four PRs from the same commit :) We don't need to be dealing with that, apologies. I updated PRs to reflect reality better now and this is now looking fine.

KshitijThareja · 2024-03-20T15:22:12Z

Hi @MisRob
Regarding the first issue, I think I know what the error is. It is somehow evaluating "null" return value to true. I'll add another check for that and update the code in some time after testing it.
About the second one, I had started researching as to why that should happen. Thanks for updating that it wasn't an issue. I was basically clueless about what might have caused such a thing to happen.

MisRob · 2024-03-20T16:27:03Z

About the second one, I had started researching as to why that should happen. Thanks for updating that it wasn't an issue. I was basically clueless about what might have caused such a thing to happen.

Ah yeah apologies, I hope you didn't spend long time on it before I figured out. Still learning :)

KshitijThareja · 2024-03-21T07:37:01Z

Hey @MisRob
I've updated the action to identify the presence of Description in changelog correctly. It should work fine now :)

bjester · 2024-04-29T16:14:50Z

@KshitijThareja There are also tradeoffs with Bash too. Bash is great, but for those unfamiliar, managing state across bash scripts and bash functions can be a real challenge. And for example, it looks like you made heavy usage of piping. It can be difficult to debug a long chain of piped commands if one perhaps stops operating as expected.

KshitijThareja · 2024-04-30T17:28:54Z

I guess it's just different perspectives at play. Nevertheless, I understand why using Python/JS for writing these scripts would be the ideal choice in this situation.
I'd also like to thank everyone for participating in this discussion. It was interesting to hear different arguments. In the end, we should go with whatever seems to be the best option to everyone.

MisRob · 2024-05-02T11:40:18Z

Yes @KshitijThareja, I think a group of people who use bash on a daily basis would see it differently. It seems you have some passion for bash :)! Anyways, thanks for being open and thanks to all who took time to contribute in the discussion, and @EshaanAgg for opening it up, once again.

It's good to have clarity around what would be helpful in general for actions being used by the team with our background. I think we can use @rtibbles summary as good guidance for the upcoming work:

Overall, my tendency would be to use Python for file manipulation and string processing (things that tend to be quite easy and concise in Python), and JS via the script action for interacting with the Github API. I do still use direct bash commands occasionally, but only for simple operations like mv, cp, unlink, etc.

As mentioned earlier @KshitijThareja, as soon as we do the preparations on our side to seamlessly integrate these two actions, we will accept the PR as is so you don't need to refactor the whole thing. It's already very helpful work. It seems most of the people here would consider migrating some parts of the script away from bash useful so we will welcome it, but definitely not required for merge. Just let us know what works for you best.

KshitijThareja · 2024-05-05T04:17:21Z

Hi @MisRob!
I'm okay with anything. If it's possible for the team to merge this PR now, we can have follow-up issues to migrate the required parts of the script. Until then, the current changes should serve their purpose. It's already a very large PR, so it'd be better if we work on the new changes separately.

MisRob · 2024-05-08T13:48:18Z

Hi @KshitijThareja, yes I agree it would make sense to do as follow-up, after we merge this PR. We can merge it yet though, that can happen after #609

MisRob · 2024-05-08T13:48:29Z

Thanks a lot!

Resolved

MisRob

Thanks both @rtibbles and @KshitijThareja!

I tested all I could think of in test-actions repository - missing changelog item, one changelog item, multiple changelog items. Haven't noticed any issues except one hiccup where the action to update CHANGELOG.md failed but I wasn't really able to reproduce yet. Let's start using it and we can keep an eye on https://github.com/learningequality/kolibri-design-system/actions for some time to see how it goes.

MisRob · 2024-08-16T06:48:59Z

I merged latest develop, updated the changelog, and added a warning comment to the pull request template

MisRob · 2024-08-16T06:49:56Z

Leaving merging on @AlexVelezLl and @rtibbles in case some branch or releases coordination is needed.

AlexVelezLl

Thank you @MisRob @KshitijThareja @rtibbles! Just had a small concern regarding the note we added in the template. And a minor observation on a step in the check changelog workflow.

.github/pull_request_template.md

AlexVelezLl · 2024-08-19T20:10:24Z

.github/workflows/changelog.yml

+    runs-on: ubuntu-latest
+    steps:
+
+      - name: Checkout code


It seems that we dont need to checkout the code as we are not going to do any operations with the PRs code but with its metadata, so we dont need to checkout the branch for that. I just tested this without this step and the action keeps working fine.

I will leave this for @rtibbles's consideration

Yes, not needed for the changelog check at all.

AlexVelezLl · 2024-08-19T20:45:09Z

.github/workflows/update_changelog.yml

+          with open("CHANGELOG.md", "r") as f:
+              current_changelog = f.read()
+
+          new_changelog = current_changelog.replace("<!-- [DO NOT REMOVE-USED BY GH ACTION] PASTE CHANGELOG -->", final_changelog)


Wow loved this strategy.

MisRob · 2024-08-28T10:59:31Z

I finished all I could here - I think we need @rtibbles input on one of the @AlexVelezLl's comments and then feel free to merge

Trigger from event that has access to secrets. Update changelog formatting by converting string manipulation to Python.

rtibbles · 2024-08-28T14:53:37Z

PR has been updated and rebased to remove conflicts.

…nvisible comment placeholder.

AlexVelezLl · 2024-08-28T18:21:16Z

Thank you all!! Merging

MisRob self-assigned this Feb 16, 2024

MisRob requested review from rtibbles and MisRob February 22, 2024 18:31

MisRob reviewed Feb 22, 2024

View reviewed changes

rtibbles previously requested changes Feb 24, 2024

View reviewed changes

KshitijThareja requested review from rtibbles and MisRob March 15, 2024 09:20

rtibbles force-pushed the 533N branch from f0487e2 to 9250143 Compare August 8, 2024 21:24

MisRob mentioned this pull request Aug 11, 2024

Automate tracking of the community pull requests learningequality/test-actions#21

Closed

MisRob approved these changes Aug 16, 2024

View reviewed changes

AlexVelezLl reviewed Aug 19, 2024

View reviewed changes

KshitijThareja and others added 10 commits August 28, 2024 07:49

Automate changelog updates

769041b

Modify check and update changelog workflows

6d0e92d

Modify update_changelog workflow

6746786

Modify update_changelog

545d218

Update changelog check action to identify null description

595852f

Remove is_dependabot step and add explicit check

b830d83

Update changelog updating to use custom Github App.

57fe44d

Trigger from event that has access to secrets. Update changelog formatting by converting string manipulation to Python.

Rerun on synchronize to ensure new commits pass checks.

5db82b9

Update changelog

1bb3198

Remove unnecessary PR checkout in PR description validation.

ad7d767

rtibbles force-pushed the 533N branch from d8cbe68 to ad7d767 Compare August 28, 2024 14:53

Only insert the new changelog item into the first occurrence of the i…

09215b6

…nvisible comment placeholder.

AlexVelezLl merged commit 7a33753 into learningequality:develop Aug 28, 2024
8 checks passed

MisRob mentioned this pull request Sep 3, 2024

Automate changelog updates #533

Closed

Automate changelog updates #547

Automate changelog updates #547

Conversation

KshitijThareja commented Feb 15, 2024 • edited by AlexVelezLl Loading

Description

Issue addressed

Changelog

Steps to test

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Reviewer guidance

After review

Comments

MisRob commented Feb 16, 2024

MisRob commented Feb 22, 2024

MisRob commented Feb 22, 2024

MisRob Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

KshitijThareja Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KshitijThareja commented Feb 23, 2024

MisRob commented Feb 23, 2024

rtibbles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KshitijThareja commented Mar 2, 2024

KshitijThareja commented Mar 3, 2024

MisRob commented Mar 4, 2024

KshitijThareja commented Mar 8, 2024

MisRob commented Mar 12, 2024

KshitijThareja commented Mar 14, 2024

MisRob commented Mar 15, 2024

rtibbles commented Mar 15, 2024

KshitijThareja commented Mar 15, 2024

KshitijThareja commented Mar 15, 2024

rtibbles commented Mar 15, 2024

KshitijThareja commented Mar 15, 2024

MisRob commented Mar 15, 2024

MisRob commented Mar 20, 2024 • edited Loading

KshitijThareja commented Mar 20, 2024

MisRob commented Mar 20, 2024

KshitijThareja commented Mar 21, 2024

bjester commented Apr 29, 2024

KshitijThareja commented Apr 30, 2024

MisRob commented May 2, 2024

KshitijThareja commented May 5, 2024

MisRob commented May 8, 2024

MisRob commented May 8, 2024

MisRob left a comment

Choose a reason for hiding this comment

MisRob commented Aug 16, 2024

MisRob commented Aug 16, 2024

AlexVelezLl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob commented Aug 28, 2024

rtibbles commented Aug 28, 2024

AlexVelezLl commented Aug 28, 2024

KshitijThareja commented Feb 15, 2024 •

edited by AlexVelezLl

Loading

MisRob Feb 22, 2024 •

edited

Loading

KshitijThareja Feb 23, 2024 •

edited

Loading

MisRob Feb 22, 2024 •

edited

Loading

MisRob Feb 23, 2024 •

edited

Loading

MisRob commented Mar 20, 2024 •

edited

Loading