Skip to content

Test buildkite_pipeline_upload action in CI #598

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
47 changes: 19 additions & 28 deletions .buildkite/pipeline.yml
Original file line number Diff line number Diff line change
@@ -1,27 +1,11 @@
# yaml-language-server: $schema=https://raw.githubusercontent.com/buildkite/pipeline-schema/main/schema.json
---
# Nodes with values to reuse in the pipeline.
common_params:
env: &xcode_image
IMAGE_ID: xcode-15.2
plugins:
- &ci_toolkit
automattic/a8c-ci-toolkit#2.18.2: ~
- &docker_plugin
docker#v5.8.0:
image: &ruby_version "public.ecr.aws/docker/library/ruby:3.2.2"
propagate-environment: true
environment:
- "RUBYGEMS_API_KEY"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got rid of all this because both were used only once.


steps:
#################
# Build and Test
#################
- group: 🧪 Build and Test"
- group: ":test_tube: Build and Test"
key: test
steps:
- label: "🧪 Build and Test using Ruby {{ matrix.ruby }}"
- label: ":test_tube: Build and Test using Ruby {{ matrix.ruby }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth using the occasion to add a custom check name for that step? (github_commit_status: or whatever the attribute is called)

command: |
echo "--- :ruby: Using ruby {{ matrix.ruby }}"
export RBENV_VERSION={{ matrix.ruby }}
Expand All @@ -39,27 +23,34 @@ steps:

echo "--- :rspec: Run Rspec"
bundle exec rspec --profile 10 --format progress
env: *xcode_image
plugins: [*ci_toolkit]

echo "--- :fastlane: :buildkite: Test pipeline upload action"
bundle exec fastlane run buildkite_pipeline_upload pipeline_file:.buildkite/pipeline_for_upload_action_test.yml
Comment on lines +27 to +28
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered having a dedicated step for this, and maybe we should, but at this point, before getting more feedback, I decided to leave in the same step as the tests.

What do you think?

A dedicated step might make things a bit clearer, but also seems like a lot (including extracting shared setup) for a simple action like this....

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 ok with keeping it here

env:
IMAGE_ID: $IMAGE_ID
plugins:
- $CI_TOOLKIT_PLUGIN
agents:
queue: "mac"
queue: mac
matrix:
setup:
ruby:
- 3.2.2
- $RUBY_VERSION

#################
# Push to RubyGems
#################
- label: ":rubygems: Publish to RubyGems"
key: "gem-push"
key: gem-push
if: build.tag != null
depends_on:
- test
# Note: We intentionally call a separate `.sh` script here (as opposed to having all the
# commands written inline) to avoid leaking a key used in the process in clear in the
# BUILDKITE_COMMAND environment variable.
command: .buildkite/commands/gem-push.sh
plugins: [*docker_plugin]
plugins:
- docker#v5.8.0:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This version could be moved to the shared vars, too, but given it's used only once I haven't done it yet.

Conversely, the CI toolkit was already setup so I leaned into it rather than removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

One benefit I like with moving the version values in shared-pipeline-vars is that it's immediately visible when we do a wave of updates what things could be updated and what plugins are in use in the pipeline and what version of each plugin is used, without having to parse through the YML files content to determine this.

So it's more to have all the versions grouped in one single place making them easy to locate, even if some of them don't really need DRY-ing because they're only used once.

image: "public.ecr.aws/docker/library/ruby:$RUBY_VERSION"
propagate-environment: true
environment:
- RUBYGEMS_API_KEY
agents:
queue: "default"
queue: default
6 changes: 6 additions & 0 deletions .buildkite/pipeline_for_upload_action_test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# yaml-language-server: $schema=https://raw.githubusercontent.com/buildkite/pipeline-schema/main/schema.json
---

steps:
- label: ":ok_hand: If this message is shown, the pipeline upload succeeded"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could benefit a custom (and more concise) github_commit_status.

Could be done in a separate PR though as for it to be useful we'd also want to disable the default status created by Buildkite on each step (in the release-toolkit.tf of buildkite-ci) and switch to making all steps have explicit notify

command: echo "Acknowledged."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make this a bit more useful by, for example, uploading with the shared pipeline vars (see https://github.com/bloom/DayOne-Android/pull/4883/files#diff-dff4b99d4e651ab9d7597876ec8dbe92aa934e42c0fb55f4aadd6c106fc96688R683) and check that a sample value is available

[[ $RUBY_VERSION = $(cat .ruby-version) ]] || false

Alternatively we could use a dedicated file so that the test is more straightforward

[[ $TEST_VALUE_FOR_PIPELINE_UPLOAD_WITH_ENV_FILE = "this string is defined in the file at .buildkite/test_env" ]] || false

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think it could be valuable to test those indeed. After all this is a key feature of the action we might want to test via integration testing:

  • Make the action load the shared-pipeline-vars (which should be the default, btw) and validate that env vars were available
  • Provide a Hash of specific env vars to the action's environment parameter and validate they were taken into account too. That being said, I'm not sure if it's possible to pass a Hash from the CLI via the fastlane run command 🤔

That being said, those features should appear be covered by our unit tests of the action… so we might wonder if it's worth also testing them in integration testing; but that would allow us to ensure Buildkite's behavior with passing env vars via Action.sh behaves as expected at least and end up interpolated properly, so maybe still useful.

12 changes: 12 additions & 0 deletions .buildkite/shared-pipeline-vars
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/bin/sh

# This file is `source`'d before calling `buildkite-agent pipeline upload`, and can be used
# to set up some variables that will be interpolated in the `.yml` pipeline before uploading it.

CI_TOOLKIT_PLUGIN_VERSION="2.18.2"
RUBY_VERSION=$(cat .ruby-version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it'd be good to use the same version in CI as the project uses locally.

But I'm also open to the idea of decoupling them in case we run into some situation where we want to use a version locally for which Docker has no image.

However, if we run into that scenario, maybe we should take a step back and ask why? Given's Ruby adoption, a version with no Docker image seems odd. Maybe if we end up in that scenario we are doing something odd ourselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should use the same Ruby version for both

XCODE_VERSION=$(sed 's/^~> *//' .xcode-version)

export CI_TOOLKIT_PLUGIN="automattic/a8c-ci-toolkit#$CI_TOOLKIT_PLUGIN_VERSION"
export IMAGE_ID="xcode-$XCODE_VERSION"
export RUBY_VERSION
1 change: 1 addition & 0 deletions .xcode-version
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
15.2
Copy link
Contributor

Choose a reason for hiding this comment

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

To be updated to a more recent image at some point (potentially separate PR)?