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

Add WordPress and Jetpack Installable Builds #16642

Merged
merged 8 commits into from
Jun 10, 2022
Merged

Conversation

jkmassel
Copy link
Contributor

@jkmassel jkmassel commented May 25, 2022

Moves Installable Builds to Buildkite.

Each build has its own comment to allow the builds to be produced in parallel.

To Test

  • Ensure CI passes.
  • Ensure the download link works for each product.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 25, 2022

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

@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot May 25, 2022
@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot May 25, 2022
@jkmassel jkmassel requested a review from a team May 25, 2022 22:03
@jkmassel jkmassel self-assigned this May 25, 2022
@jkmassel jkmassel added this to the 19.9 ❄️ milestone May 25, 2022
@jkmassel jkmassel marked this pull request as ready for review May 25, 2022 22:03
@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 25, 2022

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16642-02089a9.apk), or scanning this QR code:

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 25, 2022

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16642-02089a9.apk), or scanning this QR code:

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Looking good. I tested the QR codes, but I don't have my Android test device at hand right now, so I didn't install them. I think it's safe to assume they'd be fine, though, given all the logic to build them is in Gradle.

Comment on lines 9 to 10
gem 'fastlane-plugin-wpmreleasetoolkit', git: 'git@github.com:wordpress-mobile/release-toolkit.git', branch: 'trunk'
# gem 'fastlane-plugin-wpmreleasetoolkit', '~> 4.1'
#gem 'fastlane-plugin-wpmreleasetoolkit', git: 'git@github.com:wordpress-mobile/release-toolkit.git', branch: 'trunk'
gem 'fastlane-plugin-wpmreleasetoolkit', '~> 4.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this was an intentional breakage of our convention of always pointing to a stable release of trunk.

We haven't shipped a new version of the toolkit since merging the GlotPress 429 workaround. See the current CHANGELOG.md.

Although, thinking about it... It's conceivable that we'll have a new release out before the next beta (as we don't need to download from GlotPress during code freeze, and the release branch "correctly" points to trunk still), so maybe we can keep this as is.

(I'm leaving a comment about making a release instead of doing it myself because I'm at EOD and I don't want to start something else at this point)

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed by releasing release-toolkit 4.2.0 earlier today (which includes both the changes from 4.1 but also the changes that WPAndroid was pointing to to get the rate limiting fix from the toolkit which were not released yet before), then updating the Pluginfile and re-ran bundle install to update the Gemfile.lock accordingly.

See 7e3abb5

# bundle exec fastlane build_and_upload_installable_build
#####################################################################################
desc "Build an Installable Build and make it available for download"
lane :build_and_upload_wordpress_installable_build do | options |
Copy link
Contributor

Choose a reason for hiding this comment

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

| options |, WordPress-style formatting 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

rubocop would definitively have complained about that one… if we had it set up to lint the fastlane/lanes/*.rb files 😅 (one day…)

Copy link
Contributor

Choose a reason for hiding this comment

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

I took the liberty to address those myself in 144ff03

Comment on lines -15 to -16
agents:
queue: "android"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a legacy from previous iterations? Or is there something special about this that relates with the installable builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good question – it's a legacy-ish setup. The "best practice" is to define this in the Buildkite configuration so it doesn't need to be repeated over and over in the config. That said, I can see an argument for being safer. For now, it aligns with the other projects and we can revisit it later, if needed? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

The "best practice" is to define this in the Buildkite configuration so it doesn't need to be repeated over and over in the config. That said, I can see an argument for being safer.

I can see the argument for being safer, too, but I think it's not as strong as the one for defining our steps in such a way that they can all run on the default kind of agent for the pipeline, and only specify a custom queue for those which do something special.

@ParaskP7 ParaskP7 modified the milestones: 19.9 ❄️, 20.0 May 27, 2022
@AliSoftware
Copy link
Contributor

I'm going to do the code freeze today without including this, because I want to do a new version of the release-toolkit and have that branch point to it first (as we should have done past sprint but failed to find the time).

The current change of the Pluginfile from this PR is incorrect either way because:

  • I think 4.1 would miss some commits that since landed in the toolkit's trunk, and that the Gemfile.lock (and actual commit of the toolkit this resolved to) still included (especially for fixing the rate-limiting stuff in GlotPress), so we definitively need to create a new version of toolkit including those and point to it
  • Even if we indeed want to switch back to a tag like this PR is doing by changing the Pluginfile, it feels super suspicious to me that there is no change in the Gemfile.lock corresponding to that change in the Pluginfile

For those reasons I'm moving this PR's milestone to 20.1 and not merging it before doing the 20.0 code freeze; that being said, once we make a new release-toolkit version hopefully very soon, I'd be happy to make this PR target the release/20.0 branch, fix the Gemfile.lock, and land it in release/20.0 to be able to test it there if we want.

@AliSoftware AliSoftware modified the milestones: 20.0, 20.1 May 30, 2022
@AliSoftware AliSoftware force-pushed the move/builds-to-buildkite branch from 03a8a97 to 2f15ba1 Compare May 31, 2022 16:47
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.

Took the liberty to fix the last couple remaining issues (Pluginfile vs Gemfile.lock inconsistency, new version of release toolkit, rubocop violations, documenting the helper method…) myself to move this forward, so now approving this PR.

@jkmassel may I let you check the 3 commits I added myself to your PR, and hit the Merge button if you agree with them? (as it would be a bit cheating to approve my own changes 😄 )

PS: I've also checked that the PR comments made by the bot above have been updated by the latest run of Buildkite, and the link to the APKs confirm they point to the Installable Builds from the latest commit I added, proving that I didn't break anything with my changes 😅

# bundle exec fastlane build_and_upload_installable_build
#####################################################################################
desc "Build an Installable Build and make it available for download"
lane :build_and_upload_wordpress_installable_build do | options |
Copy link
Contributor

Choose a reason for hiding this comment

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

rubocop would definitively have complained about that one… if we had it set up to lint the fastlane/lanes/*.rb files 😅 (one day…)

# bundle exec fastlane build_and_upload_installable_build
#####################################################################################
desc "Build an Installable Build and make it available for download"
lane :build_and_upload_wordpress_installable_build do | options |
Copy link
Contributor

Choose a reason for hiding this comment

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

I took the liberty to address those myself in 144ff03

Comment on lines 9 to 10
gem 'fastlane-plugin-wpmreleasetoolkit', git: 'git@github.com:wordpress-mobile/release-toolkit.git', branch: 'trunk'
# gem 'fastlane-plugin-wpmreleasetoolkit', '~> 4.1'
#gem 'fastlane-plugin-wpmreleasetoolkit', git: 'git@github.com:wordpress-mobile/release-toolkit.git', branch: 'trunk'
gem 'fastlane-plugin-wpmreleasetoolkit', '~> 4.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed by releasing release-toolkit 4.2.0 earlier today (which includes both the changes from 4.1 but also the changes that WPAndroid was pointing to to get the rate limiting fix from the toolkit which were not released yet before), then updating the Pluginfile and re-ran bundle install to update the Gemfile.lock accordingly.

See 7e3abb5

@jkmassel jkmassel enabled auto-merge June 10, 2022 16:32
@jkmassel jkmassel merged commit 8424b02 into trunk Jun 10, 2022
@jkmassel jkmassel deleted the move/builds-to-buildkite branch June 10, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants