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.

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

Split building for ASC from uploading to TestFlight and run as separate CI steps #1635

Merged
merged 18 commits into from
Aug 8, 2024

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Aug 8, 2024

Fix

This will:

  • Ensure better retry-ability. If the TestFlight upload fails because of HTTP errors, we can retry that without rebuilding the whole ipa
  • Better CI resource management. The TestFlight upload does not need macOS, so it could run on a cheaper Linux box. This is not ready yet, but at some point we'll an agent optimized for running this kind of tasks.

Test

I tested this by triggering a release build on this branch:

image

image

image

Review

Only one developer or infrastructure engineer required to review these changes, but anyone can perform the review.

Release

These changes do not require release notes.

@dangermattic
Copy link
Collaborator

dangermattic commented Aug 8, 2024

1 Message
📖 This PR has the Releases label: some checks will be skipped.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 8, 2024

Simplenote Prototype Build📲 You can test the changes from this Pull Request in Simplenote Prototype Build by scanning the QR code below to install the corresponding build.
App NameSimplenote Prototype Build Simplenote Prototype Build
Build Numberpr1635-361cb27-019133cb-edb0-4b9f-bc39-01cd6eca409b
Version4.52
Bundle IDcom.codality.NotationalFlow.Alpha
Commit361cb27
App Center BuildSimplenote - Installable Builds #327
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Comment on lines +12 to +13
echo "--- :closed_lock_with_key: Installing Secrets"
bundle exec fastlane run configure_apply
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will become unnecessary for ASC uploads once we'll move to using env vars.


version = beta_release ? ios_get_build_version : ios_get_app_version(public_version_xcconfig_file: VERSION_FILE_PATH)
version = beta_release ? build_code_current : release_version_current
create_release(
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 can see a further iteration where uploads to TestFlight and Sentry run in parallel, and we only create the release on GitHub when the TestFlight step has succeeded.

@mokagio mokagio force-pushed the mokagio/split-build-and-tf-upload branch from 03e9135 to daf3bc9 Compare August 8, 2024 04:33
@mokagio mokagio force-pushed the mokagio/split-build-and-tf-upload branch from daf3bc9 to 3f504a0 Compare August 8, 2024 05:57
Comment on lines -43 to -54

lane :build_for_app_store_connect do |fetch_code_signing: true|
appstore_code_signing if fetch_code_signing

gym(
scheme: 'Simplenote',
workspace: WORKSPACE,
configuration: 'Distribution AppStore',
clean: true,
export_method: 'app-store'
)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks deleted but you'll find a few lines above with some additional parameters.

Comment on lines 17 to 23
build_for_app_store_connect

upload_to_app_store_connect(
beta_release: beta_release,
skip_prechecks: skip_prechecks,
create_release: create_release
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lane build_and_upload_to_app_store_connect is now unused... I'd say we're better off deleting it. It sounds convenient for local usages, but we shouldn't do local builds unless in the most rare of occasions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, let's delete it.

In the rare cases where we might need to do an App Store build from our local machines to debug stuff or unblock an automation bug, we can always call fastlane build_for_app_store_connect then fastlane upload_to_app_store_connect separately manually anway.

Comment on lines +11 to +15
PROJECT_ROOT_FOLDER = File.dirname(File.expand_path(__dir__))
VERSION_FILE_PATH = File.join(PROJECT_ROOT_FOLDER, 'config', 'Version.Public.xcconfig')
OUTPUT_DIRECTORY_PATH = File.join(PROJECT_ROOT_FOLDER, 'build', 'results')
APP_STORE_BUNDLE_IDENTIFIER = 'com.codality.NotationalFlow'
DEFAULT_BRANCH = 'trunk'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these consts at the top were previously later on in the file I grouped them for clarity and because of the new imports that require them to be already defined.

@mokagio mokagio changed the base branch from trunk to release/4.52 August 8, 2024 09:55
@mokagio mokagio marked this pull request as ready for review August 8, 2024 10:35
@mokagio mokagio requested a review from a team August 8, 2024 10:36
@mokagio mokagio added this to the 4.52 ❄️ milestone Aug 8, 2024
@mokagio mokagio added Releases tooling Related to anything that supports the building & maintaining of the project. labels Aug 8, 2024
This was prompted by the QR code being broken in recent PRs.
# @called_by CI — especially, relies on `BUILDKITE_PULL_REQUEST` being defined
#
def post_installable_build_pr_comment(app_name:, build_number:, url_slug:)
UI.message("Successfully built and uploaded installable build `#{build_number}` to App Center.")
def post_installable_build_pr_comment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These installable builds changes are obviously unrelated to the TestFlight CI flow, but I noticed that the QR code was broken so I chucked them in here.

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

LGTM 👍

fastlane/Fastfile Outdated Show resolved Hide resolved
fastlane/lanes/build.rb Outdated Show resolved Hide resolved
Comment on lines 17 to 23
build_for_app_store_connect

upload_to_app_store_connect(
beta_release: beta_release,
skip_prechecks: skip_prechecks,
create_release: create_release
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, let's delete it.

In the rare cases where we might need to do an App Store build from our local machines to debug stuff or unblock an automation bug, we can always call fastlane build_for_app_store_connect then fastlane upload_to_app_store_connect separately manually anway.

@mokagio mokagio enabled auto-merge August 8, 2024 21:03
@mokagio mokagio merged commit 69ba2ae into release/4.52 Aug 8, 2024
7 of 10 checks passed
@mokagio mokagio deleted the mokagio/split-build-and-tf-upload branch August 8, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Releases tooling Related to anything that supports the building & maintaining of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants