Adds ensure_current_branch_using_HEAD git helper

[oguzkocer]

What does it do?

As part of the versioning changes @spencertransier worked on, we have started using ensure_git_branch fastlane action. Unfortunately this action relies on git_branch action which uses environment variables to modify the result. This is not what we want in Buildkite because it'll ensure which branch the CI build is started from and ignore any branch switches.

We had to introduce our own helper git_current_branch in #463 to replace git_branch. Similarly, this PR adds ensure_current_branch_using_HEAD that we should use instead of ensure_git_branch so the release actions work as expected in Buildkite.

• I couldn't find a good name for this helper and I'd love some suggestions. I've explained some naming considerations in the code comments which I think will be useful regardless of which name we pick.
• Even though this is a modification of the official fastlane method, I thought having some unit tests would be helpful. Unfortunately I couldn't get the error case to work correctly - I am possibly missing something about the syntax - and I'd appreciate some help with that.
• I've also deprecated the ensure_on_branch! helper method. Since this is not an action, I am not entirely sure how it should be deprecated. I added a comment and started printing a warning, but please let me know if there is an official way to do this.

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.

[wpmobilebot]

🚫	Use snake_case for method names.

[oguzkocer]

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

[AliSoftware]

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.

[AliSoftware]

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

[AliSoftware]

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.

[oguzkocer]

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.

[AliSoftware]

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

[AliSoftware]

If you plan to use this as part of your Fastfile to check expected branches, you're gonna have to also create a Fastlane::Action to wrap the call to that helper; because by design the Helpers are only supposed to be used internally by the release-toolkit, not as a public API by the clients and your Fastfile, which instead should only call actions.

NB: Currently we made an exception for this with the Fastlane::Wpmreleasetoolkit::Versioning classes which are used directly in our Fastfiles instead of being exposed as actions. This is a (potentially temporary) exception that we are assuming, and shouldn't be the norm. If any other Helper class is currently being used directly in any of our Fastfile, this should be considered a mistake and should be addressed to replace it with actions.

[AliSoftware]

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

[AliSoftware]

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.

[oguzkocer]

If you plan to use this as part of your Fastfile to check expected branches, you're gonna have to also create a Fastlane::Action to wrap the call to that helper; because by design the Helpers are only supposed to be used internally by the release-toolkit, not as a public API by the clients and your Fastfile, which instead should only call actions.

TIL. With this in mind, I think it's best to drop the helper all together and introduce a fastlane action basically copy pasting the original one from fastlane and using the current_git_branch helper to avoid the environment variable issue. That approach will also address some of the other feedback.

Since that's going to be almost a full rewrite, I am going to close this PR and open a new PR for it.