Add Buildkite

[jkmassel]

This PR adds Buildkite and does some housekeeping, including:

• Adding rubocop in CI
• Requiring that development gems be installed in the vendor directory
• Adding git-lfs installation (IMHO we should deprecate LFS stuff in the future...)
• Removes the test group from the Gemfile – if you're using that instead of the gemspec you are sort of by definition out to do development work on the toolkit, so you should have all the tooling present.

[codecov-commenter]

Codecov Report

Merging #299 (9093ea7) into develop (07b3152) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #299      +/-   ##
===========================================
- Coverage    51.64%   51.62%   -0.03%     
===========================================
  Files          108      108              
  Lines         4533     4533              
===========================================
- Hits          2341     2340       -1     
- Misses        2192     2193       +1     

Impacted Files	Coverage Δ
...ctions/android/android_download_file_by_version.rb	47.82% <ø> (ø)
...olkit/actions/common/check_translation_progress.rb	84.09% <ø> (ø)
...mreleasetoolkit/helper/promo_screenshots_helper.rb	21.54% <0.00%> (-0.56%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07b3152...9093ea7. Read the comment docs.

[AliSoftware]

Looks good overall but I think we should also get rid of GitHub Actions and Danger in this PR to avoid the double rubocop reporting.

We might also want to get rid of the CircleCI config too on that PR, except if you planned to do that in a separate one as a secondary step?

[mokagio]

[AliSoftware]

The gem-push job from CircleCI is missing from the .buildkite/pipeline.yml, we need to add it before being ready to remove CircleCI.

[AliSoftware]

Note that I noticed while doing the 2.0.0 that even today the gem-push job from CircleCI fails because it doesn't install the packages from the .brewfile like it does for the test job.

Not sure why we set skip_brew_install: true on the gem-push job (maybe because we expected the test job to run beforehand and install it for us? But it doesn't run on tags…) but when we'll port this to BuildKite in this PR we need to ensure the brew install step is done during gem push too.

[AliSoftware]

PR update: gem-push step

@jkmassel @mokagio I've updated the PR to add the gem-push step to the Buildkite pipeline:

• Added the gem-push step via these set of commits, with an if: build.tag != null condition
• Added RUBYGEMS_API_KEY to the shared-secrets.json in mobile-secrets, and updated the CI/secrets/release-toolkit/env file to export it, then ran ./build to upload new secrets to S3. (I used the same API Key that we have been using on CircleCI and which is in our Secret Store – didn't create a new one)

You can check the changes I made to mobile-secrets by running git -C ~/.mobile-secrets show c20d2d5c6d0302e7d477284a628dd25572fc9174 on your Mac.

CircleCI Deploy Key re-added (and PR check failure)

I've incidentally re-added a public key for CircleCI during the release of 2.0.0 while trying to gem push the release 2.0.0 to RubyGems via the existing CircleCI job; which is why we have the ci/circleci: Build Error failure in this PR's checks. We'll need to re-delete that deploy key and remove the project from CircleCI once this Buildkite migration is merged to stop it from trying to run anymore.

Reviews

• @jkmassel I can't add you as reviewer of this PR because you're also its author, but please review the changes I made for gem-push before we can consider merging it.
• @mokagio I've dismissed your approval of this PR so that we don't accidentally merge it before someone re-reviews the new changes with new gem-push step.

New commit to add gem-push step was added since this review

[AliSoftware]

Testing the gem-push step

I tested the new gem-push step by temporarily disabling the if build.tag != null condition via 4f86bec to test the command (I expected it to fail on push saying that 2.0.0 already existed, but passing all the previous steps otherwise).

Had to do a lot of back and forth 😓 to test and make gem push be able to use authentication as expected 😓 .

• First I tried to use the same method as what we used on CircleCI, i.e. writing the credentials in $HOME/.gem/credentials
• But then it appeared that we don't have write access to $HOME in our Buildkite images.
• Then I tried to use the GEM_HOST_API_KEY env var instead of the ~/.gem/credentials file, as suggested by the gem push help here, but apparently it wasn't picked up by CI (while it seemed to work on my Mac… may it depends on the version of rubygems?).
• So after a lot of reading in the docs and finally looking at the rubygems source code on any alternatives to set the API keys (the official docs only mentioned ~/.gem/credentials as an acceptable file…) I finally found out that using --config-file would apparently not only allow to provide default flags for gem commands (original documented purpose, similar to what we can put into a gemrc file)… but also allows us to provide the API key via this file.

The last solution finally "succeeded" (well the step failed but on the error that "version 2.0.0 has already been published" which is the error I expected – as opposed to being blocked on an interactive prompt asking us for our credentials, or failing to authenticate with RubyGems), so I reverted the test commit via ac143b1 to only do the gem push on tags.

[mokagio]

This looks ready to ship. I only left nitpicks 👍

I assume you intent to keep the test and revert commit pair, as opposed interactive rebase and drop them, because there's no mention of removing them. Correct?

Part of me would like to get rid of them to keep the Git history neat, but I can see the value in having them here for the extra context. You did a great job at making it clear they are test commit, so I'm sure if someone landed on them (e.g. during a bisect) they'd understand the state of the codebase is intentionally "incorrect". 👌

The CircleCI failure is obviously due to this PR removing CircleCI 😄

[mokagio]

This LGTM. Just curious, though, did you decide to remove the test group because:

• Everyone running bundle install on this project is expected to run the tests; and
• Consumer of the gem get the dependencies via what's specified in the .gemspec, so there's no need to add groups here because, if we don't add these to the .gemspec they won't be part of the gem dependency resolution chain

?

[AliSoftware]

Yep, exactly. As Jeremy said in the description of this PR:

Removes the test group from the Gemfile – if you're using that instead of the gemspec you are sort of by definition out to do development work on the toolkit, so you should have all the tooling present.

[AliSoftware]

I assume you intent to keep the test and revert commit pair, as opposed interactive rebase and drop them, because there's no mention of removing them. Correct?

Yep.

I'm not strong about this and I'm ok either case, and I thought about rebasing to eliminate those 2 commits too instead of keeping them. But as you say it also helps keep context – especially if someone comes back to this PR and the conversations in it even after the PR got merged – also because I do mention those commits' SHA1 in some of my comments, so removing them would make the comment outdated and odd to read in the future.

Overall even if I do like a clean and nice git graph and history, I don't think it's that a big deal to include those, both to keep context for this PR but also to show in the git history that we did test this scenario, and maybe show future us how we test similar stuff if we need more similar changes to be tested later.

[AliSoftware]

The CircleCI failure is obviously due to this PR removing CircleCI 😄

I've just hit the "Stop Building" red button on the project settings in CircleCI which has removed the CircleCI Deploy Key from this repo's settings in GitHub, so CircleCI will no longer try to build this project (and fail).