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.
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 FL_GIT_BRANCH_DONT_USE_ENV_VARS
env var to git_branch
#21597
add FL_GIT_BRANCH_DONT_USE_ENV_VARS
env var to git_branch
#21597
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have the feeling that this implementation seems off, but I couldn't think of a better/cleaner one 😅 I thought about resetting the branch env var like:
old_git_branch = GIT_BRANCH
GIT_BRANCH = nil
do_stuff_here()
GIT_BRANCH = old_git_branch
You could improve this logic by wrapping this code in a method that takes a block as argument to make it cleaner. fastlane looks up multiple env vars, so that would have to be incorporated into the logic. The env vars it looks up might even change in the future, however (but I think we could simply use Fastlane::Actions::SharedValues::GIT_BRANCH_ENV_VARS
).
Overall I think your implementation is cleaner and solves the problem, even though it's a bit unconventional haha. I'm ok with this 👍
FL_GIT_BRANCH_DONT_USE_ENV_VARS
env var to git_branch
Thank you for the review @rogerluan 🙇♂️ Let me know if there is anything else I need to do before this is merged. |
I'd like for other people to review this PR and see if they have other ideas, otherwise we can go with this one :) (perhaps not @AliSoftware though cuz he's biased on this PR 😄) |
Thanks @rogerluan. As long as we find a way to get this working for us, I am happy with whatever solution you all think is best. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thought on also making this an option in the action and then a parameter on the git_branch
method?
@@ -16,7 +16,7 @@ def self.description | |||
end | |||
|
|||
def self.details | |||
"If no branch could be found, this action will return an empty string. This is a wrapper for the internal action Actions.git_branch" | |||
"If no branch could be found, this action will return an empty string. If `FL_GIT_BRANCH_DONT_USE_ENV_VARS` is `true`, it'll ignore CI ENV vars. This is a wrapper for the internal action Actions.git_branch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of just an environment variable, can this dont_use_env_vars
be added as on option on the git_branch
action?
The action would then pass this as an optional parameter into the git_branch
method?
I'm kind of leaning towards this so that this behavior is more publicly document in the action options documentation 🤔 Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that git_branch is not the only action relying on that Helper.git_branch
.
That's the whole problem and why we thought about the env car as being able to impact all actions relying on that helper.
For example, ensure_git_branch
action is also impacted by that helper's behavior (see PR description for details)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aight, this looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validci/circleci
builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
The current
git_branch
action and its relevant helper uses environment variables to modify the output. This may make things easier to work with in most cases, but there are some cases where it's important that it returns the actual git branch. Consider the scenario where a CI job is triggered frommain
branch, but the job creates arelease/x.y
branch and acts on that - for example, by using the ensure_git_branch action. The current implementation ofgit_branch
in such a case will continue to return themain
branch.It's possible to work around this issue, and that's what we have done for a while by using our own
git_branch
action. However, fastlane actions are interconnected and if we keep going down that route, we'll have to keep re-implementing more actions. We are hoping that this proposal will avoid the issue all together.Description
This PR adds
FL_GIT_BRANCH_DONT_USE_ENV_VARS
environment variable which - whentrue
- will directly return the git branch instead of using the values fromGIT_BRANCH_ENV_VARS
.Adding a new environment variable to discard the usage of environment variables feels a little off. However, within the project structure, this seems like the most consistent and cleanest solution to the problem.
Testing Steps
I've added a new unit test that should cover the new case. While writing this test, I've combined the approach from existing tests to make it consistent.