Create Installable Builds with Buildkite by jkmassel · Pull Request #17581 · wordpress-mobile/WordPress-iOS

jkmassel · 2021-11-27T04:15:10Z

Moves installable builds from CircleCI to Buildkite.

Uses the release toolkit to post the installable build instructions instead of writing out a file for Peril – apart from that, it works the same way.

To test:

Note that all the tasks on this PR pass.
Download the IPA files from the links.
Verify that the builds show up in App Center.
Extra ~~paranoia~~ credit: try installing the App Center builds to ensure they work.

Regression Notes

Potential unintended areas of impact
Could theoretically break installable builds.
What I did to test those areas of impact (or what existing automated tests I relied on)
Checked that they were actually posted to App Center.
What automated tests I added (or what prevented me from doing so)
n/a

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2021-11-27T04:35:25Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

jkmassel · 2021-11-28T04:34:04Z

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr17581-d8ea670. IPA is available here. If you need access to this, you can ask a maintainer to add you.

jkmassel · 2021-11-28T18:47:39Z

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr17581-d8ea670. IPA is available here. If you need access to this, you can ask a maintainer to add you.

fastlane/Jetpack-Fastfile

AliSoftware · 2021-11-30T20:51:05Z

.buildkite/commands/installable-build-jetpack.sh

+bundle exec fastlane run configure_apply
+
+echo "--- :hammer_and_wrench: Building"
+bundle exec fastlane build_and_upload_jetpack_installable_build build_number:$BUILDKITE_PULL_REQUEST


Given the similarities between the 2 scripts (WP vs JP) I wonder if it wouldn't be better to provide the target as argument?

Either by making a common lane build_and_upload_installable_build app:{wordpress|jetpack} build_number:<value> (which could then internally in the Fastfile switch on the parameter and call the appropriate original lane?) – and then pass down that app: argument from the pipeline to the .sh to the fastlane call

Or by keeping individual lanes and make the switch at the .sh level to call the appropriate lane (which would make a smaller change and allow to detect typos earlier)

This could even be something as simple as:

Suggested change

bundle exec fastlane build_and_upload_jetpack_installable_build build_number:$BUILDKITE_PULL_REQUEST

APP="$1"

bundle exec fastlane "build_and_upload_${APP}_installable_build" build_number:$BUILDKITE_PULL_REQUEST

That would avoid the duplication (and double-maintenance) for the 2 otherwise similar cases 🙂

The repetition here exists for all of the other commands too, which was why I figured this was ok. It'd be possible to extract it all our into a shared install-dependencies.sh file, but in the past I've found we've run into a lot of weirdness when we try to over-DRY things 😅

jkmassel · 2021-12-04T22:22:33Z

.buildkite/commands/build-for-testing.sh

 gem install bundler

 echo "--- :rubygems: Setting up Gems"
-restore_cache "$(hash_file .ruby-version)-$(hash_file Gemfile.lock)"


This was redundant

mokagio

Looking good. Left just one question about a possible early return.

mokagio · 2021-12-06T08:30:50Z

.buildkite/pipeline.yml

-  - label: ":pipeline: Build"
+  - label: ":pipeline: Build for Testing"


mokagio · 2021-12-06T08:33:04Z

.circleci/config.yml

-  Jetpack Nightly:
-    triggers:
-       - schedule:
-           cron: "0 0 * * *"
-           filters:
-             branches:
-               only:
-                 - develop
-    jobs:
-      - Jetpack Installable Build


I checked the Buildkite schedules tab and there is no equivalent for this.

Has it been deemed unnecessary?

I'll let Jeremy reply but my guess is, since we'll now create Installable Builds for all PRs without waiting for confirmation (i.e. we won't have the "Hold" job anymore), this indeed kinda makes those Nightly Builds irrelevant anymore?

Yep, this is the case – we can also trivially re-add it with a separate pipeline file, but I'd like to see if it's actually needed :)

mokagio · 2021-12-06T08:34:25Z

Gemfile.lock

+  revision: 83e4d5f036f1871ce6f8e9411550d4040cebddca
+  branch: add/github-comment-action


