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

Make create_release_backmerge_pull_request delete existing branch before creating it anew #601

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Oct 17, 2024

What does it do?

This updates create_release_backmerge_pull_request to delete the intermediate branch if it already exists before creating it anew.

This is mostly to fix an issue that PocketCasts iOS folks have been running into when running the new_beta_release lane from their machine, while the merge/release-x.y-into-trunk intermediate branch that had been created from the previous beta (or during code_freeze) had not been deleted from their local working copy, thus making the backmerge PR creation fail. (see p1728400101439009/1727442098.188059-slack-C029BMLUGRX&channel=C029BMLUGRX&message_ts=1728400101.439009)

Rationale:

Case when the intermediate branch already exists in the local working copy

When this happens, this is likely a leftover from a previous backmerge PR (from a previous run of code_freeze or new_beta_release created locally, where the remote branch might have been deleted since, but the Release Manager have not deleted the intermediate branch from their working copy locally.
This is what happened to Daniele and Sergio in the last 2 sprints of PocketCasts iOS (see p1728400101439009/1727442098.188059-slack-C029BMLUGRX&channel=C029BMLUGRX&message_ts=1728400101.439009)

As such, it makes sense to delete the local intermediate branch before recreating it anew for those cases.

Note

I am aware that this problem will disappear all by itself once PocketCasts iOS will have migrated to Release-on-CI, and the new_beta_release lane being run on CI instead of on developers' machines. But (1) I'll still need a sprint or two to land release-on-CI in PocketCasts iOS, and probably likewise for PCAndroid (2) It is still good practice to make sure that, even if a project is configured to use Release-on-CI, it is still possible to run the lanes locally on occasion and have them still not break your local setup.

Case when the intermediate branch already exists on the remote

Previously this was an error that made the action fail on purpose, with a custom error mentioning that a backmerge PR already exist and the RM should close it first.

But in practice, that behavior created more issues than it was worth, because when such a case happened, Release Managers are not sure how to proceed. Indeed:

  • The existing PR is typically from a previous beta so it's not like merging the existing backmerge PR would allow you to move on—you'd still had to rebase it or create a new one
  • But it's not like you can re-run new_beta_release either after having merged or closed the first backmerge PR, since you only want to create the backmerge PR not do the whole dance of a new beta
  • And having the RM manually create the backmerge PR themselves isn't ideal either, because in such case they can easily forget to create an intermediate branch, and would thus risk to accidentally merge the BASE (e.g. trunk) into the BASE (e.g. release/x.y) during conflict resolution

So instead I think it makes more sense in those cases to delete the intermediate branch from the remote—which will make GitHub close the associated PR when the PR's head branch gets deleted—and recreate it anew, with a warning( UI.important, yellow in the logs)

  • If the first backmerge PR was not merged in time, it makes sense anyway to close it and create a new one that points to a more recent intermediate branch that would include more commits from release/* anyway
  • Reviewers who might have already started commenting on the previous backmerge PR that hadn't been merged in time would be notified of the PR being closed, so even if there had been some conversation or conflict resolution happening already on that initial backmerge PR, they'd be notified and think about reproducing them on the new backmerge PR.

In any case, even if such a situation would still require developers and RMs to sync with each other, it's still a better outcome IMHO than having the lane or ReleaseV2 task fail and the RM being lost and needing to ask us for support.

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations
  • Add Unit Tests (aka specs/*_spec.rb) if applicable
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.

@AliSoftware AliSoftware requested review from iangmaia and a team October 17, 2024 18:13
@AliSoftware AliSoftware self-assigned this Oct 17, 2024
Comment on lines 15 to +17
### Bug Fixes

_None_
- `create_release_backmerge_pull_request` now deletes existing intermediate branches before creating them anew. [#601]
Copy link
Contributor Author

@AliSoftware AliSoftware Oct 17, 2024

Choose a reason for hiding this comment

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

Note that even if the behavior of the action changed for the case when a backmerge PR (and intermediate branch) already exists (previously errored, now delete the branch + close the PR and create it anew), I still considered this a Bug Fix rather than a Breaking Change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair.

end

Fastlane::Helper::GitHelper.delete_local_branch_if_exists!(intermediate_branch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key addition that should fix the issue that @danielebogo and @SergioEstevao have been having lately in PocketCasts iOS when the new_beta_release lane tried to create the backmerge PR while they still had a local branch from a previous beta / backmerge created from their machine.

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

So instead I think it makes more sense in those cases to delete the intermediate branch from the remote—which will make GitHub close the associated PR when the PR's head branch gets deleted—and recreate it anew, with a warning( UI.important, yellow in the logs)

Sounds good. From a "keeping the process neat" I like the idea of failing if an intermediate branch exists already because ideally we should merge integration PRs (what the intermediate branches are for) ASAP. But in practice there are various situations, such as those you listed and more, where this is impractical. Sorting out the branches at the tooling level seems like better DX, which is what we want.

Comment on lines 15 to +17
### Bug Fixes

_None_
- `create_release_backmerge_pull_request` now deletes existing intermediate branches before creating them anew. [#601]
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair.

spec/create_release_backmerge_pull_request_spec.rb Outdated Show resolved Hide resolved
AliSoftware and others added 2 commits October 18, 2024 09:11
Co-authored-by: Gio Lodi <gio@mokacoding.com>
Co-authored-by: Gio Lodi <gio@mokacoding.com>
@AliSoftware AliSoftware merged commit 444dcd2 into trunk Oct 18, 2024
5 checks passed
@AliSoftware AliSoftware deleted the fix-backmerge-action-branch-deletion branch October 18, 2024 09:37
@iangmaia
Copy link
Contributor

So instead I think it makes more sense in those cases to delete the intermediate branch from the remote—which will make GitHub close the associated PR when the PR's head branch gets deleted—and recreate it anew, with a warning( UI.important, yellow in the logs)

Sounds good. From a "keeping the process neat" I like the idea of failing if an intermediate branch exists already because ideally we should merge integration PRs (what the intermediate branches are for) ASAP. But in practice there are various situations, such as those you listed and more, where this is impractical. Sorting out the branches at the tooling level seems like better DX, which is what we want.

Totally agree 👍

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.

3 participants