Skip to content

Add actions to compute generated release notes (i.e. PRs list) between tags#505

Merged
AliSoftware merged 12 commits into
trunkfrom
changelog-helper
Jul 24, 2023
Merged

Add actions to compute generated release notes (i.e. PRs list) between tags#505
AliSoftware merged 12 commits into
trunkfrom
changelog-helper

Conversation

@AliSoftware
Copy link
Copy Markdown
Contributor

@AliSoftware AliSoftware commented Jul 18, 2023

Why

These are features that I worked on as part of my Project Thread for Tumblr (Android: #20409, #20434, #20451, #20452; iOS: #25398, #25420, #25459), to generate the list of PRs that went into an alpha/beta/RC build since the last build of the corresponding version.

Typically, we use that to generate a PR list (aka generated release notes) when we create a new alpha, beta or RC build, to:

  • List the PRs that landed between the build we just did on CI and the previous build of the same version

    Example use cases
    • Listing PRs between tag builds/beta/30.2.0.104 we just built and the corresponding previous tag builds/beta/30.2.0.103
    • …or between tag builds/beta/30.2.0.106 we just build after our code freeze promoting alpha to beta; and the previous tag builds/alpha/30.2.0.105 that happened before the code-freeze
    • …or between tag builds/rc/30.2.0.110 we do during submission day and the previous beta that was tagged builds/beta/30.2.0.109
  • Attach that list of PRs / generated release notes to the GitHub Release we generate during those builds (example)

  • Use that list of PRs / generated release notes as the "changelog" text of the corresponding App Center build

How

1️⃣ New action find_previous_tag(pattern:)

This uses git describe to find the last tag that was reachable from the current commit (but that is not the current commit) and that matches the provided pattern.

e.g. find_previous_tag(pattern: 'builds/*/30.5.*') for the tag name convention used by Tumblr-Android, or find_previous_tag(pattern: '30.5-rc-*') for the convention used by e.g. Jetpack

💡 This is typically useful to then call the next action below with the proper previous_tag parameter.

2️⃣ New action get_prs_between_tags (†)

This uses the releases/generate-notes GitHub API to generate for you the markdown list of PRs that landed between two tags.

💡 Note that the markdown list generated by GitHub can be customized using .github/release.yml (example of such a customization in Tumblr-Android, which allows to generate a nicely categorized PR list based on the PR labels)

(†) 💭 I'm open to proposals for a better name for this action, if you feel inspired 🙂

Testing

  • I've added the corresponding specs for both actions, so if CI goes green on those, this should be ok
  • I've tested integrating those actions in Tumblr-Android here and confirmed the code was still properly generating the same changelog after the refactor now using those release-toolkit actions.

@AliSoftware AliSoftware self-assigned this Jul 18, 2023
@AliSoftware AliSoftware force-pushed the changelog-helper branch 2 times, most recently from f23eab8 to e7958e6 Compare July 18, 2023 20:50
Not a breaking change yet but still worth mentioning so that people are ready when the deprecation turns into a breaking change in the future.
Because it was not used by any app client and quite obsolete at that point, and to avoid confusion with my upcoming action to get PRs list from the GitHub API instead
Instead of being in the GitHubHelper
Since the default value was derived from the `BUILDKITE_REPO` env var, while `env_names` is already designed for such use case
@AliSoftware AliSoftware marked this pull request as ready for review July 19, 2023 18:40
@AliSoftware AliSoftware requested review from a team and jkmassel July 19, 2023 18:41
Comment thread MIGRATION.md Outdated
- See `ios_generate_strings_file_from_code`, `ios_extract_keys_from_strings_files`, `ios_download_strings_files_from_glotpress` and `ios_merge_strings_files` for typical replacements.
- The action `ios_get_app_version` now requires a parameter `public_version_xcconfig_file` with the public `.xcconfig` file path instead of relying on the environment variable `PUBLIC_CONFIG_FILE`. While the complete removal of this environment variable is our goal, at this point it is still required by other actions such as `ios_bump_version_release` and `ios_codefreeze_prechecks`.
- The usage of a `Deliverfile` (including its `app_version`) is discouraged -- please use `upload_to_app_store` directly from your `Fastfile` instead. Therefore, the parameter `skip_deliver` from the actions `ios_bump_version_hotfix` and `ios_bump_version_release` has been removed.
- If you were using the `get_prs_list` action, you should consider using `get_prs_between_tags` instead (which relies on the GitHub API and a config file, rather than milestones)
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.

From what I can see, get_prs_list was actually removed so there's not really an option to continue using it?

Suggested change
- If you were using the `get_prs_list` action, you should consider using `get_prs_between_tags` instead (which relies on the GitHub API and a config file, rather than milestones)
- If you were using the `get_prs_list` action, you should use `get_prs_between_tags` instead (which relies on the GitHub API and a config file, rather than milestones)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To me use suggests it's the only solution and also that it would work the same way (as in just switch one for the other). While they are in fact different, as get_prs_list was based on the list of PRs in a given milestone, not the PRs that have been merged between two tags.

While by saying consider using here I meant "either you still want a list of PRs and you might be interested in get_prs_between_tagsbut if you go that route you'll have to change the process used by your app's release process accordingly, to rely on tags not on milestones for that list… or maybe this use of get_prs_list in your client app was just legacy and in fact you haven't been using its result anymore and could be a good occasion to drop it completely"

I considered just renaming get_prs_list you something like get_prs_in_milestone instead and keeping it. But also, to my knowledge, we've stopped using that action a while ago in our client repos, replacing it with a manually-maintained RELEASE-NOTES.txt (that allows developers to provide better context for when those are intended to be used as a base for the final user-facing release notes).

So I'm pretty sure that no repo use it anymore, or if their Fastfile calls it the file it generates isn't used by anyone anymore, so they might as well remove their call to it. And then, maybe consider if get_prs_between_tags could be useful to them… but at potentially a completely different step in the process (like when creating GitHub Pre-Releases for beta builds that happen in the middle of sprints/milestones.

Copy link
Copy Markdown
Contributor

@iangmaia iangmaia Jul 24, 2023

Choose a reason for hiding this comment

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

To me use suggests it's the only solution and also that it would work the same way (as in just switch one for the other)

Indeed, to me that that would imply that the actions were interchangeable -- I didn't realise at first they weren't, thanks for the context.

So I'm pretty sure that no repo use it anymore, or if their Fastfile calls it the file it generates isn't used by anyone anymore, so they might as well remove their call to it. And then, maybe consider if get_prs_between_tags could be useful to them… but at potentially a completely different step in the process (like when creating GitHub Pre-Releases for beta builds that happen in the middle of sprints/milestones.

If teams also started using a manually maintained RELEASE-NOTES.txt, what is the use case for the new action and who's going to use it? Perhaps this is what I'm missing from the MIGRATION.md note 🤔: if I use an older version, why should I change to / adopt the new action (and what to do if I use the old one or not)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point. Short answer is that this get_prs_between_tags is more to generate internal release notes for App Center builds and GitHub Pre-Releases (while get_prs_list was initially aimed to generate the RELEASE-NOTES.txt during a whole sprint to hand over to our freelance writer). But I agree that this might not be clear in this MIGRATION.md paragraph; I'll try to reword this.

Copy link
Copy Markdown
Contributor Author

@AliSoftware AliSoftware Jul 24, 2023

Choose a reason for hiding this comment

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

@iangmaia I tried to improve the wording in 5f6bc24. Let me know what you think.

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.

Yes, it looks much clearer now 👍

#
def generate_release_notes(repository:, tag_name:, previous_tag:, target_commitish: nil, config_file_path: nil)
repo_path = Octokit::Repository.path(repository)
api_url = "#{repo_path}/releases/generate-notes"
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.

TIL that there's such thing in the API, nice!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, very useful one, and thankfully… because without it, it would have been quite a nightmare to correctly implement a similar automatic logic ourselves, especially for cases of complicated git tree graphs (see paaHJt-4Ne-p2).

Strangely there's no built-in shim/wrapper method in the Octokit ruby Client for this API endpoint, hence us having to call the low-level client.post(…) method ourselves to use it 🙃 eh. 🤷

@AliSoftware AliSoftware requested review from a team and iangmaia July 22, 2023 19:30
Copy link
Copy Markdown
Contributor

@iangmaia iangmaia left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@AliSoftware AliSoftware merged commit 0f9ba65 into trunk Jul 24, 2023
@AliSoftware AliSoftware deleted the changelog-helper branch July 24, 2023 16:27
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.

2 participants