-
Notifications
You must be signed in to change notification settings - Fork 226
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
base: release/7.79
Are you sure you want to change the base?
Conversation
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
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
|
name: version_number | ||
) | ||
|
||
create_backmerge_pr |
There was a problem hiding this comment.
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 thecreate_release_backmerge_pull_request
action… but given PCAndroid uses "Squash Merge", the action will detect that some commits fromrelease/N
are not present inmain
and thus still create such arelease/N->main
backmerge PR anyway. - Given that we don't expect any last-minute commits having landed in
release/N
branch after thefinalize_release
happened earlier in the scenario, in practice it should be OK 99% of the time to skip the creation of thisrelease/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 afterfinalize_release
but not requiring a new run offinalize_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 versionN+1
(and the creation ofrelease/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? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 oftooling/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:(†) Note that in 95% of cases there won't be anything to backmerge—and the creation of that backmerge PR will be automatically skipped.
release/*
branch after submission.create_release_backmerge_pull_request
anyway is more of safety net in case there would have been a new commit onrelease/*
anyway since, to make sure not lose it completely accidentally once we delete therelease/*
branchFastlane
,Gemfile.lock
, …) after the submission to fix issues encountered in the tooling, or update of theCHANGELOG.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 uponrelease/*
branch deletionThis PR also:
GH_REPOSITORY
constant toGITHUB_REPO
, for consistency with our other reposTesting 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 the7.79
publication to test that this lane works as expected if we want. Even if7.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 therelease/7.79
branch at the end of its scenario—which is what thispublish_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 target7.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.