Asking because the PR is "Ready for review", not the "Draft": do you plan on shipping a new release toolkit build before merging this?

IMHO it would be appropriate, but I'm okay if it doesn't happen.

Just chiming in to say that since I likely won't get back to edit the release-toolkit (to port more actions to it as part of my L10n project) that soon, as I'm still working on the client apps's widgets and extensions, I'm ok with us doing a minor version of the release-toolkit for this if deemed appropriate indeed. I'll soon need to work on a couple of new actions (for the Siri Intents and all) but that can totally go in a separate release-toolkit minor version after we do a new release for this.

That is, as long as @mokagio you confirm that you fixing your local env on wordpress-mobile/release-toolkit#314 (comment) and wordpress-mobile/release-toolkit#315 (comment) fixed those issues you had and that those are not blockers to making a new version which would include localization changes too.

PS: I'll make a quick PR to isolate my change that fixes #314 so it can be merged easier (rather than waiting for my PR on next widget to land) – EDIT: done in #17627

I'm following @AliSoftware's lead on this, so I'm fine to continue to target develop (I'll update this reference) as long as he needs to, or move to a tag if one is created.

I followed up on the dirname issue confirming it was due to messed up state on my end. Still unclear about the --tags one, but #17627 will prevent it from happening during a code freeze again (whatever the tags-related cause is), so we're good 👍

Addressed in 138b43f

mokagio · 2021-12-06T08:36:15Z

fastlane/Fastfile

-    # Install SentryCLI prior to trying to upload dSYMs
-    sh('curl -sL https://sentry.io/get-cli/ | bash')
-


The fact that CI passes is proof enough that this is unnecessary now, but for reference, this is unnecessary because our CI agents are provisioned with it already.

Is this impacting our confidence in being able to run a release build locally in an emergency, if we needed to?

Good question – I'd be fine to do something like an UI.user_error('Sentry not installed') unless sh('command -v sentry-cli') in a before hook to preserve running locally in an uninitialized environment, but IMHO having a curl $ANYTHING | bash is generally a bad idea (though if you blame this file, I think I'm the one who put this here... 😅 )

I like the idea of erroring out if a pre-required external tool is not available, with a suggestion on how to install it, but without doing it for the user.

If we attempted to install it, we'd take on board the responsibility of keeping the install code working and up to date.

Addressed in 19e1106

mokagio · 2021-12-06T08:37:39Z

fastlane/Fastfile

      dsym_path: lane_context[SharedValues::DSYM_OUTPUT_PATH]
    )

+    return if ENV['BUILDKITE_PULL_REQUEST'].nil?


Have you considered moving this check to the start of the lane? Is there a use case in which we would run all the above not on a pull request?

Is there a use case in which we would run all the above not on a pull request

I don't know of one, but I also can't entirely rule it out (for instance, @startuptester had requested the ability to create an installable build of develop on-demand from a webhook, which I think could call this action and "just work")

fastlane/Jetpack-Fastfile

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

AliSoftware · 2021-12-07T01:18:27Z

Could we make it so that the CI check name appears with a less ugly name on GitHub checks? (Ideally all while still keeping the 🛠️ emoji in Buildkite logs?)

jkmassel · 2021-12-09T20:56:47Z

Could we make it so that the CI check name appears with a less ugly name on GitHub checks? (Ideally all while still keeping the 🛠️ emoji in Buildkite logs?)

Yep addressed in 19e1106

AliSoftware · 2021-12-15T13:25:26Z

Ohhh so that's where those pipeline- prefixes that I've been seeing in many other steps's names before! 👍

FWIW I was about to submit a bug report to Buildkite, but then noticed that other people beat me to it, so logging this here for reference:

https://forum.buildkite.community/t/github-status-check-names-and-emojis/686 ➡️ Don't hesitate to upvote this one for visibility 😉
Improve github status integration (steps) buildkite/agent#374 ➡️ Unfortunately closed 3+ years ago as part of their spring cleaning and never addressed 😞

I wonder if we could push this back into their radar by pinging them on our Slack Connection with them? 🤔

