Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fastlane setup improvements before release in CI automation #22191

Merged
merged 12 commits into from
Dec 12, 2023
Merged

Fastlane setup improvements before release in CI automation #22191

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fbdc705
Remove unused REPOSITORY_NAME const from Fastfile
mokagio Dec 6, 2023
b35fcbf
Do not print code freeze preamble if user selected skip_confirm
mokagio Dec 7, 2023
07812be
Use `skip_confirm` to also skip confirmation for push in code freeze
mokagio Dec 7, 2023
2b90cbf
DRY release branch name definition in code freeze lane
mokagio Dec 7, 2023
6aedfa3
Exit code freeze lane without error when user aborts
mokagio Dec 7, 2023
ba5aff7
DRY skip confirmation controls in complete code freeze lane
mokagio Dec 7, 2023
f7184fd
Use `set_branch_protection` over deprecated `setbranchprotection`
mokagio Dec 7, 2023
87f061c
Abstract release branch check in `ensure_git_branch_is_release_branch`
mokagio Dec 7, 2023
e176d53
DRY release branch name definition and computation
mokagio Dec 7, 2023
19979c3
DRY skip confirmation controls in new beta lane
mokagio Dec 7, 2023
661c4de
Use `skip_confirm` to drive `allow_retry` in beta `lint_localizations`
mokagio Dec 7, 2023
3cd2521
Use DRYed branch name logic to create branch in code freeze automation
mokagio Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions fastlane/Fastfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ SECRETS_DIR = File.join(Dir.home, '.configure', 'wordpress-ios', 'secrets')
PROJECT_ENV_FILE_PATH = File.join(SECRETS_DIR, 'project.env')
APP_STORE_CONNECT_KEY_PATH = File.join(SECRETS_DIR, 'app_store_connect_fastlane_api_key.json')

# Other defines used across multiple lanes
REPOSITORY_NAME = 'WordPress-iOS'

Comment on lines -29 to -31
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was unused.

