Skip to content

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

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

Merged
merged 8 commits into from
Dec 5, 2025
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9ff68a4
Add Buildkite step to optionally trigger FAD build on `trunk`
mokagio Dec 4, 2025
b8470b6
Only add PR number to release notes if one is available
mokagio Dec 4, 2025
0a28f5e
Refactor `pull_request_number` method into a constant
mokagio Dec 4, 2025
f5f24eb
Make `input` text a question
mokagio Dec 5, 2025
153f41f
Initial plan
Copilot Dec 5, 2025
9ce0ff5
Rename PULL_REQUEST_NUMBER to pull_request_number
Copilot Dec 5, 2025
c96ea40
Move pull_request_number to top of Fastfile
Copilot Dec 5, 2025
66961b3
Rename `PULL_REQUEST_NUMBER` to `pull_request_number` (#16435)
mokagio Dec 5, 2025
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
14 changes: 14 additions & 0 deletions .buildkite/pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ steps:
- github_commit_status:
context: Prototype Build

- group: ":rocket: Prototype Build"
if: build.branch == "trunk"
steps:
- input: Deploy Prototype Build
prompt: Share a Prototype Build via Firebase App Distribution?
key: prototype_triggered
- label: Prototype Build
depends_on: prototype_triggered
command: .buildkite/commands/prototype-build.sh
Copy link
Contributor Author

@mokagio mokagio Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

plugins: [$CI_TOOLKIT]
notify:
- github_commit_status:
context: Prototype Build From Trunk
Copy link
Contributor Author

@mokagio mokagio Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@AliSoftware AliSoftware Dec 4, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mokagio mokagio Dec 5, 2025

Choose a reason for hiding this comment

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

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:

image

If clicked, it shows:

image

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.

Copy link
Contributor

@AliSoftware AliSoftware Dec 5, 2025

Choose a reason for hiding this comment

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

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


#################
# Run Unit Tests
#################
Expand Down
15 changes: 6 additions & 9 deletions fastlane/Fastfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,10 +777,10 @@ platform :ios do
build_for_prototype_build

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?
Copy link
Contributor Author

@mokagio mokagio Dec 4, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@twstokes twstokes Dec 4, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@AliSoftware AliSoftware Dec 4, 2025

Choose a reason for hiding this comment

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

Tested this quickly in irb, and seems the heredoc syntax already includes a trailing \n:
Image
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?

Copy link
Contributor Author

@mokagio mokagio Dec 5, 2025

Choose a reason for hiding this comment

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

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)

IMG_5773 IMG_5774


firebase_app_distribution(
app: FIREBASE_APP_ID,
Expand All @@ -796,13 +796,13 @@ platform :ios do
dsym_path: './build/WooCommerce.app.dSYM.zip'
)

next if pull_request_number.nil?
next if PULL_REQUEST_NUMBER.nil?

# PR Comment
comment_body = prototype_build_details_comment(app_display_name: 'WooCommerce iOS Prototype')
comment_on_pr(
project: GITHUB_REPO,
pr_number: pull_request_number,
pr_number: PULL_REQUEST_NUMBER,
reuse_identifier: 'prototype-build-link',
body: comment_body
)
Expand All @@ -812,7 +812,7 @@ platform :ios do
lane :build_for_prototype_build do |fetch_code_signing: true|
update_certs_and_profiles_enterprise if fetch_code_signing

pr_prefix = pull_request_number&.then { |num| "pr#{num}" }
pr_prefix = PULL_REQUEST_NUMBER&.then { |num| "pr#{num}" }
commit_short_hash = ENV.fetch('BUILDKITE_COMMIT', '0')[0...7]
build_number = [pr_prefix, commit_short_hash].compact.join('-')

Expand Down Expand Up @@ -1526,11 +1526,8 @@ TEST_ANALYTICS_ENVIRONMENT = %w[
# Release Management Utils
# -----------------------------------------------------------------------------------

def pull_request_number
# Buildkite sets this env var to the PR number if on a PR, but to 'false' (and not nil) if not on a PR
pr_num = ENV.fetch('BUILDKITE_PULL_REQUEST', 'false')
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) }
Copy link
Contributor Author

@mokagio mokagio Dec 4, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@AliSoftware AliSoftware Dec 4, 2025

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. Or keep it as a def pull_request_number helper, but add memoization to avoid recomputing the value (not worth it imho)

Copy link
Contributor Author

@mokagio mokagio Dec 5, 2025

Choose a reason for hiding this comment

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

Fair point.

@copilot address this comment by renaming PULL_REQUEST_NUMBER to pull_request_number


def create_backmerge_pr
version = release_version_current
Expand Down