Skip to content

Adds ensure_current_branch_using_HEAD git helper #520

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

Closed
oguzkocer wants to merge 4 commits into from
Closed

Adds ensure_current_branch_using_HEAD git helper #520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
105b123
Adds ensure_current_branch_using_HEAD git helper
oguzkocer Oct 17, 2023
37e6182
Adds unit tests for ensure_current_branch_using_HEAD git helper
oguzkocer Oct 17, 2023
dd816ab
Deprecate ensure_on_branch! git helper
oguzkocer Oct 17, 2023
0b0669c
Adds changelog for ensure_current_branch_using_HEAD
oguzkocer Oct 17, 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
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ _None_

### New Features

_None_
- Adds `ensure_current_branch_using_HEAD` git helper. [#520]

### Bug Fixes

_None_

### Internal Changes

_None_
- Deprecates `ensure_on_branch!` in favor of `ensure_current_branch_using_HEAD` [#520]

## 9.1.0

Expand Down
28 changes: 28 additions & 0 deletions lib/fastlane/plugin/wpmreleasetoolkit/helper/git_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,31 @@ def self.current_git_branch
Fastlane::Actions.git_branch_name_using_HEAD
end

# Raises an exception and stop the lane execution if the repo is not on a specific branch
# It uses the the `current_git_branch` method which doesn't allow for environment variable modifications.
#
# Modified from fastlane's original `ensure_git_branch` implementation:
#
# https://github.com/fastlane/fastlane/blob/2.213.0/fastlane/lib/fastlane/actions/ensure_git_branch.rb
#
# Note that picking a proper name for this method is a bit hard as the following considerations need
# to be taken into account:
#
# 1. fastlane's original action is called `ensure_git_branch`.
# 2. We have a deprecated helper called `ensure_on_branch`. We could have used this name, but
# the implementation is different, so it'd be a breaking change for all clients.
# 3. This method relies on the `current_git_branch` helper which internally
# uses the `git_branch_name_using_HEAD` fastlane helper.
def self.ensure_current_branch_using_HEAD(branch)
Copy link

@wpmobilebot wpmobilebot Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🚫 Use snake_case for method names.

Copy link
Contributor Author

@oguzkocer oguzkocer Oct 17, 2023

Choose a reason for hiding this comment

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

Since the name of the method is up for discussion, I'll wait until it's resolved to address this lint.

Copy link
Contributor

@AliSoftware AliSoftware Oct 18, 2023

Choose a reason for hiding this comment

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

While I get that this method relies on current_branch_name_using_HEAD and that's probably what the name you picked come from, I think this is a bit over-verbose (and would make it named like this mostly only as a counterpoint to the fastlane one that uses env vars, while we should already in our own implementations in release-toolkit that we don't want to rely on ENV vars, so imho the precision in the naming is likely unnecessary)

As such, I'd suggest just ensure_current_branch! — the ! in the name being a common Ruby convention to indicate that the method will crash on purpose / interrupt the code and exit if the precondition is not met.

Copy link
Contributor

@AliSoftware AliSoftware Oct 18, 2023

Choose a reason for hiding this comment

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

Also, given branch is taken as a regex, I'd make this explicit in the parameter name. Finally, I'd also suggest to use keyword parameters rather than just positional parameters, to enforce the call site having to provide the parameter name and make it even more explicit that this is a RegExp pattern.

Suggested change
def self.ensure_current_branch_using_HEAD(branch)
def self.ensure_current_branch!(matches:)

or

Suggested change
def self.ensure_current_branch_using_HEAD(branch)
def self.ensure_current_branch!(pattern:)

Copy link
Contributor

@AliSoftware AliSoftware Oct 18, 2023

Choose a reason for hiding this comment

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

Actually, I like the ensure_on_branch! method name, so I wonder if we shouldn't instead keep that method, update its implementation, and just fix all the call sites.

After all, (1) this method being a helper, it's expected to only be used internally by the release-toolkit, not externally (as things that are meant to be used externally should be made into actions), so changing its behavior/implementation should only affect the release-toolkit itself, not its clients and (2) the only usages I see of ensure_on_branch! in the release-toolkit are 4 occurrences, which all look like ensure_on_branch!('release'), and which would be easy to change to ensure_on_branch!(pattern: '^release/') to match the new behavior of using current_branch.match?(/#{pattern}/).

That way, no deprecation needed, still having a nice name for that helper, and making it more robust.

Copy link
Contributor Author

@oguzkocer oguzkocer Oct 18, 2023

Choose a reason for hiding this comment

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

Even if that's the expectation - it's very likely that it's used in the clients so I don't think we should do that. This work is something that popped up as something extra and I want to finish it as quickly as I can to focus on my project.

Copy link
Contributor

@AliSoftware AliSoftware Oct 18, 2023

Choose a reason for hiding this comment

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

Then another option would be to make the existing ensure_on_branch! keep the same (legacy) implementation (with include? if the parameter is a String (for backwards compatibility, until we stop the client projects from using the helper directly, which they shouldn't do), and use the new implementation if the parameter is a RegExp.

      def self.ensure_on_branch!(pattern)
        current_branch_name = Action.sh('git', 'symbolic-ref', '-q', 'HEAD')
        is_right_branch = pattern.is_a?(RegExp) ? current_branch_name.match?(pattern) : current_branch_name.include?(pattern)
        UI.user_error!("This command works only on #{branch_name} branch") unless is_right_branch
      end

current_branch = current_git_branch
branch_expr = /#{branch}/
if current_branch =~ branch_expr
UI.success("Git branch matches `#{branch}`, all good! 💪")
else
UI.user_error!("Git is not on a branch matching `#{branch}`. Current branch is `#{current_branch}`! Please ensure the repo is checked out to the correct branch.")
end
end

# Checks if a branch exists locally.
#
# @param [String] branch_name The name of the branch to check for
Expand All @@ -203,13 +228,16 @@ def self.branch_exists?(branch_name)
!Action.sh('git', 'branch', '--list', branch_name).empty?
end

# DEPRECATED!
#
# Ensure that we are on the expected branch, and abort if not.
#
# @param [String] branch_name The name of the branch we expect to be on
#
# @raise [UserError] Raises a user_error! and interrupts the lane if we are not on the expected branch.
#
def self.ensure_on_branch!(branch_name)
UI.important 'Warning: This helper is deprecated, please use Fastlane::Helper::GitHelper.ensure_current_branch_using_HEAD instead!'
current_branch_name = Action.sh('git', 'symbolic-ref', '-q', 'HEAD')
UI.user_error!("This command works only on #{branch_name} branch") unless current_branch_name.include?(branch_name)
end
Expand Down
13 changes: 13 additions & 0 deletions spec/git_helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,19 @@
expect(described_class.has_git_lfs?).to be false
end

it 'can ensure currently on release branch' do
allow(described_class).to receive(:current_git_branch).and_return('release/23.5')
expect(described_class.ensure_current_branch_using_HEAD('^release/')).to be true
end

it 'fails for unexpected git branch' do
current_branch = 'trunk'
branch = '^release/'
expected_error = "Git is not on a branch matching `#{branch}`. Current branch is `#{current_branch}`! Please ensure the repo is checked out to the correct branch."
allow(described_class).to receive(:current_git_branch).and_return('trunk')
expect(described_class.ensure_current_branch_using_HEAD(branch)).to raise_error(FastlaneCore::Interface::FastlaneError, expected_error)
end

context('commit(message:, files:)') do
before(:each) do
allow_fastlane_action_sh
Expand Down