Rename branch_to_build parameter into branch
#19281
Conversation
This makes the lane parameter using the same name as the `buildkite_trigger_build` action, avoiding confusion in the rare cases we need to call it manually. If you called the `trigger_release_build` or `trigger_beta_build` lane without a parameter, you'll get prompted to pass a `branch` one, by the underlying `buildkite_trigger_build` action. But if you next time called the lane with a `branch` parameter, you'd still get prompted to pass one, because the lane expected `branch_to_build` , but this was not made clear by the messages Fastlane prompted. Rather than adding extra code for this, I thought aligning the parameters name would sort out the issue. This leans a bit into the convention over configuration realm, though. There's nothing stopping future developers to reintroduce the same confusion by renaming the parameter either at the lane or action level. Given the limited userbase of this code and the relative slow pace at which it changes, I think that's an acceptable tradeoff to have convenience for the time being.
| end | ||
|
|
||
| UI.message("Gutenberg version: #{(res.scan(/'([^']*)'/))[0][0]}") | ||
| UI.message("Gutenberg version: #{res.scan(/'([^']*)'/)[0][0]}") |
There was a problem hiding this comment.
This is unrelated to the PR, but a tiny RuboCop violation fix that I though valuable to sneak in.
fastlane/lanes/release.rb
Outdated
| # | ||
| lane :trigger_beta_build do |options| | ||
| trigger_buildkite_release_build(branch: options[:branch_to_build], beta: true) | ||
| trigger_buildkite_release_build(branch: options[:branch], beta: true) |
There was a problem hiding this comment.
tbh in the couple of cases I need to call this lane manually, I'm already often annoyed that I have to provide the branch name explicitly… and I've pondered a while ago if we shouldn't default it to "release/#{ios_get_app_version}" if we don't provide a branch: parameter explicitly from the CLI, to make things even easier to call?
| trigger_buildkite_release_build(branch: options[:branch_to_build], beta: true) | |
| trigger_buildkite_release_build(branch: options[:branch], beta: true) | |
| branch_to_build = options[:branch] || "release/#{ios_get_app_version}" | |
| trigger_buildkite_release_build(branch: branch_to_build, beta: true) |
There was a problem hiding this comment.
Good idea. This default value is particularly appropriate for "beta" and "release" builds as release/... is the expected branch for them.
Also using ios_get_app_version would work most of the time, the only case I can think of resulting in an unexpected build would be if the branch from which we run this is not up-to-date with trunk, or if we run from trunk in the middle of a code freeze. Both are super-edge cases. Besides, an unexpected value in that way would be rejected by ASC, so no harm done 👍
This makes the code a tiny bit clearer.
|
Added some further refinements:
This, IMHO, makes the code nicer to read. That comes at the cost of jumping here and there if you want to know the details of an individual call, but it's worth if each top-level lane can read like a set of instructions. |
…-parameters-names-in-buildkite-actions
Generated by 🚫 dangerJS |
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
mokagio
deleted the
mokagio/align-parameters-names-in-buildkite-actions
branch
This makes the lane parameter using the same name as the
buildkite_trigger_buildaction, avoiding confusion in the rare cases we need to call it manually.If you called the
trigger_release_buildortrigger_beta_buildlane without a parameter, you'll get prompted to pass abranchone, by the underlyingbuildkite_trigger_buildaction.But if you next time called the lane with a
branchparameter, you'd still get prompted to pass one, because the lane expectedbranch_to_build, but this was not made clear by the messages Fastlane prompted.For some reason, Preview doesn't open the screenshot I uploaded above, so I can't highlight the parts that show the
branchinput inconsistencies.Rather than adding extra code for this, I thought aligning the parameters name would sort out the issue.
The approach I took leans a bit into the convention over configuration realm, though. There's nothing stopping future developers to reintroduce the same confusion by renaming the parameter either at the lane or action level. Given the limited userbase of this code and the relative slow pace at which it changes, I think that's an acceptable tradeoff to have convenience for the time being.
Regression Notes
RELEASE-NOTES.txtif necessary. N.A.