-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
CI workflow refactor #52
Conversation
Signed-off-by: Mert Şişmanoğlu <[email protected]>
…c action versions Signed-off-by: Mert Şişmanoğlu <[email protected]>
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.
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 |
Signed-off-by: Mert Şişmanoğlu <[email protected]>
I didn't notice this, thanks you so much. |
- vhost package has 0 core dependency Signed-off-by: Mert Şişmanoğlu <[email protected]>
|
iojs CI is failing when the step runs '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 |
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]>
Signed-off-by: Mert Şişmanoğlu <[email protected]>
Okay. So this CI is similar to compress I was thinking about using 2 separate workflow like in express but I think (ℹ️ I couldn't test it locally with act because it's ubuntu images are a bit different) |
Signed-off-by: Mert Şişmanoğlu <[email protected]>
|
We are almost there! Great work @mertssmnoglu! Seems like we need to fix the artifacts (ref): |
|
I solved the conflicts that I generated when I merged #50 👍 |
Co-authored-by: Sebastian Beltran <[email protected]>
Signed-off-by: Mert Şişmanoğlu <[email protected]>
I found a solution(walkaround) actions/upload-artifact#480 (comment) |
…iple reports Signed-off-by: Mert Şişmanoğlu <[email protected]>
Signed-off-by: Mert Şişmanoğlu <[email protected]>
Signed-off-by: Mert Şişmanoğlu <[email protected]>
Signed-off-by: Mert Şişmanoğlu <[email protected]>
…name Signed-off-by: Mert Şişmanoğlu <[email protected]>
Co-authored-by: Sebastian Beltran <[email protected]>
bjohansebas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @mertssmnoglu !
UlisesGascon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
We're finally there! It's a great pleasure for me to see a pipeline without any failing step. |
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
expressjsorganization follows the same or similar CI conditions.Inspired by expressjs/express#5599 and compression ci
Changes Summary
lintjob for the CIcontents: readubuntu-latestas a base runner.Lint,TestandCoveragezizmor report
Questions ❓
Do we want manual workflow triggering?