Repository navigation
👷 Move to GitHub Actions - #49
Conversation
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
Thank you @harshzalavadiya ! It looks very promising. I have just few comments
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| node: ["0.8"] |
There was a problem hiding this comment.
Is it not possible to test v0.6?
To be fully bulletproof on this level, we should test all major since v0.6 (so also v0.10, v0.12, v4, v6 and v8)
There was a problem hiding this comment.
I tried doing multiple builds yesterday on github actions also on DO server but sadly all I got was segment fault errors same is happening on Travis CI as well if you see current build logs
Here's commit history of old repo https://github.com/tiny-leaf/querystring-old/commits/node-0.6-test
There was a problem hiding this comment.
Ok, thanks for investigating that. If that's not possible, we need to stick to 0.8 as you've configured
There was a problem hiding this comment.
Still, can we have v0.10, v0.12, v4, v6 and v8 covered?
| @@ -0,0 +1,3 @@ | |||
| test | |||
| .github | |||
| package-lock.json | |||
There was a problem hiding this comment.
Let's ditchpackage-lock.json from here (it is automatically ignored by npm)
Also we have this file just added to repo with #47, so I believe this have to be updated against master
There was a problem hiding this comment.
Done, Since History.md was already there I have added CHANGELOG.md
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
Thank you @harshzalavadiya ! Please see my comments
| History.md | ||
| test | ||
| .github | ||
| CHANGELOG.md |
There was a problem hiding this comment.
I believe CHANGELOG.md is automatically taken by npm (ignoring it is ineffective)
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| node: ["0.8"] |
There was a problem hiding this comment.
Still, can we have v0.10, v0.12, v4, v6 and v8 covered?
| - name: Publish | ||
| if: startsWith(github.ref, 'refs/tags/') && matrix.os == 'ubuntu-latest' && matrix.node == '14.x' | ||
| run: echo "//registry.npmjs.org/:_authToken=$NPM_AUTH_TOKEN" > ~/.npmrc && npm publish --access public | ||
| env: | ||
| NPM_AUTH_TOKEN: ${{ secrets.NPM_AUTH_TOKEN }} |
There was a problem hiding this comment.
Let's remove publication from here.
It should be done only on version tag builds, so if configured with CI, I think it's best if we create another dedicated workflow file for it
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
Thanks for update @harshzalavadiya ! Please see my comments
| on: | ||
| push: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
Ideally this should be configured on tags.
Nice workflow I have setup in other packages i that:
- master build checks wether version is bumped in
package.jsonIf it's bumped then tag is pushed to repository - tag build publishes new version to npm
You may check how that's achieved e.g. here: https://github.com/serverless/utils/tree/master/.github/workflows. It is also documented here: https://github.com/serverless/utils/blob/master/RELEASE_PROCESS.md
Still I think it's totally fine if in scope of this PR we just focus on PR's and master validation (no publishing)
There was a problem hiding this comment.
yes, using tags validation directly is much cleaner workflow, I have added tag validation instead of branch from example
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
Thank you @harshzalavadiya ! I have just few suggestions to publication workflow and we should be good to go
| run: npm test | ||
|
|
||
| - name: Publish | ||
| run: echo "//registry.npmjs.org/:_authToken=$NPM_AUTH_TOKEN" > ~/.npmrc && npm publish --access public |
There was a problem hiding this comment.
Instead of that. Let's configure registry-url at actions/setup-node@v1 (as e.g. here: https://github.com/serverless/template-package/blob/50f01db83c28c7b7c8e8437564efe9fe73bd3434/.github/workflows/publish.yml#L35)
|
|
||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: |
There was a problem hiding this comment.
Let's not use matrix in that case (we're publishing in context of one env)
|
@medikoo sorry to be a bother but any feedback on this PR? |
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
Thank you @harshzalavadiya !
Sorry, I didn't look into it as simply I didn't receive any notification it's ready for review (review was not re-requested).
Split from #46
This PR is to move CI from Travis to GitHub Actions
I'm not adding node 0.6 in legacy matrix since it's not able to do setup and same fails on travis and GH actions both and even if setup succeds it keeps on throwing segment fault