[EDIT] Note that the Forum answer hints at the fact that they were working on this back in 2020. I took a look at the docs again and I saw that there's a key attribute (in addition to label) for command steps. Could it be used to solve this then, providing a concise key name and hoping they now use that (and not label as the base for the GitHub status check name?

jkmassel · 2021-12-16T00:21:17Z

@AliSoftware I've used the github_commit_status key for the moment to set prettier job names. I figure we can iterate on this in the future?

AliSoftware · 2021-12-16T14:30:22Z

@AliSoftware I've used the github_commit_status key for the moment to set prettier job names. I figure we can iterate on this in the future?

That looks perfect 👍 Note I see that the 22 checks on this PR still include both the old and new names, but I'm assuming it's because those checks ran with the old name before you made the change and that's why they still appear here, and wouldn't in subsequent PRs once this is merged, right?

(And sorry, I didn't intent to derail the original intent of this PR that much with that aside about CI check names 😅 )

jkmassel · 2021-12-16T19:17:02Z

I'm assuming it's because those checks ran with the old name before you made the change and that's why they still appear here, and wouldn't in subsequent PRs once this is merged, right?

This is my assumption as well 🙂

AliSoftware

Inconsistencies in WP vs JP, both in build number used and logic to derive it.
Misssing check for sentry
Obsolete require

AliSoftware · 2021-12-17T15:45:44Z

fastlane/Jetpack-Fastfile

  desc "Builds and uploads a Jetpack installable build"
  lane :build_and_upload_jetpack_installable_build do | options |
    jetpack_alpha_code_signing
+    require 'git'


This os obsolete now that we rely on the last_git_commit action

Removed in de0c32a

AliSoftware · 2021-12-17T15:46:31Z

fastlane/Jetpack-Fastfile


-    # Install SentryCLI prior to trying to upload dSYMs
-    sh("curl -sL https://sentry.io/get-cli/ | bash") unless sh("which sentry-cli")
-


No call to ensure_sentry_installed here to replace this deleted code?

Nice one – fixed in 210e9c7

AliSoftware · 2021-12-17T15:51:38Z

fastlane/Jetpack-Fastfile

+    short_hash = last_git_commit[:abbreviated_commit_hash]
+
+    build_number = [
+      short_hash,
+      '-',
+      Time.new.strftime("%Y%m%d-%H%M")
+    ].join


Any reason why the build_number for Jetpack is forced to be this <sha1-Ymd-HM> (nice) but Wordpress does not seem to follow that same logic in the main Fastfile?

Why not make WordPress follow the same version format?

Aligned as part of de0c32a

AliSoftware · 2021-12-17T15:59:40Z

.buildkite/commands/installable-build-jetpack.sh

+bundle exec fastlane run configure_apply
+
+echo "--- :hammer_and_wrench: Building"
+bundle exec fastlane build_and_upload_jetpack_installable_build build_number:$BUILDKITE_PULL_REQUEST


bundle exec fastlane build_and_upload_jetpack_installable_build build_number:$BUILDKITE_PULL_REQUEST

According to the Jetpack-Fastfile the build_number parameter is totally unused in the case of Jetpack, as instead the sha1 and datetime are used as build number.

We should definitively uniformize this so that both WP and JP use the same build-number format, especially when they're both related to the same build and commit, so we won't compare apple to oranges.

I like the idea of having the BUILDKITE_PULL_REQUEST as part of the build number if it's present, though including the sha1 even if it comes from a PR seems relevant to differentiate Installable Builds from same PR but generated before/after additional commits being pushed to the PR. I don't really see a strong point in having the date and time in the build number tho.

I'd suggest to use a similar format to what is used with Android, namely prNNN-sha1 for Installable Builds made from PRs, and trunk-sha1 for the ones one might build from a branch (e.g. if an EW manually triggers a build on the current trunk branch to test it, or if we do nightly builds, etc?).

Something like this probably (not tested):

Suggested change

bundle exec fastlane build_and_upload_jetpack_installable_build build_number:$BUILDKITE_PULL_REQUEST

if [[ "$BUILDKITE_PULL_REQUEST" == "false" ]]; then

BUILD_NUMBER="${BUILDKITE_BRANCH}-${BUILDKITE_COMMIT:0:7}"

else

BUILD_NUMBER="pr${BUILDKITE_PULL_REQUEST}-${BUILDKITE_COMMIT:0:7}"

fi

bundle exec fastlane build_and_upload_jetpack_installable_build build_number:"$BUILD_NUMBER"

Then you'd need to ensure that the logic in the Jetpack-Fastfile for JP is the same as the one in the Fastfile for WP.

Addressed in de0c32a – I like the format, but I moved the logic into Ruby – this makes it a bit easier to maintain IMHO, and also drops the requirement for build numbers to be specified on the command line – the lane has enough information in any case to decide what it should be called.

If, in the future, we need the ability to override it, I'd be fine to add the option back in.

mokagio · 2022-01-10T05:51:34Z

@jkmassel I'm going to bump this to the next release because we'll be code freezing 19.0 today and it seems like this needs a bit more touches.

mokagio · 2022-01-24T10:08:47Z

I'm going to bump this to the next release because we'll be code freezing 19.1 today and it seems like this needs a bit more touches.

…ds-to-prs # Conflicts: # .circleci/config.yml # Gemfile.lock

AliSoftware

Looks all great now 👍

PS: Just saw that my last review ended up being individual comments instead of a proper PR review submission, sorry 😅 (did it from my phone which is probably what misled me 😅 )

AliSoftware · 2022-01-28T11:35:53Z

fastlane/Fastfile

+    branch = ENV['BUILDKITE_BRANCH']
+    pr_num = ENV['BUILDKITE_PULL_REQUEST']
+
+    return pr_num == 'false' ? "#{branch}-#{commit}" : "pr#{pr_num}-#{commit}"


Way more readable than the previous way 👍 👏

AliSoftware · 2022-01-28T11:39:47Z

fastlane/Fastfile

+
+    comment_on_pr(
+      project: 'wordpress-mobile/wordpress-ios',
+      pr_number: Integer(ENV['BUILDKITE_PULL_REQUEST']),


nit: never seen the use of Integer(string) in Ruby before, TIL, as I believe the idiomatic way to do that usually is to use string.to_i instead 🙃

mokagio · 2022-01-28T15:24:39Z

🎉

jkmassel added 2 commits November 23, 2021 14:25

Add WP/JP installable builds

3713880

Remove Sentry auto-installation

c0cb6b8

jkmassel changed the title ~~Add/installable builds to prs~~ Create Installable Builds with Buildkite Nov 28, 2021

Add built-in PR commenting

9153b24

jkmassel force-pushed the add/installable-builds-to-prs branch from 87c7282 to 9153b24 Compare November 28, 2021 18:23

Use more precise JP installable build versions

b2cbb0f

AliSoftware reviewed Nov 30, 2021

View reviewed changes

fastlane/Jetpack-Fastfile Outdated Show resolved Hide resolved

AliSoftware reviewed Nov 30, 2021

View reviewed changes

Use develop for the release toolkit

418b7d0

jkmassel self-assigned this Dec 3, 2021

jkmassel added [Type] Enhancement Tooling Build, Release, and Validation Tools labels Dec 3, 2021

jkmassel added this to the 18.9 milestone Dec 3, 2021

jkmassel added 3 commits December 3, 2021 21:25

Fix argument type

94d7c63

Fix version codes

446811d

Only create installable builds for PRs

017b585

jkmassel requested a review from a team December 4, 2021 22:19

jkmassel marked this pull request as ready for review December 4, 2021 22:19

jkmassel commented Dec 4, 2021

View reviewed changes

mokagio reviewed Dec 6, 2021

View reviewed changes

Use the last_git_commit action

8ab3f5c

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

This was referenced Dec 8, 2021

[HACK] A new CI step to check if dependencies were changed woocommerce/woocommerce-android#5420

Merged

Dummy PR to test dependency diff woocommerce/woocommerce-android#5430

Closed

jkmassel added 2 commits December 9, 2021 13:54

Fix long step names

e0173f1

Ensure Sentry is installed

19e1106

jkmassel added this to the 19.0 milestone Dec 14, 2021

Use short github job names

d59b875

AliSoftware requested changes Dec 17, 2021

View reviewed changes

mokagio modified the milestones: 19.0, 19.1 Jan 10, 2022

mokagio modified the milestones: 19.1, 19.2 Jan 24, 2022

jkmassel added 2 commits January 27, 2022 15:17

Use consistent build versions

de0c32a

Ensure Sentry is installed for Jetpack, too

210e9c7

jkmassel requested a review from AliSoftware January 27, 2022 22:39

Merge remote-tracking branch 'origin/trunk' into add/installable-buil…

bd115c6

…ds-to-prs # Conflicts: # .circleci/config.yml # Gemfile.lock

jkmassel enabled auto-merge January 27, 2022 22:50

Fix editor goofs

fe5dbdd

auto-merge was automatically disabled January 27, 2022 22:52
Pull Request is not mergeable

Use a more-specific method name

fa6c143

jkmassel enabled auto-merge January 27, 2022 22:53

jkmassel added 2 commits January 27, 2022 16:01

Cleanup generate_installable_build_number

d9ef9c1

Fix a bad variable name

d8ea670

auto-merge was automatically disabled January 27, 2022 23:07
Pull Request is not mergeable

jkmassel enabled auto-merge January 27, 2022 23:55

AliSoftware approved these changes Jan 28, 2022

View reviewed changes

jkmassel merged commit 7cda265 into trunk Jan 28, 2022

jkmassel deleted the add/installable-builds-to-prs branch January 28, 2022 11:41

	bundle exec fastlane build_and_upload_jetpack_installable_build build_number:$BUILDKITE_PULL_REQUEST
	APP="$1"
	bundle exec fastlane "build_and_upload_${APP}_installable_build" build_number:$BUILDKITE_PULL_REQUEST

		- label: ":pipeline: Build"
		- label: ":pipeline: Build for Testing"

		revision: 83e4d5f036f1871ce6f8e9411550d4040cebddca
		branch: add/github-comment-action

		# Install SentryCLI prior to trying to upload dSYMs
		sh('curl -sL https://sentry.io/get-cli/ \| bash')


		# Install SentryCLI prior to trying to upload dSYMs
		sh("curl -sL https://sentry.io/get-cli/ \| bash") unless sh("which sentry-cli")

-bundle exec fastlane build_and_upload_jetpack_installable_build build_number:$BUILDKITE_PULL_REQUEST
+if [[ "$BUILDKITE_PULL_REQUEST" == "false" ]]; then
+  BUILD_NUMBER="${BUILDKITE_BRANCH}-${BUILDKITE_COMMIT:0:7}"
+else
+  BUILD_NUMBER="pr${BUILDKITE_PULL_REQUEST}-${BUILDKITE_COMMIT:0:7}"
+fi
+bundle exec fastlane build_and_upload_jetpack_installable_build build_number:"$BUILD_NUMBER"

Conversation

jkmassel commented Nov 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression Notes

Uh oh!

peril-wordpress-mobile bot commented Nov 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkmassel commented Nov 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkmassel commented Nov 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mokagio left a comment

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!

AliSoftware Dec 6, 2021 • 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!

mokagio Dec 6, 2021 • 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!

jkmassel Dec 6, 2021 • 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!

jkmassel Dec 6, 2021 • edited by mokagio 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!

Uh oh!

AliSoftware commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkmassel commented Dec 9, 2021

Uh oh!

AliSoftware commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkmassel commented Dec 16, 2021

Uh oh!

AliSoftware commented Dec 16, 2021

Uh oh!

jkmassel commented Dec 16, 2021

jkmassel commented Nov 27, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Nov 27, 2021 •

edited

Loading

jkmassel commented Nov 28, 2021 •

edited

Loading

jkmassel commented Nov 28, 2021 •

edited

Loading

AliSoftware Dec 6, 2021 •

edited

Loading

mokagio Dec 6, 2021 •

edited

Loading

jkmassel Dec 6, 2021 •

edited

Loading

jkmassel Dec 6, 2021 •

edited by mokagio

Loading

AliSoftware commented Dec 7, 2021 •

edited

Loading

AliSoftware commented Dec 15, 2021 •

edited

Loading

AliSoftware Dec 17, 2021 •

edited

Loading