Skip to content

Use git_branch_name_using_HEAD instead of git_branch#463

Merged
oguzkocer merged 5 commits intotrunkfrom
add/current-git-branch
May 5, 2023
Merged

Use git_branch_name_using_HEAD instead of git_branch#463
oguzkocer merged 5 commits intotrunkfrom
add/current-git-branch

Conversation

@oguzkocer
Copy link
Contributor

@oguzkocer oguzkocer commented Apr 21, 2023

What does it do?

We have been using git_branch helper action to check if we are on a release branch during complete code freeze & finalize release steps. This has been working as expected in our local environments. However, the return value of this helper can be modified by environment variables such as GIT_BRANCH or BUILDKITE_BRANCH. That means, if we switch the git branch during a CI step, it'll not return the value we are interested in.

There is a Fastlane::Actions.git_branch_name_using_HEAD helper method we can use, which is exactly what we expect. git_branch method actually wraps its implementation around this helper and adds the extra capability of modifying the return value with environment variables.

We have 2 different approaches to address this issue.

  1. As implemented in 918027b, we can directly use Fastlane::Actions.git_branch_name_using_HEAD everywhere we need. The main issue with that is, we need to add a disclaimer about why we are using this method instead of git_branch.
  2. As implemented in 7669604, we can add a method to git_helper. I like this better because we can have a longer explanation and contain it in a single place. I think this also makes it more likely that we'll use it instead of git_branch when we need to use it somewhere new - although neither option can guarantee that.

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 approprioate existing ### subsection of the existing ## Trunk section.

@oguzkocer oguzkocer changed the title Use git_branch_name_using_HEAD instead of git_branch Use git_branch_name_using_HEAD instead of git_branch Apr 21, 2023
@oguzkocer oguzkocer marked this pull request as ready for review April 21, 2023 20:51
@oguzkocer oguzkocer requested a review from a team April 21, 2023 20:51
@oguzkocer oguzkocer enabled auto-merge April 21, 2023 20:52
Copy link
Contributor

@spencertransier spencertransier left a comment

Choose a reason for hiding this comment

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

I like this better because we can have a longer explanation and contain it in a single place. I think this also makes it more likely that we'll use it instead of git_branch when we need to use it somewhere new - although neither option can guarantee that.

I also like the 7669604 better. It feels cleaner and more self-explanatory, removing the need for the long comment each time the Fastlane helper is called directly.

(Not approving since auto-merge is enabled and I figured I'd let folks with better understanding weigh in here if needed)

@spencertransier spencertransier requested a review from a team April 27, 2023 03:23
@AliSoftware
Copy link
Contributor

I also like the 7669604 better. It feels cleaner and more self-explanatory, removing the need for the long comment each time the Fastlane helper is called directly.

Agreed.

require_relative '../../helper/git_helper'

UI.user_error!('This is not a release branch. Abort.') unless other_action.git_branch.start_with?('release/')
current_branch = Fastlane::Actions.git_branch_name_using_HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Should current_git_branch be used here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Yes it should be, thank you for catching that!

Addressed in f256ae5.

@oguzkocer oguzkocer merged commit 869614f into trunk May 5, 2023
@oguzkocer oguzkocer deleted the add/current-git-branch branch May 5, 2023 03:00
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.

4 participants