Test `buildkite_pipeline_upload` action in CI #598

mokagio · 2024-09-30T05:11:04Z

What does it do?

Calls the new buildkite_pipeline_upload action in CI to upload a dedicated pipeline and have an integration-level test for the action.

See #597 (review)

Checklist before requesting a review

Run bundle exec rubocop to test for code style violations and recommendations
Add Unit Tests (aka specs/*_spec.rb) if applicable
Run bundle exec rspec to run the whole test suite and ensure all your tests pass
Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section. – N.A.
If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider. – N.A.

dangermattic · 2024-09-30T05:12:47Z

	2 Warnings
⚠️	Please add an entry in the CHANGELOG.md file to describe the changes made by this PR
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

mokagio · 2024-09-30T05:44:32Z

.buildkite/pipeline.yml

-        image: &ruby_version "public.ecr.aws/docker/library/ruby:3.2.2"
-        propagate-environment: true
-        environment:
-          - "RUBYGEMS_API_KEY"


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

mokagio · 2024-09-30T05:46:19Z

.buildkite/pipeline.yml

+          echo "--- :fastlane: :buildkite: Test pipeline upload action"
+          bundle exec fastlane run buildkite_pipeline_upload pipeline_file:.buildkite/pipeline_for_upload_action_test.yml


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....

I'm ok with keeping it here

mokagio · 2024-09-30T05:48:47Z

.buildkite/pipeline.yml

    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:


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.

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.

mokagio · 2024-09-30T05:51:46Z

.buildkite/shared-pipeline-vars

+# 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)


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?

I agree we should use the same Ruby version for both

mokagio · 2024-09-30T05:54:03Z

.buildkite/shared-pipeline-vars

+# 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="3.7.1"


Notice this is now the latest version. We were a few versions behind.

mokagio · 2024-09-30T06:01:46Z

.buildkite/pipeline_for_upload_action_test.yml

+
+steps:
+  - label: ":ok_hand: If this message is shown, the pipeline upload succeeded"
+    command: echo "Acknowledged."


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

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.

AliSoftware · 2024-09-30T08:34:03Z

.buildkite/pipeline.yml

    key: test
    steps:
-      - label: "🧪 Build and Test using Ruby {{ matrix.ruby }}"
+      - label: ":test_tube: Build and Test using Ruby {{ matrix.ruby }}"


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

AliSoftware · 2024-09-30T08:34:34Z

.buildkite/pipeline.yml

+          echo "--- :fastlane: :buildkite: Test pipeline upload action"
+          bundle exec fastlane run buildkite_pipeline_upload pipeline_file:.buildkite/pipeline_for_upload_action_test.yml


I'm ok with keeping it here

AliSoftware · 2024-09-30T08:38:50Z

.buildkite/pipeline.yml

    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:


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.

AliSoftware · 2024-09-30T08:46:40Z

.buildkite/pipeline_for_upload_action_test.yml

+
+steps:
+  - label: ":ok_hand: If this message is shown, the pipeline upload succeeded"
+    command: echo "Acknowledged."


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.

AliSoftware · 2024-09-30T08:47:29Z

.xcode-version

@@ -0,0 +1 @@
+15.2


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

AliSoftware · 2024-09-30T08:50:04Z

.buildkite/shared-pipeline-vars

+# 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)


I agree we should use the same Ruby version for both

AliSoftware · 2024-09-30T08:53:49Z

.buildkite/pipeline_for_upload_action_test.yml

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


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

iangmaia · 2024-09-30T16:33:21Z

Thanks for this @mokagio! Looks like the build with the pipeline upload worked fine 🎉
I've merged #597 and released 12.2.0 for release-toolkit.

AliSoftware · 2024-11-21T12:08:41Z

@mokagio I just stumbled upon this old PR while looking at the opened PRs for this repo.

It this still relevant?
If so, could benefit fixing conflicts on the pipeline file as well as some freshing-up (account for some new convention decisions made since like wrt to using the emoji in labels after all, adding git_commit_status notify blocks, updating the IMAGE_ID to a more recent one, …) 🙂

mokagio · 2024-11-27T05:19:19Z

@AliSoftware I'd say it's still relevant yet. It doesn't hurt to have these tests does it?

However, just to set expectations, I don't think I'll have time to address this before my support rotation next week subsequent AFK.

mokagio force-pushed the mokagio/test-upload-buildkite-pipeline-action branch 3 times, most recently from 6a74513 to 755fec6 Compare September 30, 2024 05:17

mokagio added 2 commits September 30, 2024 15:20

Adopt shared pipeline vars for CI

ac82697

Remove comments that said the same as labels and unnecessary quotations

d51ba84

mokagio force-pushed the mokagio/test-upload-buildkite-pipeline-action branch from 755fec6 to d51ba84 Compare September 30, 2024 05:20

Add test for pipeline upload action

314fbef

mokagio commented Sep 30, 2024

View reviewed changes

Update CI toolkit from 2.x to 3.7.1

3e74526

mokagio commented Sep 30, 2024

View reviewed changes

mokagio requested a review from a team September 30, 2024 05:54

mokagio marked this pull request as ready for review September 30, 2024 05:54

mokagio commented Sep 30, 2024

View reviewed changes

AliSoftware reviewed Sep 30, 2024

View reviewed changes

Base automatically changed from iangmaia/upload-buildkite-build-action to trunk September 30, 2024 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test `buildkite_pipeline_upload` action in CI #598

Test `buildkite_pipeline_upload` action in CI #598

mokagio commented Sep 30, 2024 •

edited

Loading

dangermattic commented Sep 30, 2024 •

edited

Loading

mokagio Sep 30, 2024

mokagio Sep 30, 2024

AliSoftware Sep 30, 2024

mokagio Sep 30, 2024

AliSoftware Sep 30, 2024

mokagio Sep 30, 2024

AliSoftware Sep 30, 2024

mokagio Sep 30, 2024

mokagio Sep 30, 2024

AliSoftware Sep 30, 2024

AliSoftware Sep 30, 2024

AliSoftware Sep 30, 2024

AliSoftware Sep 30, 2024

AliSoftware Sep 30, 2024

AliSoftware Sep 30, 2024

AliSoftware Sep 30, 2024

AliSoftware Sep 30, 2024

iangmaia commented Sep 30, 2024 •

edited

Loading

AliSoftware commented Nov 21, 2024

mokagio commented Nov 27, 2024

		echo "--- :fastlane: :buildkite: Test pipeline upload action"
		bundle exec fastlane run buildkite_pipeline_upload pipeline_file:.buildkite/pipeline_for_upload_action_test.yml

		@@ -0,0 +1 @@
		15.2

Test buildkite_pipeline_upload action in CI #598

Are you sure you want to change the base?

Test buildkite_pipeline_upload action in CI #598

Conversation

mokagio commented Sep 30, 2024 • edited Loading

What does it do?

Checklist before requesting a review

dangermattic commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iangmaia commented Sep 30, 2024 • edited Loading

AliSoftware commented Nov 21, 2024

mokagio commented Nov 27, 2024

Test `buildkite_pipeline_upload` action in CI #598

Test `buildkite_pipeline_upload` action in CI #598

mokagio commented Sep 30, 2024 •

edited

Loading

dangermattic commented Sep 30, 2024 •

edited

Loading

iangmaia commented Sep 30, 2024 •

edited

Loading