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

CI : Github action improvement #744

Merged
merged 4 commits into from
Nov 18, 2019
Merged

CI : Github action improvement #744

merged 4 commits into from
Nov 18, 2019

Conversation

zekth
Copy link
Contributor

@zekth zekth commented Nov 18, 2019

As discussed in #743 (review) Adding the coverage job.

So it's on the same yml file because it's triggered by the same events:

on: [push, pull_request]

But it can be splitted. Also @jsumners in your example you do the matrix job but is it revelant to have it here? Because we want to test once the coverage and not the compatibility on each node version like in the build job. Do we keep it on only one node version like LTS ?

Note: Also increased the sleep in the pino.destination test because IO in the windows image seems to be slower and make the tests fail; with 300 it passes regularly.

Last run for ref : https://github.com/zekth/pino/commit/3aa8b0ff1a6110705638e3abeb786de44378da85/checks?check_suite_id=316635812

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

The parallel configuration is necessary here because of the matrix of jobs. See https://github.com/coverallsapp/github-action/#complete-parallel-job-example

My suggestion to have a coverage.yml would result in, basically:

name: Coverage
runs-on: ubuntu-latest
steps:
  - uses: actions/checkout@v1
  - uses: actions/setup-node@v1
  - run: npm i
  - run: npm run cov-ci
  - uses: coverallsapp/github-action@master
     with:
        github-token: ${{ secrets.GITHUB_TOKEN }}

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@zekth
Copy link
Contributor Author

zekth commented Nov 18, 2019

The parallel configuration is necessary here because of the matrix of jobs. See https://github.com/coverallsapp/github-action/#complete-parallel-job-example

My suggestion to have a coverage.yml would result in, basically:

name: Coverage
runs-on: ubuntu-latest
steps:
  - uses: actions/checkout@v1
  - uses: actions/setup-node@v1
  - run: npm i
  - run: npm run cov-ci
  - uses: coverallsapp/github-action@master
     with:
        github-token: ${{ secrets.GITHUB_TOKEN }}

Yeah totally agree, that's why i've asked in the PR. So which version of node you suggest? 12 (LTS) ?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 29a2419 into pinojs:master Nov 18, 2019
@zekth zekth deleted the improve_ci branch November 18, 2019 23:15
@zekth
Copy link
Contributor Author

zekth commented Nov 26, 2019

CI seems still a bit flaky on Windows : https://github.com/pinojs/pino/runs/313765404

Increasing the sleep time? Seems like IO is randomly slow in this env.

@mcollina
Copy link
Member

mcollina commented Nov 27, 2019 via email

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants