Add Buildkite step to optionally trigger FAD build on `trunk` #16432

mokagio · 2025-12-04T03:05:55Z

Description

Similar to woocommerce/woocommerce-android#15027, this PR adds an optional step that will appear on CI builds from trunk to deploy to Firebase App Distribution.

Test Steps

Given the step will appear only on trunk we can only test once this is merged... However, you can see an hacked version here #16431

Screenshots

fad.mov

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. - N.A.

mokagio · 2025-12-04T03:39:24Z

.buildkite/pipeline.yml

+        key: prototype_triggered
+      - label: Prototype Build
+        depends: prototype_triggered
+        command: .buildkite/commands/prototype-build.sh


I considered splitting build from upload as part of this PR, but decided to save it log it as a nice to have for a future date instead.

Doing so would have required updating the existing prototype build step and I didn't want to grow the scope.

mokagio · 2025-12-04T03:43:34Z

.buildkite/pipeline.yml

+        plugins: [$CI_TOOLKIT]
+        notify:
+          - github_commit_status:
+              context: Prototype Build From Trunk


Notice the different name for the check, just in case the same commit already run from a PR (impossible with merge commits and squashes, but one never knows... rebase and merge? cherry pick and force push bypassing checks?) and also to keep things tidy.

I wonder if it actually makes sense to have this notify at all. Given this step will only run on trunk, and not on PRs—while the github_commit_status are mostly (only?) used to report the status of CI steps on GitHub PRs' UI, right?

So maybe we might as well remove that notify attribute altogether on that one?

I think there's marginal value in keeping this. GitHub shows the commit checks outside PRs, too.

Here's from example in the repo's landing page:

If clicked, it shows:

It might be useful at some point to have the check -> CI job link accessible this way.

But... I don't feel strongly about it. Happy to delete if this doesn't sound like a strong enough reason.

Ah good point, completely forgot about that UI in GitHub to show checks on any commit even outside PRs 👍

mokagio · 2025-12-04T03:46:00Z

fastlane/Fastfile

    release_notes = <<~NOTES
-      Pull Request: ##{pull_request_number || 'N/A'}
      Branch: `#{ENV.fetch('BUILDKITE_BRANCH', 'N/A')}`
      Commit: #{ENV.fetch('BUILDKITE_COMMIT', 'N/A')[0...7]}
    NOTES
+    release_notes += "Pull Request: ##{PULL_REQUEST_NUMBER}" unless PULL_REQUEST_NUMBER.nil?


Let me know what you think, but if we have builds from trunk which definitely won't have a PR, it would seem noisy to read "Pull Request: N/A" in the release notes, especially at the top.

mokagio · 2025-12-04T03:47:06Z

fastlane/Fastfile

-  pr_num == 'false' ? nil : Integer(pr_num)
-end
+# Buildkite sets this env var to the PR number if on a PR, but to 'false' (and not nil) if not on a PR
+PULL_REQUEST_NUMBER = ENV['BUILDKITE_PULL_REQUEST']&.then { |n| n == 'false' ? nil : Integer(n) }


A micro-optimization for sure, but it seemed unnecessary to read a constant value from the env a number of times when we could have stored it in a Ruby constant.

I am on two minds about this tbh.

I see your point of not recomputing the value multiple times, but OTOH, making it a constant (in the strict sense of the term, i.e. with all-uppercase syntax and all) feels a bit wrong to me conceptually, because it's not a value that is fixed and never changing across all runs (like e.g. FIREBASE_APP_ID or IOS_LOCALES or SIMULATOR_VERSION are), but instead something that will have a different value depending on the PR. I think that's the main rationale as to why that was exposed as a helper function rather than a Ruby constant in the first place.

My two cents:

Either rename the variable to lowercase (pull_request_number = … )—to not technically make it a Ruby constant. Then move its declaration to the top of the Fastfile.

Or keep it as a def pull_request_number helper, but add memoization to avoid recomputing the value (not worth it imho)

Fair point.

@copilot address this comment by renaming PULL_REQUEST_NUMBER to pull_request_number

No need to read every time

wpmobilebot · 2025-12-04T04:03:34Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`pr16432-66961b3`
	Version	`23.8`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`66961b3`
	Installation URL	656u3kd61spco

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

twstokes · 2025-12-04T14:04:50Z

fastlane/Fastfile

      Branch: `#{ENV.fetch('BUILDKITE_BRANCH', 'N/A')}`
      Commit: #{ENV.fetch('BUILDKITE_COMMIT', 'N/A')[0...7]}
    NOTES
+    release_notes += "Pull Request: ##{PULL_REQUEST_NUMBER}" unless PULL_REQUEST_NUMBER.nil?


Should there be a newline there?

Suggested change

release_notes += "Pull Request: ##{PULL_REQUEST_NUMBER}" unless PULL_REQUEST_NUMBER.nil?

release_notes += "\nPull Request: ##{PULL_REQUEST_NUMBER}" unless PULL_REQUEST_NUMBER.nil?

Tested this quickly in irb, and seems the heredoc syntax already includes a trailing \n:

So no need for an extra one at the start there (but one could argue we might need one at the end, for consistency of always ending the release_notes string with a newline?)

Suggested change

release_notes += "Pull Request: ##{PULL_REQUEST_NUMBER}" unless PULL_REQUEST_NUMBER.nil?

release_notes += "Pull Request: ##{PULL_REQUEST_NUMBER}\n" unless PULL_REQUEST_NUMBER.nil?

but one could argue we might need one at the end, for consistency of always ending the release_notes string with a newline?

