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

[Tooling] Introduce publish_release lane #3382

Open
wants to merge 4 commits into
base: release/7.79
Choose a base branch
from

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Dec 18, 2024

Note

This is a re-opening of #3371 because the initial changes were accidentally commited to the wrong branch (merge/release-7.79-into-main instead of tooling/publish_release) when I initially worked on them (and I wasn't attentive enough to spot my mistake when I pushed them and opened the PR 😅 ).

Description

Introduce a publish_release lane to automate:

  • The publication of the GitHub Release of the final build (once the version submitted to Google on Friday have been approved)
  • The creation of the final backmerge PR if needed(†)
  • The deletion of the release branch
(†) Note that in 95% of cases there won't be anything to backmerge—and the creation of that backmerge PR will be automatically skipped.
  • This is because there had already been a backmerge PR created during submission on Friday and no new commit should have been added to the release/* branch after submission.
  • But calling create_release_backmerge_pull_request anyway is more of safety net in case there would have been a new commit on release/* anyway since, to make sure not lose it completely accidentally once we delete the release/* branch
  • Rare cases where this could happen is if a commit was added to change tooling-related files (Fastlane, Gemfile.lock, …) after the submission to fix issues encountered in the tooling, or update of the CHANGELOG.md file if one notice after having submitted the build that a PR was forgotten to be added to it, etc. Those seldom cases would not require a resubmission of the final build so that'd be ok for them to land even after the submission was done, and in those rare cases a backmerge PR would be created to propagate those properly instead of risking to lose those changes upon release/* branch deletion

This PR also:

  • Reorders lanes so that lanes called by Release Manager as part of release scenario steps are all grouped together at the top.
  • Renames GH_REPOSITORY constant to GITHUB_REPO, for consistency with our other repos

Testing Instructions

Won't really be able to test those without doing a real final release publication as part of a Release Scenario, given this lane creates side-effects.

Important

I've made this PR target release/7.79 so that we could use the occasion of the 7.79 publication to test that this lane works as expected if we want. Even if 7.79 won't be sent to Production track in Google Play this time around due to the EOY break (internal ref: pdeCcb-85s-p2), we will probably want to publish the GitHub Release and delete the release/7.79 branch at the end of its scenario—which is what this publish_release branch is all about.

That being said, if this PR is not ready to be merged by the time release/7.79 is ready to be finalized, it's ok to make it target 7.80 instead.

Next Steps

I will create the corresponding diff for the Release Scenario to replace the corresponding manual tasks during "Release" milestone to just call this new lane instead.

So that parent lanes called to initiate release steps are grouped together on top, and child lanes called by those and run by CI for building and uploading are grouped together after those.
For consistency with our other repos
@AliSoftware AliSoftware requested a review from a team as a code owner December 18, 2024 18:44
@AliSoftware AliSoftware requested review from mebarbosa and removed request for a team December 18, 2024 18:44
@dangermattic
Copy link
Collaborator

dangermattic commented Dec 18, 2024

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 18, 2024

📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
App Name 📱 Mobile
Build TypedebugProd
Commit3358c9e
Direct Downloadpocketcasts-app-prototype-build-pr3382-3358c9e.apk
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
App Name 🚗 Automotive
Build TypedebugProd
Commit3358c9e
Direct Downloadpocketcasts-automotive-prototype-build-pr3382-3358c9e.apk
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
App Name ⌚ Wear
Build TypedebugProd
Commit3358c9e
Direct Downloadpocketcasts-wear-prototype-build-pr3382-3358c9e.apk

name: version_number
)

create_backmerge_pr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See @iangmaia 's comment on the original PR about this.

  • In most cases there won't be anything to merge from release/N->main at that stage so creation of such a PR would be expected to be skipped by the create_release_backmerge_pull_request action… but given PCAndroid uses "Squash Merge", the action will detect that some commits from release/N are not present in main and thus still create such a release/N->main backmerge PR anyway.
  • Given that we don't expect any last-minute commits having landed in release/N branch after the finalize_release happened earlier in the scenario, in practice it should be OK 99% of the time to skip the creation of this release/N->main backmerge PR. There's still a risk that a late commit that didn't warrant a resubmission (e.g. last-minute tooling fix landed after finalize_release but not requiring a new run of finalize_release after it landed because it only changed tooling, not the binary being submitted) would be lost though, but that might be an acceptable balance too?
  • It's important to note though that, regardless of all the above, we still want to ensure that there's a release/N->release/N+1 backmerge PR being created to make sure we include all the latest fixes of version N in version N+1 for those late fixes that might have landed after the code freeze of version N+1 (and the creation of release/N+1 branch) already happened.

As a result, we might want to adjust this call to only backmerge to the next release/N+1 branch (if it already exists at that time, which should be the case given the scenario timing)… but not to main?


Ideally we should probably find a way to improve our create_release_backmerge_pull_request action in release-toolkit so it can support the case of repos that are using squash-merges, so that its implementation logic to detect if such a PR is necessary would not be based on "if there are commits in the release/* branch not present in the merge-base of release/* and main", but instead based on the diff being empty or not (worth pondering if we should then do such a comparison on the two-dots vs three-dots diff and consider any potential edge cases though)

In the meantime, since the current Release Scenario never mentioned creating a backmerge PR amongst the tasks of the "Release" milestone, maybe it'd be just as simple to remove that call from publish_release, to keep the current status quo? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should probably find a way to improve our create_release_backmerge_pull_request action in release-toolkit so it can support the case of repos that are using squash-merges

I agree. I vaguely remember one of my first implementations was using the actual diff, but iinm using only the diff had its own problems. Perhaps we could combine the current implementation with that approach as well 🤔

Also to ponder, GitHub itself doesn't seem to use such approach and just creates the PR 🤷

In the meantime, since the current Release Scenario never mentioned creating a backmerge PR amongst the tasks of the "Release" milestone, maybe it'd be just as simple to remove that call from publish_release, to keep the current status quo? 🤔

Yep, I've decided together with the team to follow this approach for WPAndroid / WPiOS for now. Additionally, they also argued that creating a PR from this step can be somewhat unexpected for the release manager, but I guess that could be solved with either a separate step or better communication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, they also argued that creating a PR from this step can be somewhat unexpected for the release manager, but I guess that could be solved with either a separate step or better communication.

I can see how it can be confuising to have a backmerge PR release/N->trunk be created at that stage if no new commit has landed in release/N since the submission, but yet the action creates a PR nonetheless because of the repo using squash-merge and the action thus detecting commits from release/N missing from trunk.

But if we solve this to support the use case of repos using squash-merge (maybe just checking for the diff being non-empty in those cases indeed?), then at that point I think we should still keep the publish_release lane being the one doing the backmerge PRs, including because it's also the lane that ensures that the release/N branch is deleted once the tag has been created by the GitHub Release.

We can't delete the release/* branch until we've made sure we created a git tag (by way of publishing the GitHub release) first, and we can't delete the release/* branch until we ensured that everything that was in that release/* branch has been propagated to other branches (trunk and release/N+1) first (either by a merge-commit or by a squash-merge), otherwise for the very rare cases where we might have added a commit post-finalize_release (e.g. tooling fix or similar) we risk losing that fix forever instead of it being propagated to next versions. So making this backmerge PR be done in a separate Task in the release scenario might not be practically possible as the release/* branch would have already been deleted by the previous task calling publish_release at that point.

But as long as we manage to make the create_release_backmerge_pull_request action properly detect when a PR is not necessary on repos using squash-merge like it does for repos using merge-commits, in 95% of cases there won't be a PR created at publish_release stage anymore so that won't create some unexpected surprise for the release manager, all while ensuring that in the rare cases where it would be necessary to create the PR to ensure not to lose post-submission (tooling?) fixes, it is still done.

Copy link
Contributor

Choose a reason for hiding this comment

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

as long as we manage to make the create_release_backmerge_pull_request action properly detect when a PR is not necessary on repos using squash-merge like it does for repos using merge-commits

Yeah, I think that's the main point: making the action work properly for repos with a squash merge policy.

The PR(s) creation during release publishing (or eventual breakdown into different Rv2 tasks, or a better task description to the release manager, etc) can be a separate discussion.

@AliSoftware AliSoftware added [Type] Tooling Related to the Gradle build scripts and the setup or maintenance of the project build process. [Area] Tooling labels Dec 18, 2024
@AliSoftware AliSoftware self-assigned this Dec 18, 2024
@AliSoftware AliSoftware requested a review from a team December 18, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Area] Tooling [Type] Tooling Related to the Gradle build scripts and the setup or maintenance of the project build process.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants