Skip to content
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

Migrate ci pipeline to github actions #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carpasse
Copy link
Contributor

Agreed to migrate to GH actions in expressjs/discussions#229

The new ci action and README.md changes are based on the respective files in accepts repository

@carpasse carpasse force-pushed the migrate-ci-pipeline-to-gh-actions branch 2 times, most recently from 88ffc3e to eab99c9 Compare May 29, 2024 08:12
@carpasse
Copy link
Contributor Author

Copy link
Member

@blakeembrey blakeembrey left a comment

Choose a reason for hiding this comment

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

This seems hard to maintain and not a direct port of the travis code. What's the logic behind changing mocha/nyc/instanbul in this PR? Why is the README changing to badge gen?

It feels like it'd be simpler to run just the steps that you seem to trying to hack around on the versions supported. For example, no need for coverage on node 0.8.

Also, does the existing node action for installing versions not work here?

Can we avoid doing this without changing the package.json file in the test process?

@carpasse
Copy link
Contributor Author

carpasse commented Jun 3, 2024

Hi @blakeembrey

This isn't a direct port because the priority was maintaining consistency with all other GH actions in the express, pillarjs, and jshttp organizations. The change in Mocha was made to use a more modern version compatible with the latest Node.js. Additionally, replacing Istanbul with NYC aligns with how coverage is tracked in all other repositories.

The pipeline removes ESLint for Node.js versions 10 and below, which is consistent with previous actions. I can add it to the install step if you prefer.

@carpasse
Copy link
Contributor Author

carpasse commented Jun 3, 2024

May be worth mentioning that the travis build is not working at the moment
https://app.travis-ci.com/pillarjs/resolve-path

@carpasse carpasse force-pushed the migrate-ci-pipeline-to-gh-actions branch from eab99c9 to 7c81ce3 Compare August 17, 2024 07:32
@carpasse carpasse force-pushed the migrate-ci-pipeline-to-gh-actions branch from 7c81ce3 to aa0d24b Compare August 17, 2024 07:36
@carpasse
Copy link
Contributor Author

I've refactored the CI pipeline task, and I believe it's now more closely aligned with what Travis CI pipeline did.

The key differences from the Travis CI pipeline are:

  • Added missing Node.js versions to the test matrix.
  • Stopped linting for Node.js versions < 12 (previously < 6).
  • Switched to NYC for test coverage collection, replacing Istanbul.

@blakeembrey can you please take a look and let me know if there’s anything you think I should change?

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