Skip to content

Conversation

@tdeekens
Copy link
Contributor

@tdeekens tdeekens commented Mar 17, 2025

Summary

This repository contains library code mainly. We settled on using GitHub Actions for these kind of repos. At the same time this repo already used GitHub Actions for releasing the library. In addition to using CircleCI for building, testing and linting. This duality of CI is not ideal and should be homogenized.

For this to work I imported credentials from CircleCI to GitHub. I also updated some very old documentation links I found.

Screenshot 2025-03-17 at 19 52 35@2x

One approved the branch protection will be updated. Then the CircleCI project delted.

@changeset-bot
Copy link

changeset-bot bot commented Mar 17, 2025

⚠️ No Changeset found

Latest commit: b70124a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.76%. Comparing base (5c86d1b) to head (b70124a).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1936   +/-   ##
=======================================
  Coverage   97.76%   97.76%           
=======================================
  Files         147      147           
  Lines        4786     4786           
  Branches     1283     1283           
=======================================
  Hits         4679     4679           
  Misses        104      104           
  Partials        3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tdeekens tdeekens requested review from barbara79 and Copilot March 17, 2025 16:09
@tdeekens tdeekens self-assigned this Mar 17, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the CI/CD configuration by removing CircleCI support and standardizing on GitHub Actions across the repository. Key changes include:

  • Introducing a composite GitHub Action in .github/actions/ci/action.yml for dependency installation and Node.js setup.
  • Creating new GitHub Actions workflows for quality checks and regression testing.
  • Updating release workflows and documentation to reference GitHub Actions instead of CircleCI/Travis.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
.github/actions/ci/action.yml Adds a composite GitHub Action to install dependencies
.github/workflows/quality.yml Adds a workflow for linting, type checking, and testing
.github/workflows/regression.yml Adds a workflow for regression testing with multiple Node.js versions
docs/README.md Updates CI badge from Travis CI to GitHub Actions
.github/workflows/release.yml Refactors the release workflow to use the new composite action
docs/cli/csv-parser-price.md Replaces Travis with GitHub Actions CI badge
docs/cli/csv-parser-orders.md Replaces Travis with GitHub Actions CI badge

@@ -0,0 +1,22 @@
name: CI
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 is re-used among the actions.

pull_request:

jobs:
linting:
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 start with lining. So static analysis.

- name: Lint
run: pnpm lint

type-checking:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type checking runs in parallel.


testing:
name: Testing
needs: [linting, type-checking]
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 only test once our static analysis finished.

- name: Integration tests
run: pnpm test:integration

regression-testing:
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 isolate out the integration tests. The idea is that we consider this regression testing. Where we run on the nvmrc's version of Node.js before. Then we test if our changes cause regressions on older versions of Node.js

matrix:
version: [18, 20]
fail-fast: true
max-parallel: 1
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 is important. We run against one project in integration tests. We can't run in parallel.

<p align="center">
<a href="https://travis-ci.org/commercetools/nodejs">
<img alt="Travis CI Status" src="https://img.shields.io/travis/commercetools/nodejs/master.svg?style=flat-square&label=travis">
<a href="https://github.com/commercetools/nodejs/actions">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed. These were wrong for a long time.

@tdeekens tdeekens requested a review from islam3zzat March 17, 2025 18:55
@tdeekens tdeekens marked this pull request as ready for review March 17, 2025 19:03
@tdeekens
Copy link
Contributor Author

Note, that tests all pass. The failure indication is due to the configured branch protections which I would change once this is approved.

Screenshot 2025-03-18 at 08 36 25@2x

runs-on: ubuntu-latest
strategy:
matrix:
version: [18, 20]
Copy link
Member

Choose a reason for hiding this comment

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

I think it will be better if we set this matrix to [20, 22], since 18 is now in maintenance mode.

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 test 22 in the regular testing flow. That's defined in the .nvmrc and what we develop with. v18 is not EoL and we officially still support it. Hence I would argue to also regression test for it. If we drop it, we should officially do so using the engines field and a major release. Keep in mind that with the latest major release we require >= Node.js v18 - prior we said to support v12 even.

Hope this makes sense. In a nutshell: let's not release major and deprecate Node.js v18 support in a migration to GitHub Actions.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, this make sense.

@ajimae
Copy link
Member

ajimae commented Mar 18, 2025

I believe we will also need to migrate over the secrets/env variables for the integration tests, and remove the nodejs repository from the list of CircleCI projects and also in list of configured apps on Github (I don't have the permission to do this though). I can see it's still trying to run these tests without its configuration file. I can help with the secrets for the test.

Screenshot 2025-03-18 at 11 30 57 Screenshot 2025-03-18 at 11 31 57

@tdeekens
Copy link
Contributor Author

I believe we will also need to migrate over the secrets/env variables for the integration tests, and remove the nodejs repository from the list of CircleCI projects and also in list of configured apps on Github (I don't have the permission to do this though). I can see it's still trying to run these tests without its configuration file. I can help with the secrets for the test.

Screenshot 2025-03-18 at 11 30 57 Screenshot 2025-03-18 at 11 31 57

I've migrated everything over and tests run to completion on the PR. If you approve the PR I would take care of the clean up and remove CircleCI entirely.

@ajimae
Copy link
Member

ajimae commented Mar 18, 2025

I believe we will also need to migrate over the secrets/env variables for the integration tests, and remove the nodejs repository from the list of CircleCI projects and also in list of configured apps on Github (I don't have the permission to do this though). I can see it's still trying to run these tests without its configuration file. I can help with the secrets for the test.
Screenshot 2025-03-18 at 11 30 57
Screenshot 2025-03-18 at 11 31 57

I've migrated everything over and tests run to completion on the PR. If you approve the PR I would take care of the clean up and remove CircleCI entirely.

I just saw this now, awesome 🎉 🎉

@tdeekens
Copy link
Contributor Author

I believe we will also need to migrate over the secrets/env variables for the integration tests, and remove the nodejs repository from the list of CircleCI projects and also in list of configured apps on Github (I don't have the permission to do this though). I can see it's still trying to run these tests without its configuration file. I can help with the secrets for the test.
Screenshot 2025-03-18 at 11 30 57
Screenshot 2025-03-18 at 11 31 57

I've migrated everything over and tests run to completion on the PR. If you approve the PR I would take care of the clean up and remove CircleCI entirely.

I just saw this now, awesome 🎉 🎉

Is this the stamp or approval? 😆

Copy link
Member

@ajimae ajimae left a comment

Choose a reason for hiding this comment

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

LGTM

@tdeekens
Copy link
Contributor Author

  • CircleCI won't build anymore
  • Branch protection updated to reflect GitHub

Merging now and cleaning up secrets in CircleCI.

@tdeekens tdeekens merged commit 01e862a into master Mar 18, 2025
10 of 11 checks passed
@tdeekens tdeekens deleted the drop-circleci branch March 18, 2025 10:39
@barbara79
Copy link
Contributor

Thank you! sorry for the delay LGTM

barbara79 pushed a commit that referenced this pull request Mar 19, 2025
* refactor(ci): to not use circleci but only github

* refactor: to have one regression job
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.

4 participants