Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

add workflow for GitHub Actions #1073

Closed
wants to merge 1 commit into from

Conversation

erossignon
Copy link
Contributor

No description provided.

uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}
- run: sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace all these lines with a single run: |, also did you try to keep only the npm install and npm test commands?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe consolidate them into Install dependencies step.

npm install and npm test should remain separate steps.

@robertsLando
Copy link
Contributor

Also remember to replace the badge in readme with the workflow badge

- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I see a matrix but there is no one specified in this job

runs-on: ubuntu-latest
strategy:
matrix:
node-version: [10.x, 14.x]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't include 12.x too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. And, add Current (15.x) as well. It is important for this project to track the latest developments of Node.js.

- run: sudo apt-get update -qq
- run: sudo apt-get -yq --no-install-suggests --no-install-recommends --force-yes install gcc-4.8 g++-4.8
- run: sudo apt-get -yq --no-install-suggests --no-install-recommends --force-yes install libavahi-compat-libdnssd-dev # npm install mdns
- run: export CXX="g++-4.8" CC="gcc-4.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

GCC 4.8? Not acceptable in 2021.

Why not simply use build-essential?

I would also like to see a workflow for the latest GCC and Clang.**

** I can do this myself later, though.

@leerob
Copy link
Member

leerob commented Mar 23, 2021

Thanks for this! Looking great - agree with the feedback above 🚀

@robertsLando
Copy link
Contributor

Supersided by #1074

@erossignon
Copy link
Contributor Author

Thank you guy, let's close this one.

@erossignon erossignon closed this Mar 23, 2021
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

4 participants