Skip to content

CI workflow refactor #52

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 9 commits into
base: master
Choose a base branch
from

Conversation

mertssmnoglu
Copy link

@mertssmnoglu mertssmnoglu commented May 17, 2025

Call for Contribution, Coveralls Page

This PR is created for making the CI pipeline stable again for future developments. The changes are very similar to express's CI pipeline to make sure expressjs organization follows the same or similar CI conditions.

Inspired by expressjs/express#5599 and compression ci

Changes Summary

  • Add specific lint job for the CI
  • Set default action permission to contents: read
  • Use SHA hashes for action versions
  • Cover all available nodejs releases
    • nodejs 0.8, 0.10, 0.12
    • iojs 1.8, 2.5, 3.3
    • nodejs 4 to 24
  • Prefer ubuntu-latest as a base runner.
  • Use capitalized run names. like Lint, Test and Coverage

zizmor report

 INFO zizmor: skipping impostor-commit: can't run without a GitHub API token
 INFO zizmor: skipping ref-confusion: can't run without a GitHub API token
 INFO zizmor: skipping known-vulnerable-actions: can't run without a GitHub API token
 INFO zizmor: skipping forbidden-uses: audit not configured
 INFO zizmor: skipping stale-action-refs: can't run without a GitHub API token
 INFO audit: zizmor: 🌈 completed .github/workflows/ci.yml
No findings to report. Good job!

Questions ❓

Do we want manual workflow triggering?

@bjohansebas
Copy link
Member

Is using latest version of ubuntu is a good practice? What about using ubuntu-24.04?

Yes, it's a good practice, especially in open source, since it reduces our maintenance burden. GitHub has secure machines, so there's no risk.

Should we remove deprecated node (and io.js) versions from the test matrix?

Nope, in Express there's another CI (see https://github.com/expressjs/express/blob/4.x/.github/workflows/iojs.yml), which covers support for io.js

@mertssmnoglu
Copy link
Author

Should we remove deprecated node (and io.js) versions from the test matrix?

Nope, in Express there's another CI (see https://github.com/expressjs/express/blob/4.x/.github/workflows/iojs.yml), which covers support for io.js

I didn't notice this, thanks you so much.
14e2be0 This is basically a copy of the express 4.x iojs ci with package read permission and version fixings.

- vhost package has 0 core dependency

Signed-off-by: Mert Şişmanoğlu <[email protected]>
@mertssmnoglu
Copy link
Author

iojs CI is failing when the step runs 'npm install'
Can it be a dependency problem?

npm install

npm ERR! Linux 6.11.0-1014-azure
npm ERR! argv "/home/runner/.nvm/versions/io.js/v1.8.4/bin/iojs" "/home/runner/.nvm/versions/io.js/v1.8.4/bin/npm" "install"
npm ERR! node v1.8.4
npm ERR! npm  v2.9.0
npm ERR! path /home/runner/work/vhost/vhost/node_modules/eslint-plugin-markdown/node_modules/mdast-util-from-markdown/node_modules/@types/mdast/package.json
npm ERR! code ENOTDIR
npm ERR! errno -20
npm ERR! syscall open

npm ERR! ENOTDIR: not a directory, open '/home/runner/work/vhost/vhost/node_modules/eslint-plugin-markdown/node_modules/mdast-util-from-markdown/node_modules/@types/mdast/package.json'
npm ERR! 
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /home/runner/work/vhost/vhost/npm-debug.log
Error: Process completed with exit code 236.

I removed 'npm install' steps because vhost has 0 dependency. 423ade6

@bjohansebas
Copy link
Member

I removed 'npm install' steps because vhost has 0 dependency

We still need to install the devDependencies to run the tests. So that command is necessary

We need to remove the linter for those versions :), like it’s being done in compression (https://github.com/expressjs/compression/blob/5f13b148d2a1a2daaa8647e03592214bb240bf18/.github/workflows/ci.yml#L204).

…rsions

- update trigger on
- concurrency
- permissions
- pinned action versions

Signed-off-by: Mert Şişmanoğlu <[email protected]>
…cting coverage reports

Signed-off-by: Mert Şişmanoğlu <[email protected]>
@mertssmnoglu
Copy link
Author

We need to remove the linter for those versions :), like it’s being done in compression (https://github.com/expressjs/compression/blob/5f13b148d2a1a2daaa8647e03592214bb240bf18/.github/workflows/ci.yml#L204).

Okay. So this CI is similar to compress

I was thinking about using 2 separate workflow like in express but I think ci.yml will cover all versions. If everything goes well I can remove iojs.yml from this PR.

(ℹ️ I couldn't test it locally with act because it's ubuntu images are a bit different)

@mertssmnoglu mertssmnoglu marked this pull request as ready for review May 20, 2025 20:31
Copy link
Member

Choose a reason for hiding this comment

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

This file would be unnecessary

Copy link
Author

Choose a reason for hiding this comment

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

Deleted c47e724

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