Add api_url parameter to create_release_backmerge_pull_request

[iangmaia]

What does it do?

Currently, the action create_release_backmerge_pull_request does not take an api_url to forward to Fastlane's create_pull_request action. api_url is required when dealing with GitHub Enterprise.

This PR adds the api_url parameter.

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.

[AliSoftware]

I wonder if it'd make more sense to reference the ConfigItem from the original create_pull_request action, filtering on the parameters we want to just forward, instead of duplicating them (and tiehr optional. description, type,…) ourselves?

i.e.

FORWARDED_PARAM_KEYS = %i[api_token api_bearer repo labels api_url assignees reviewers team_reviewers].freeze

def self.available_options
  forwarded_params = Fastlane::Actions::CreatePullRequestAction.available_options.select do |opt|
    FORWARDED_PARAM_KEYS.include?(opt.key)
  end
  [
    *forwarded_params,
    # Then here only add the params that are specific to our `create_release_backmerge_pull_request` action
    # that are not directly forwarded to `create_pull_request`
    FastlaneCore::ConfigItem.new(key: :source_branch,
       …

And then when we call the create_pull_request in the implementation of our action, we can inject the forwarded params by constructing a hash of fwd_params = FORWARDED_PARAMS_KEYS.to_h { |k| [k, params[k] } then inject it in create_pull_request(title: …, body: …, **fwd_params)

wdyt?

[iangmaia]

I liked the idea and implemented it... but then the tests started to fail mysteriously due to an additional call to git rev-parse --abbrev-ref HEAD, and some default values also failed (i.e. nil vs. []) 😓 so this is something to consider when implementing this pattern. Eventually I got it working, though! Updated on 21adece.

[AliSoftware]

💡 While we're fixing this action, it'd be nice if, when we push the intermediate branch on line L123/R126 (other_action.push_to_git_remote(tags: false)), we could also ask to set the tracking branch (-u) during push.

This would especially be useful not only to note if new commits have been pushed on remote but not on local etc… but also so that the local working copy can detect when the remote branch has been deleted (i.e. the PR has been merged) when the git remote -vv marks it as [gone], and so that git branch -d can see that the branch was merged and can thus be safely deleted locally too.

[iangmaia]

💡 While we're fixing this action, it'd be nice if, when we push the intermediate branch on line L123/R126 (other_action.push_to_git_remote(tags: false)), we could also ask to set the tracking branch (-u) during push.

This would especially be useful not only to note if new commits have been pushed on remote but not on local etc… but also so that the local working copy can detect when the remote branch has been deleted (i.e. the PR has been merged) when the git remote -vv marks it as [gone], and so that git branch -d can see that the branch was merged and can thus be safely deleted locally too.

Good idea 👍 Updated on 4795969.

[AliSoftware]

@iangmaia Another fix we need to do (and test, probably using some live example not just specs) is wrapping the call to the intermediate_branch_created_callback.call callback within a Dir.chdir(FastlaneCore::FastlaneFolder.path) do … end (at least I think that'd be the fix?) to workaround the fact that if we try to call another action from this proc defined in the caller's Fastfile, it wouldn't try to go up a directory and fail running the action. See Tumblr/ios#29516 & Tumblr/android#24360.

Another solution is for the caller to use other_action.git_commit instead of git_commit when declaring their proc, but that seems less natural to expect to realize that the proc would be called from within the implementation of create_release_backmerge_pull_request and thus from another action and that the caller would have to use other_action even if the code of the proc is within their Fastfile…

[iangmaia]

@iangmaia Another fix we need to do (and test, probably using some live example not just specs) is wrapping the call to the intermediate_branch_created_callback.call callback within a Dir.chdir(FastlaneCore::FastlaneFolder.path) do … end (at least I think that'd be the fix?) to workaround the fact that if we try to call another action from this proc defined in the caller's Fastfile, it wouldn't try to go up a directory and fail running the action. See Tumblr/ios#29516 & Tumblr/android#24360.

Another solution is for the caller to use other_action.git_commit instead of git_commit when declaring their proc, but that seems less natural to expect to realize that the proc would be called from within the implementation of create_release_backmerge_pull_request and thus from another action and that the caller would have to use other_action even if the code of the proc is within their Fastfile…

Isn't FastlaneCore::FastlaneFolder.path going to be something like ./fastlane/ (based on this)? 🤔

[AliSoftware]

Isn't FastlaneCore::FastlaneFolder.path going to be something like ./fastlane/ (based on this)? 🤔

Yes, which is the path in which fastlane is expecting your pwd to be when you run code written from the Fastfile, while when you run code written from an action it expects to run from the root of your repo (this is for legacy reasons mostly)

See this section in the doc which explains it all

To my rough understanding, this is also why other_action is passing FastlaneCore::FastlaneFolder.path, to workaround this implementation detail.

• Indeed, when you call an action from the Fastile, fastlane expects the pwd to be inside ./fastlane at the the you call the code… but also due to those legacy reasons, the inner implementation of fastlane actions are implemented in a way that assumes their code runs from the root of the repo (due to legacy very early implementations from before the fastlane/ folder was introduced), and that's why the runner does a cd .. at the time it hands over the baton from the Fastfile code to the action code, to reconcile those discrepancies…
• This is also why, when you want to call one action from another action's implementation, like we do here for example, you use other_action so that it behaves as is that action was called from the fastfile/ folder (as when it's called directly from the Fastfile) and not from the root folder (where the pwd would be during the execution of the parent action calling that child action)

Is it convoluted implementation and logic? Yes, definitievly… but that's what you get when there's lots of legacy and you don't want to break millions of codebases by changing that suddenly 😅

[iangmaia]

• Indeed, when you call an action from the Fastile, fastlane expects the pwd to be inside ./fastlane at the the you call the code… but also due to those legacy reasons, the inner implementation of fastlane actions are implemented in a way that assumes their code runs from the root of the repo (due to legacy very early implementations from before the fastlane/ folder was introduced), and that's why the runner does a cd .. at the time it hands over the baton from the Fastfile code to the action code, to reconcile those discrepancies…
• This is also why, when you want to call one action from another action's implementation, like we do here for example, you use other_action so that it behaves as is that action was called from the fastfile/ folder (as when it's called directly from the Fastfile) and not from the root folder (where the pwd would be during the execution of the parent action calling that child action)

Ahh, TIL 😅 Thanks a lot for the details, it all makes sense now 👍 this is the kind of implementation detail I never got into before with Fastlane (or only end up seeing add-hoc when this kind of issue arises).

that's what you get when there's lots of legacy and you don't want to break millions of codebases by changing that suddenly 😅

💯

---

I've just pushed daff906.

[AliSoftware]

Tested via https://github.tumblr.net/Tumblr/ios/pull/29539/files, after:

• Creating a dummy release/100.0 branch
• Making its Gemfile point to this branch of the release-toolkit,
• removing the ENV[…] workaround we had for the API URL, and adding theapi_url: parameter to the action call instead
• Tweaking the code called in the intermediate_branch_created_callback block to force a dummy change (so we have something to commit) then calling the git_commit action from it
• Finally, run bundle exec fastlane backmerge_release_branch

And I was able to confirm that:

• The api_url parameter was properly forwarded and allowed to create the PR on our private GH:E instance
• The merge/release-100.0-into-develop intermediate branch was set up to track the remote branch after it being pushed to the remote
• The call to git_commit action worked, without erroring with "not a git repo" as it was before.

🎉

---

⚠️ The only think remaining before merging is to update the CHANGELOG.md with the extra stuff that this PR fixes (in addition to the item you initially created the PR for).

[AliSoftware]

You should also add an entry under ### Bug Fixes to mention that the calling another fastlane action from the intermediate_branch_created_callback proc now behaves correctly.

[AliSoftware]

Could be worth mentioning (probably in the same list item) that the intermediate branch that action creates is also now tracking the remote branch?