It wouldn't hurt, but for what is worth, not having the new line does not affect how Firebase renders the release notes. They look the same with (via old version using one heredoc) or without (via this branch's implementation)

.buildkite/pipeline.yml

AliSoftware · 2025-12-04T21:45:55Z

.buildkite/pipeline.yml

+        plugins: [$CI_TOOLKIT]
+        notify:
+          - github_commit_status:
+              context: Prototype Build From Trunk


I wonder if it actually makes sense to have this notify at all. Given this step will only run on trunk, and not on PRs—while the github_commit_status are mostly (only?) used to report the status of CI steps on GitHub PRs' UI, right?

So maybe we might as well remove that notify attribute altogether on that one?

AliSoftware · 2025-12-04T21:57:27Z

fastlane/Fastfile

-  pr_num == 'false' ? nil : Integer(pr_num)
-end
+# Buildkite sets this env var to the PR number if on a PR, but to 'false' (and not nil) if not on a PR
+PULL_REQUEST_NUMBER = ENV['BUILDKITE_PULL_REQUEST']&.then { |n| n == 'false' ? nil : Integer(n) }


I am on two minds about this tbh.

I see your point of not recomputing the value multiple times, but OTOH, making it a constant (in the strict sense of the term, i.e. with all-uppercase syntax and all) feels a bit wrong to me conceptually, because it's not a value that is fixed and never changing across all runs (like e.g. FIREBASE_APP_ID or IOS_LOCALES or SIMULATOR_VERSION are), but instead something that will have a different value depending on the PR. I think that's the main rationale as to why that was exposed as a helper function rather than a Ruby constant in the first place.

My two cents:

Either rename the variable to lowercase (pull_request_number = … )—to not technically make it a Ruby constant. Then move its declaration to the top of the Fastfile.

Or keep it as a def pull_request_number helper, but add memoization to avoid recomputing the value (not worth it imho)

Co-authored-by: Olivier Halligon <[email protected]>

Copilot · 2025-12-05T00:31:00Z

@mokagio I've opened a new pull request, #16435, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: mokagio <[email protected]>

mokagio · 2025-12-05T01:30:51Z

@AliSoftware @twstokes I should have implemented or commented on all your feedback. Thanks.

mokagio commented Dec 4, 2025

View reviewed changes

mokagio requested review from AliSoftware, iangmaia and twstokes December 4, 2025 03:49

mokagio marked this pull request as ready for review December 4, 2025 03:49

mokagio added this to the 23.9 milestone Dec 4, 2025

mokagio added the category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. label Dec 4, 2025

mokagio added 2 commits December 4, 2025 14:52

Add Buildkite step to optionally trigger FAD build on trunk

9ff68a4

Only add PR number to release notes if one is available

b8470b6

mokagio force-pushed the ainfra-1614-build-wc-ios-prototypes-from-trunk branch from 46780f7 to 52f78e1 Compare December 4, 2025 03:53

Refactor pull_request_number method into a constant

0a28f5e

No need to read every time

mokagio force-pushed the ainfra-1614-build-wc-ios-prototypes-from-trunk branch from 52f78e1 to 0a28f5e Compare December 4, 2025 03:54

mokagio mentioned this pull request Dec 4, 2025

Split building from uploading the Prototype Build #16433

Open

twstokes reviewed Dec 4, 2025

View reviewed changes

AliSoftware reviewed Dec 4, 2025

View reviewed changes

mokagio and others added 2 commits December 5, 2025 11:17

Make input text a question

f5f24eb

Co-authored-by: Olivier Halligon <[email protected]>

Initial plan

153f41f

Copilot AI mentioned this pull request Dec 5, 2025

Rename PULL_REQUEST_NUMBER to pull_request_number #16435

Merged

1 task

Copilot AI and others added 3 commits December 5, 2025 00:32

Rename PULL_REQUEST_NUMBER to pull_request_number

9ce0ff5

Co-authored-by: mokagio <[email protected]>

Move pull_request_number to top of Fastfile

c96ea40

Co-authored-by: mokagio <[email protected]>

Rename PULL_REQUEST_NUMBER to pull_request_number (#16435)

66961b3

mokagio requested review from AliSoftware and twstokes December 5, 2025 01:30

mokagio enabled auto-merge December 5, 2025 01:31

AliSoftware approved these changes Dec 5, 2025

View reviewed changes

mokagio merged commit 2989fb8 into trunk Dec 5, 2025
13 checks passed

mokagio deleted the ainfra-1614-build-wc-ios-prototypes-from-trunk branch December 5, 2025 03:06

	release_notes += "Pull Request: ##{PULL_REQUEST_NUMBER}" unless PULL_REQUEST_NUMBER.nil?
	release_notes += "\nPull Request: ##{PULL_REQUEST_NUMBER}" unless PULL_REQUEST_NUMBER.nil?

Add Buildkite step to optionally trigger FAD build on trunk #16432

Add Buildkite step to optionally trigger FAD build on trunk #16432

Conversation

mokagio commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Steps

Screenshots

Uh oh!

mokagio Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Dec 5, 2025

Uh oh!

mokagio commented Dec 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Add Buildkite step to optionally trigger FAD build on `trunk` #16432

Add Buildkite step to optionally trigger FAD build on `trunk` #16432

mokagio commented Dec 4, 2025 •

edited

Loading

mokagio Dec 4, 2025 •

edited

Loading

mokagio Dec 4, 2025 •

edited

Loading

wpmobilebot commented Dec 4, 2025 •

edited

Loading