WORDPRESS_BUNDLE_IDENTIFIER = 'org.wordpress'
WORDPRESS_EXTENSIONS_BUNDLE_IDENTIFIERS = %w[
WordPressShare
Expand Down Expand Up @@ -281,3 +278,24 @@ before_all do |lane|
end
# rubocop:enable Style/IfUnlessModifier
end

def compute_release_branch_name(options:, version: release_version_current)
branch_option = :branch
branch_name = options[branch_option]

if branch_name.nil?
branch_name = release_branch_name(version:)
UI.message("No branch given via option '#{branch_option}'. Defaulting to #{branch_name}.")
end

branch_name
end
Comment on lines +282 to +292
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this in Fastfile because it will be shared between lanes across files.

I don't remember why we need this computation logic over using the simpler interpolation from release_branch_name(version:).

I decided to roll with it instead of investigating to make faster progress with the release automation.


def release_branch_name(version: release_version_current)
"release/#{version}"
end

def ensure_git_branch_is_release_branch
# Verify that the current branch is a release branch. Notice that `ensure_git_branch` expects a RegEx parameter
ensure_git_branch(branch: '^release/')
Copy link
Contributor

Choose a reason for hiding this comment

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

ensure_git_branch depends on environment variables, which means when we run the release automation in CI, the branch name it'll check is the one passed from Buildkite instead of the actual branch we are on. To avoid this, we need to add FL_GIT_BRANCH_DONT_USE_ENV_VARS env var as we do in WPAndroid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I didn't run into this issue during my tests, #22194, but maybe it was due to some additional test setup...

However, this might explain something that surprised me and that I filed for investigation. In the screenshot below from this build the git_branch output is the branch the lane started on, not the new one.

image

Addressed in d9691c6

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeap, that's the exact issue that environment variable addresses. If you'd like to learn more about it, here is the PR that we submitted to fastlane.

end
Comment on lines +298 to +301
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this because I think the method name clearly explains what the method does, and we have moved the awkwardness with the explanation comment and RegExp input in a single, out of sight, place.

133 changes: 60 additions & 73 deletions fastlane/lanes/release.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,32 @@
# Make sure that Gutenberg is configured as expected for a successful code freeze
gutenberg_dep_check

# 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
Code Freeze:
• New release branch from #{DEFAULT_BRANCH}: release/#{release_version_next}
release_branch_name = compute_release_branch_name(options:, version: release_version_next)

• Current release version and build code: #{release_version_current} (#{build_code_current}).
• New release version and build code: #{release_version_next} (#{build_code_code_freeze}).
skip_user_confirmation = options[:skip_confirm]

• Current internal release version and build code: #{release_version_current_internal} (#{build_code_current_internal})
• New internal release version and build code: #{release_version_next} (#{build_code_code_freeze_internal})
MESSAGE
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
Code Freeze:
• New release branch from #{DEFAULT_BRANCH}: #{release_branch_name}

UI.important(message)
UI.user_error!('Aborted by user request') unless options[:skip_confirm] || UI.confirm('Do you want to continue?')
• Current release version and build code: #{release_version_current} (#{build_code_current}).
• New release version and build code: #{release_version_next} (#{build_code_code_freeze}).

• Current internal release version and build code: #{release_version_current_internal} (#{build_code_current_internal})
• New internal release version and build code: #{release_version_next} (#{build_code_code_freeze_internal})
MESSAGE

UI.important(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about always printing the message outside of the skip_user_confirmation check to make the information available in CI logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My rationale for only printing in interactive mode was that there's no point in showing that message if the user cannot act on it.

But.. your point of making information available in the CI logs is quite valid. Who knows, there might be a bug at some point in how the codes are evaluated, like it happened in #22087, at which point having logs might be a huge help in diagnosing.

Addressed in 2f8c867

UI.user_error!('Aborted by user request') unless UI.confirm('Do you want to continue?')
end

# Create the release branch
release_branch_name = "release/#{release_version_next}"
mokagio marked this conversation as resolved.
Show resolved Hide resolved
UI.message 'Creating release branch...'
Fastlane::Helper::GitHelper.create_branch("release/#{release_version_next}", from: DEFAULT_BRANCH)
Fastlane::Helper::GitHelper.create_branch(release_branch_name, from: DEFAULT_BRANCH)
UI.success "Done! New release branch is: #{git_branch}"

# Bump the release version and build code and write it to the `xcconfig` file
Expand Down Expand Up @@ -93,17 +100,14 @@
release_notes_file_path: release_notes_source_path
)

if prompt_for_confirmation(
message: 'Ready to push changes to remote to let the automation configure it on GitHub?',
bypass: ENV.fetch('RELEASE_TOOLKIT_SKIP_PUSH_CONFIRM', false)
)
push_to_git_remote(tags: false)
else
UI.message('Aborting code completion. See you later.')
unless skip_user_confirmation || UI.confirm('Ready to push changes to remote to let the automation configure it on GitHub?')
UI.message('Aborting code freeze as requested.')
next
end

setbranchprotection(repository: GITHUB_REPO, branch: "release/#{new_version}")
push_to_git_remote(tags: false)

set_branch_protection(repository: GITHUB_REPO, branch: release_branch_name)
setfrozentag(repository: GITHUB_REPO, milestone: new_version)

ios_check_beta_deps(podfile: File.join(PROJECT_ROOT_FOLDER, 'Podfile'))
Expand All @@ -119,27 +123,26 @@
#
desc 'Completes the final steps for the code freeze'
lane :complete_code_freeze do |options|
# Verify that the current branch is a release branch. Notice that `ensure_git_branch` expects a RegEx parameter
ensure_git_branch(branch: '^release/')
ensure_git_branch_is_release_branch

# Verify that there's nothing in progress in the working copy
ensure_git_status_clean

UI.important("Completing code freeze for: #{release_version_current}")
UI.user_error!('Aborted by user request') unless options[:skip_confirm] || UI.confirm('Do you want to continue?')

skip_user_confirmation = options[:skip_confirm]

UI.user_error!('Aborted by user request') unless skip_user_confirmation || UI.confirm('Do you want to continue?')

generate_strings_file_for_glotpress

if prompt_for_confirmation(
message: 'Ready to push changes to remote and trigger the beta build?',
bypass: ENV.fetch('RELEASE_TOOLKIT_SKIP_PUSH_CONFIRM', false)
)
push_to_git_remote(tags: false)
trigger_beta_build
else
unless skip_user_confirmation || UI.confirm('Ready to push changes to remote and trigger the beta build?')
UI.message('Aborting code freeze completion. See you later.')
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is not in the changeset, 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. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. I reworded it with something that is hopefully clearer in 919d62c. Open to suggestions in the upcoming PR.

next
end

push_to_git_remote(tags: false)
trigger_beta_build
end

# Creates a new beta by bumping the app version appropriately then triggering a beta build on CI
Expand All @@ -148,42 +151,44 @@
#
desc 'Trigger a new beta build on CI'
lane :new_beta_release do |options|
ensure_git_branch_is_release_branch

# Verify that there's nothing in progress in the working copy
ensure_git_status_clean

# Verify that the current branch is a release branch. Notice that `ensure_git_branch` expects a RegEx parameter
ensure_git_branch(branch: '^release/')

git_pull

# Check versions
message = <<-MESSAGE
• Current build code: #{build_code_current}
• New build code: #{build_code_next}
skip_user_confirmation = options[:skip_confirm]

• Current internal build code: #{build_code_current_internal}
• New internal build code: #{build_code_next_internal}
MESSAGE
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
• Current build code: #{build_code_current}
• New build code: #{build_code_next}

UI.important(message)
UI.user_error!('Aborted by user request') unless options[:skip_confirm] || UI.confirm('Do you want to continue?')
• Current internal build code: #{build_code_current_internal}
• New internal build code: #{build_code_next_internal}
MESSAGE

UI.important(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to a previous comment, how do you feel about always printing the message outside of the skip_user_confirmation check to make the information available in CI logs?

UI.user_error!('Aborted by user request') unless UI.confirm('Do you want to continue?')
end

generate_strings_file_for_glotpress
download_localized_strings_and_metadata(options)
lint_localizations
lint_localizations(allow_retry: skip_user_confirmation == false)

bump_build_codes

if prompt_for_confirmation(
message: 'Ready to push changes to remote and trigger the beta build?',
bypass: ENV.fetch('RELEASE_TOOLKIT_SKIP_PUSH_CONFIRM', false)
)
push_to_git_remote(tags: false)
trigger_beta_build
else
UI.message('Aborting beta deployment. See you later.')
unless skip_user_confirmation || UI.confirm('Ready to push changes to remote and trigger the beta build?')
UI.message('Aborting beta deployment.')
next
end

push_to_git_remote(tags: false)

trigger_beta_build
end

# Sets the stage to start working on a hotfix
Expand Down Expand Up @@ -229,7 +234,7 @@

# Create the hotfix branch
UI.message 'Creating hotfix branch...'
Fastlane::Helper::GitHelper.create_branch("release/#{new_version}", from: previous_version)
Fastlane::Helper::GitHelper.create_branch(compute_release_branch_name(options:, version: new_version), from: previous_version)
UI.success "Done! New hotfix branch is: #{git_branch}"

# Bump the hotfix version and build code and write it to the `xcconfig` file
Expand Down Expand Up @@ -257,8 +262,7 @@
#
desc 'Performs the final checks and triggers a release build for the hotfix in the current branch'
lane :finalize_hotfix_release do |options|
# Verify that the current branch is a release branch. Notice that `ensure_git_branch` expects a RegEx parameter
ensure_git_branch(branch: '^release/')
ensure_git_branch_is_release_branch

# Verify that there's nothing in progress in the working copy
ensure_git_status_clean
Expand All @@ -285,8 +289,7 @@
lane :finalize_release do |options|
UI.user_error!('To finalize a hotfix, please use the finalize_hotfix_release lane instead') if ios_current_branch_is_hotfix

# Verify that the current branch is a release branch. Notice that `ensure_git_branch` expects a RegEx parameter
ensure_git_branch(branch: '^release/')
ensure_git_branch_is_release_branch

# Verify that there's nothing in progress in the working copy
ensure_git_status_clean
Expand Down Expand Up @@ -430,22 +433,6 @@ def prompt_for_confirmation(message:, bypass:)
UI.confirm(message)
end

def compute_release_branch_name(options:)
branch_option = :branch
branch_name = options[branch_option]

if branch_name.nil?
branch_name = release_branch_name
UI.message("No branch given via option '#{branch_option}'. Defaulting to #{branch_name}.")
end

branch_name
end

def release_branch_name
"release/#{release_version_current}"
end

def bump_build_codes
bump_production_build_code
bump_internal_build_code
Expand Down