-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Further Fastlane setup improvements before adding CI release automation #22214
Conversation
As much as I like the informal tone, it was a one-off and therefore inconsistent with the rest of the automation messaging.
The "squiggly heredoc" allows to indent the content of an heredoc multiline string – good for code readability – without said indentation leaking into the printed string. See https://ruby-doc.org/core-2.5.0/doc/syntax/literals_rdoc.html#:~:text=To%20have%20indented%20content%20as%20well%20as%20an%20indented%20closing%20identifier%2C%20you%20can%20use%20a%20%E2%80%9Csquiggly%E2%80%9D%20heredoc%2C%20which%20uses%20a%20%E2%80%9C~%E2%80%9D%20instead%20of%20a%20%E2%80%9C%2D%E2%80%9D%20after%20%3C%3C%3A
See conversation at #22191 (comment)
@@ -24,28 +24,25 @@ | |||
|
|||
release_branch_name = compute_release_branch_name(options:, version: release_version_next) | |||
|
|||
skip_user_confirmation = options[:skip_confirm] |
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.
As discussed here, #22191 (comment), reverted the change that printed this information only in user interactive (skip_confirm:false
) mode.
UI.message('Aborting code freeze as requested.') | ||
UI.message("Terminating as requested. Don't forget to run the remainder of this automation manually.") |
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.
From #22191 (comment)
but this message feels a bit off to me. When we run this lane locally, the code freeze is completed at this point - we just didn't push it to remote yet.
It does make sense in CI, because if the change is not pushed, then it's discarded. However, that's not the case for local. 🤷
Changed the verb to "terminating" which I hope removes the impression that the lane is atomic and therefore aborting would discard changes. Also added the second sentence to reinforce the fact that changes have been made and it's up to the user to complete the process.
I considered listing what the remaining steps were, but then decided against it to avoid having to maintain that list as the lane evolves.
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 think this works OK as a generic message. Thanks for the improvement!
message = <<~MESSAGE | ||
Code Freeze: | ||
• New release branch from #{DEFAULT_BRANCH}: #{release_branch_name} | ||
|
||
unless skip_user_confirmation | ||
# The `release_version_next` is used as the `new internal release version` value because the external and internal | ||
# release versions are always the same. | ||
message = <<-MESSAGE |
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 want to call out in this diff the move from <<-
to <<~
and how it affects the indentation once message
is printed.
cc @spencertransier as (I think) the original author. It's a neat Ruby feature I hope you'll enjoy.
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.
Looks great! I left a couple non-blocking suggestions.
fastlane/Fastfile
Outdated
# That means if we change the branch as part of an action, it'll return the incorrect branch. | ||
# This environment variable disables that behavior. | ||
# See https://github.com/fastlane/fastlane/pull/21597 | ||
ENV['FL_GIT_BRANCH_DONT_USE_ENV_VARS'] = true |
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.
Nitpick but also a question. I thought environment variables can only be strings - or not set at all. I guess Ruby might be setting this as a string, I am not sure - but for those who are less familiar with Ruby (like myself) I think it's worth setting this as a string value instead.
Having said that, I might totally be wrong about environment variables, and if so, I'd like to fix that 😅
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.
UI.message('Aborting code freeze as requested.') | ||
UI.message("Terminating as requested. Don't forget to run the remainder of this automation manually.") |
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 think this works OK as a generic message. Thanks for the improvement!
Co-authored-by: Oguz Kocer <oguzkocer@users.noreply.github.com>
That's just how Ruby works, and I should have known better...
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr22214-42156ab | |
Version | 23.9 | |
Bundle ID | org.wordpress.alpha | |
Commit | 42156ab | |
App Center Build | WPiOS - One-Offs #8127 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr22214-42156ab | |
Version | 23.9 | |
Bundle ID | com.jetpack.alpha | |
Commit | 42156ab | |
App Center Build | jetpack-installable-builds #7147 |
Follows up on @oguzkocer comments from the #22191 review, plus a few further refinements.
Regression Notes
PR submission checklist:
RELEASE-NOTES.txt
if necessary. N.A.UI changes testing checklist: Not a UI PR.