Skip to content

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented May 30, 2024

Moves the lint and unit tests CI steps from CircleCI to Buildkite.

It uses an experimental approach to npm that:

  1. Runs npm ci instead of npm install, as recommended for CI jobs
  2. Uses caching, but since we're using npm ci, which recreates node_modules, only caches the NPM cache.
  3. Has a gate at the start of the pipeline to create the cache if necessary, so that we'll avoid race conditions where multiple jobs might attempt to upload the NPM cache simultaneously.

I'm a bit unsure about the approach to be honest. Is the cache gate necessary? What are the chances of race condition? Besides, the performance of using vs not using the cache with npm ci do not seem to be that different, which makes me question whether it's worth having in the first place.

Keen to hear what you think. I'd still propose to merge as is and see how it behaves. This investigation has a broader scope that just this project, as we have new Electron apps that might use this new approach.

Test

See green CI.

Release

N.A.

@mokagio mokagio force-pushed the mokagio/buildkite-ci-lint-and-test branch from d77025e to d4f2e48 Compare May 30, 2024 04:31
@mokagio mokagio changed the base branch from trunk to release/2.22.0 May 30, 2024 04:31
@mokagio mokagio requested a review from codebykat May 30, 2024 07:15

# --legacy-peer-deps is necessary because of react-monaco-editor.
# See README for more details
install_npm_packages_clean --legacy-peer-deps
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mokagio mokagio requested a review from a team May 30, 2024 07:20
@mokagio
Copy link
Contributor Author

mokagio commented May 30, 2024

Requested an @Automattic/apps-infrastructure review for feedback on the npm approach.

@codebykat the head is release/2.22.0 because I'm wishfully hoping to port the rest of the setup before it's time to cut the final build. At worst, we'll ship 2.22.0 on the old infra and continue the upgrade later.

Copy link
Contributor

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

Okay by me, if it works - I don't have an opinion about the npm caching. I know CircleCI is doing a bunch of stuff to sign the builds already, but it's finicky, maybe Buildkite is less finicky (however I have not worked with Buildkite at all). I'm happy for us to do whatever integrates best with the apps infra processes.

I did leave a note about the .xcode-version which probably shouldn't be there.

@mokagio
Copy link
Contributor Author

mokagio commented Jun 24, 2024

Superseded by #3204

@mokagio mokagio closed this Jun 24, 2024
@mokagio mokagio deleted the mokagio/buildkite-ci-lint-and-test branch June 24, 2024 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants