Skip to content

feat(changelog): GitHub Milestones based changelogs - #291

Merged
BYK merged 19 commits into
masterfrom
byk/feat/changelog-from-milestones
Sep 23, 2021
Merged

feat(changelog): GitHub Milestones based changelogs#291
BYK merged 19 commits into
masterfrom
byk/feat/changelog-from-milestones

Conversation

@BYK

@BYK BYK commented Sep 20, 2021

Copy link
Copy Markdown
Member

This patch implements the GitHub Milestones based automatic
changelog generation. It builds up on #285 and #287 and ties
them with GitHub milestone data to generate a changelog that
is composed of logical sections with references to relevant
PRs.

Any remaining items are put under "Various fixes & improvements"
section as an itemized list.

This patch implements the GitHub Milestones based automatic
changelog generation. It builds up on #285 and #287 and ties
them with GitHub milestone data to generate a changelog that
is composed of logical sections with references to relevant
PRs.

Any remaining items are put under "Various fixes & improvements"
section as an itemized list.
@BYK
BYK marked this pull request as ready for review September 20, 2021 19:21
@BYK

BYK commented Sep 20, 2021

Copy link
Copy Markdown
Member Author

Changelog generated for this commit:


Automated Changelog Generation (ongoing)

We now automatically generate changelog entries for the auto changelog policy where none provided, instead of saying "No documented changes". The commits/PRs are grouped by their associated GitHub milestones and the milestone title and description are used in the changelog along with a list of related commits/PRs. Any unaccounted changes are grouped under the "Various improvements and fixes" section.

PRs: #291, #290, #289, #287, #285

Added Maven Target (ongoing)

Added the long-awaited Maven target, full with Android support.

PRs: #271, #275, #276, #270, #258

Added symbol-collector Target

Added target for our very own symbol-collector to collect and upload all native system symbols with Craft.

PRs: #284, #277, #269, #268, #267, #266

Fixed Cocoapods Support

Turns out our Cocoapods target was a bit outdated and broken. We have fixed it in this release! 🥳

PRs: #281, #282

Various fixes & improvements

@iker-barriocanal iker-barriocanal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest renaming changes.ts to something closer to gitChanges. Without context, changes it's harder to understand what is referring to.

Comment thread src/utils/changes.ts
Comment thread src/utils/changes.ts Outdated
Comment thread src/utils/changes.ts
Comment thread src/utils/changes.ts
Comment thread src/utils/changes.ts Outdated
Comment thread src/utils/changes.ts Outdated
Comment thread src/utils/changes.ts
Comment thread src/utils/changes.ts Outdated
@BYK

BYK commented Sep 21, 2021

Copy link
Copy Markdown
Member Author

I suggest renaming changes.ts to something closer to gitChanges. Without context, changes it's harder to understand what is referring to.

I'll rename it to changelog.ts then.

Comment thread src/utils/githubApi.ts
Comment thread src/utils/githubApi.ts
Comment thread src/utils/changelog.ts

@kamilogorek kamilogorek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All the replies make sense, looks good to me

@iker-barriocanal iker-barriocanal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd move the changes related to node versions and tsconfig to different PRs, but not blocking.

Comment thread src/utils/githubApi.ts
@BYK

BYK commented Sep 21, 2021

Copy link
Copy Markdown
Member Author

I'd move the changes related to node versions and tsconfig to different PRs, but not blocking.

This was also on my mind so I'll get that done first before landing.

Thanks a lot @kamilogorek and @iker-barriocanal for the great reviews. Btw. I'll also add some tests before landing as nothing hits these lines of codes during tests and they were buggy during my refactors.

Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/commands/prepare.ts
Comment thread src/utils/githubApi.ts
Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/utils/__tests__/changelog.test.ts
Comment thread src/utils/changelog.ts
Comment thread src/utils/changelog.ts Outdated
Comment thread src/utils/changelog.ts Outdated
): Promise<string> {
const gitCommits = await getChangesSince(git, rev);

const githubCommits = await getMilestoneAndPRFromCommits(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: getMilestonesAndPRsFromCommits since we get plural milestones and PRs (right?) and we have plural in getMilestonesDetails

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or since milestones are nested under PRs: getPRsWithMilestonesFromCommits?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The naming is due to the commit -> PR -> milestone mapping. 1 commit to 1 PR to 1 Milestone. I guess making the other one singular is the right way to go here.

Comment thread src/utils/changelog.ts

@chadwhitacre chadwhitacre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some remaining questions/nits/naming suggestions.

FTR was slightly harder to review because of the changes.ts -> changelog.ts rename. It's a good rename but doing the filename change on a separate PR would've saved me some manual diffing locally to understand what had changed, or perhaps a note calling out the additions.

@BYK

BYK commented Sep 23, 2021

Copy link
Copy Markdown
Member Author

FTR was slightly harder to review because of the changes.ts -> changelog.ts rename.

Sorry about this, realized it too late that GitHub did not recognize the rename properly 🙁

@BYK
BYK enabled auto-merge (squash) September 23, 2021 19:35

@chadwhitacre chadwhitacre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems we're talking past each other on function names. Happy to unblock this and revisit later if necessary.

@BYK
BYK merged commit 9d5a62f into master Sep 23, 2021
@BYK
BYK deleted the byk/feat/changelog-from-milestones branch September 23, 2021 19:57
BYK added a commit that referenced this pull request Sep 24, 2021
In #291 we used level 2 headers for subsections in the auto
generated changelogs. This made them collide with the level 2
version headers, causing our GitHub target to not pick up the
automatically generated changelog. It is also semantically
incorrect.

This patch fixes the issue, introduces constants to determine
these levels, and adds a helper function that utilizes these
constants.
BYK added a commit that referenced this pull request Sep 24, 2021
In #291 we used level 2 headers for subsections in the auto
generated changelogs. This made them collide with the level 2
version headers, causing our GitHub target to not pick up the
automatically generated changelog. It is also semantically
incorrect.

This patch fixes the issue, introduces constants to determine
these levels, and adds a helper function that utilizes these
constants.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants