Add create_branch and checkout_and_pull actions

[spencertransier]

What does it do?

Adds two new actions:

• create_branch
• checkout_and_pull

These are wrappers for the GitHelper methods of the same names. There are currently dozens of instances across the A8C apps where those methods are being called directly. As discussed in paaHJt-5v4-p2, we're moving away from calling Release Toolkit helper methods directly and instead work through actions.

I thought about adding unit tests for these actions or at least the underlying GitHelper methods, but the methods are basically running git commands and not much else. I'm open to discussing this, though! I also wasn't sure how we'd be able to test the pull part of the checkout_and_pull action in a test.

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.

[spencertransier]

In https://github.com/wordpress-mobile/release-toolkit/blob/c7329289f6f00a78cdf9f1988e627390efcf3c43/lib/fastlane/plugin/wpmreleasetoolkit/helper/git_helper.rb#L55C7-L57C118, the param can be either a string or hash with this explanation:

# @param [String,Hash] branch Name of the branch to pull.
      #        If you provide a Hash with a single key=>value pair, it will build the branch name as `"#{key}/#{value}"`,
      #        i.e. `checkout_and_pull(release: version)` is equivalent to `checkout_and_pull("release/#{version}")`.

For this action, I just went with string (partially because I didn't think Fastlane action options could be defined with multiple types). The hash scenario seems a bit complex versus just specifying a string? If the hash option is needed, we could add an additional option for the hash type, but I would vote for just passing a string.

[AliSoftware]

The hash scenario seems a bit complex versus just specifying a string?

Agreed. I'm ok with removing the ability to use it with a Hash too.

[AliSoftware]

Given my two comments in this review, I suggest that we instead have:

• git_switch(branch: …) action which is only to checkout an existing branch (user_error! if not exists)
• git_create_branch(name: …, switch_after_create: true) action, which creates a branch if it doesn't exist yet, and optionally switch to it (default: true)
• Use the built-in git_pull action from fastlane in our Fastfiles to do any git pull that we'd need.

[AliSoftware]

The hash scenario seems a bit complex versus just specifying a string?

Agreed. I'm ok with removing the ability to use it with a Hash too.

[AliSoftware]

I'm not sure I'm a fan of having an action doing multiple things. Admittedly it's quite common to do both a checkout and a pull one after the other, but I think our philosophy for such things is instead to make things more atomic.

Fastlane already has a git_pull action built-in. So maybe we should instead just create a git_checkout (or git_switch or switch_to_branch) action, and let the callers (i.e. the apps' Fastfile) be the ones doing git_checkout(branch: …) then git_pull one after the other?

I feel like this separation also make sense because sometimes we want to switch to a branch but we already know there's no need to git_pull it.

For example, in Tumblr:

• During code_freeze, we ensure_git_branch(branch: '^develop$') then git_pull (as opposed to make the lane do the checkout of develop; it was a choice to let the RM do the checkout of the right branch so they know what they were doing and what they were freezing before starting the lane) at the start
• Then we create the release/* branch from develop… but without switching to it yet, because we want to bump the alpha version on develop after that first and git push that
• Then we switch back to the release/* branch to bump the beta version and trigger a new beta. At that point, for this first beta, we don't want to git_pull because:
  • we know we just created the release/* branch so we're guaranteed there's no remote commit to pull, and this would be pointless
  • and in fact if we did a git_pull it would fail and crash the lane, because the release/* branch does not exist on the remote yet. And we don't want to push it to remote until we've made the commit to bump the beta version, otherwise it would trigger an extra CI build for nothing.

Hopefully this shows some scenarios where we'd want to git checkout without pulling (when branch is not present in remote yet), and want pulling without a checkout 🙃

[AliSoftware]

This behavior is confusing to me, especially since running create_branch(branch_name: 'feature/abc', from: 'feature/def') will end up just switching to feature/abc if it already exists… even if that existing branch was not cut from feature/def. So this would be misleading.

Again, I feel like it's the responsibility of the apps' Fastfile to checkout the right branch (e.g. trunk) before creating the new branch from the current commit.

---

What I'd suggest instead would be to have a git_switch action with a branch parameter, which will switch to an existing branch and UI.user_error! if it does not exist. Then have a git_create_branch action, maybe with an optional parameter switch_after_create (defaulting to true) which will return true if the branch was created and false if it already existed.

Then when one needs to cut a branch B from a branch A, they would call git_switch(branch: A) first, then git_create_branch(name: B).

[jkmassel]

I agree with most of what Olivier wrote – WDYT about using a different helper implementation (and deprecating the old one) – in this case, one that's less surprising in terms of what it does when the branch already exists