Build: Fully migrate from Travis to GitHub Actions#589
Conversation
Support for the `git://` protocol has been removed by GitHub: https://github.blog/2021-09-01-improving-git-protocol-security-github/
|
@fnagel I'd like to merge it on Wednesday at the latest to be able to tackle other PRs on a working builder (the Travis one is broken at the moment for this repo and I don't really want to fix it considering this migration) like #587. If you could review before that day, great, if not, I still hope you can review post-merge. 🙂 |
fnagel
left a comment
There was a problem hiding this comment.
+1 by reading. Looks good afaics.
FYI: you might want to create a branch for testing the GH actions, no need to run it in the main repo.
| run: | | ||
| sudo apt-get install xsltproc | ||
|
|
||
| - name: Cache |
There was a problem hiding this comment.
You might want to add some more fallback restore keys:
restore-keys: |
${{ runner.os }}-node-${{ matrix.node-version }}-npm-lock-
${{ runner.os }}-node-${{ matrix.node-version }}-
${{ runner.os }}-node-
${{ runner.os }}-
There was a problem hiding this comment.
That can be done, although I'm not sure if restoring across Node.js versions is the best idea. Sometimes, switching to a new Node.js major requires a full re-installation or some binary packages may not install properly.
There was a problem hiding this comment.
Also, I don't see a point of including both ${{ runner.os }}-node-${{ matrix.node-version }}-npm-lock- and ${{ runner.os }}-node-${{ matrix.node-version }}- - they have the same variable paths so if one matches, the other one should as well.
There was a problem hiding this comment.
...although, to that point, the official docs also show a similar example:
https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows
Do you see a reason to include all of them?
There was a problem hiding this comment.
I think the restore keys work like a fallback chain for always restoring something at all and then updating it. Speeds up builds when using version matrix. Never had issues with it but not 100% sure if it's not possible to cause issues.
There was a problem hiding this comment.
I don't think it will cause issues, it just looks redundant. There should be no situation where the cache entry for ${{ runner.os }}-node-${{ matrix.node-version }}-npm-lock- for given os & node-version values is matched but for ${{ runner.os }}-node-${{ matrix.node-version }}- for those same values it does not match.
| run: | | ||
| sudo apt-get install xsltproc | ||
|
|
||
| - name: Cache |
There was a problem hiding this comment.
Installing npm before loading caching feels more natural to me. I would move Use Node.js ${{ matrix.NODE_VERSION }} before the cache restoring.
|
OK, let's leave it as-is; we can revisit if it causes issues. |
Initial setup was introduce in #588 so that anything is able to even run. This PR aims at migrating the whole setup.
Ref #588