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.

Sign up for GitHub

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

Add create_branch and checkout_and_pull actions #532

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Add create_branch and checkout_and_pull actions #532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a45f2a6
Add checkout and pull wrapper
spencertransier Dec 30, 2023
55cc9c5
Add create branch action
spencertransier Dec 30, 2023
ea3f746
Update CHANGELOG.md
spencertransier Dec 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ _None_
### New Features

- Added optional `build_gradle_path` and `version_properties_path` config items to actions that previously used the `PROJECT_ROOT_FOLDER` environment variable [#519]
- Added a `checkout_and_pull` git action [#532]
- Added a `create_branch` git action [#532]

### Internal Changes

Expand Down
39 changes: 39 additions & 0 deletions lib/fastlane/plugin/wpmreleasetoolkit/actions/common/checkout_and_pull.rb
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙃

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
require 'fastlane/action'
require_relative '../../helper/git_helper'
module Fastlane
module Actions
class CheckoutAndPullAction < Action
def self.run(params)
branch_name = params[:branch_name]

Fastlane::Helper::GitHelper.checkout_and_pull(branch_name)
end

def self.description
'Checkout and pull the specified branch'
end

def self.return_value
'True if it succeeded switching and pulling, false if there was an error during the switch or pull.'
end

def self.available_options
[
FastlaneCore::ConfigItem.new(key: :branch_name,
env_name: 'BRANCH_NAME_TO_CHECKOUT_AND_PULL',
description: 'The name of the branch to checkout and pull',
optional: false,
type: String),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

]
end

def self.is_supported?(platform)
true
end

def self.authors
['Automattic']
end
end
end
end
45 changes: 45 additions & 0 deletions lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_branch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
require 'fastlane/action'
require_relative '../../helper/git_helper'
module Fastlane
module Actions
class CreateBranchAction < Action
def self.run(params)
branch_name = params[:branch_name]
from = params[:from]

Fastlane::Helper::GitHelper.create_branch(branch_name, from: from)
end

def self.description
'Create a new branch named `branch_name`, cutting it from branch/commit/tag `from`'
end

def self.details
'If the branch with that name already exists, it will instead switch to it and pull new commits.'
end
Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

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).


def self.available_options
[
FastlaneCore::ConfigItem.new(key: :branch_name,
env_name: 'BRANCH_NAME_TO_CREATE',
description: 'The full name of the new branch to create, e.g. "release/1.2"',
optional: false,
type: String),
FastlaneCore::ConfigItem.new(key: :from,
env_name: 'BRANCH_OR_COMMIT_TO_CUT_FROM',
description: 'The branch/tag/commit from which to cut the branch from. If `nil`, will cut the new branch from the current commit. Otherwise, will checkout that commit/branch/tag before cutting the branch.',
optional: true,
type: String),
]
end

def self.is_supported?(platform)
true
end

def self.authors
['Automattic']
end
end
end
end