Move to Github Actions #785

NotMoni · 2020-04-23T12:45:57Z

No description provided.

larson-carter

I agree that this is a great move. Approved!!

Richienb

Since we're moving to Actions, it makes sense to remove the .travis.yml file.

.github/workflows/nodejs.yml

Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

NotMoni · 2020-05-19T22:23:03Z

Done 🦄

tinovyatkin · 2020-05-19T22:57:14Z

.github/workflows/nodejs.yml

+    - run: npm run lint
+    - run: npm run coverage
+      env:
+        CI: true


GitHub Actions sets CI automatically now: https://github.blog/changelog/2020-04-15-github-actions-sets-the-ci-environment-variable-to-true/

Coveralls have their own GitHub Actions, so, it's probably better to use it than to run coverage script: https://github.com/coverallsapp/github-action

GitHub Actions sets CI automatically now

Awesome 😍

GitHub Actions sets CI automatically now: https://github.blog/changelog/2020-04-15-github-actions-sets-the-ci-environment-variable-to-true/

Coveralls have their own GitHub Actions, so, it's probably better to use it than to run coverage script: https://github.com/coverallsapp/github-action

So use coveralls?

tinovyatkin · 2020-05-19T23:00:32Z

.github/workflows/nodejs.yml

+      uses: actions/setup-node@v1
+      with:
+        node-version: ${{ matrix.node-version }}
+    - run: npm run lint


It's better to separate lint into a single action - this way it will run only once on a selected version of Node/OS and give faster and more prominent feedback to a contributor.

I agree. Could we make it a separate workflow? First node then lint?

Yea, Should I make it two files?

Yes, that what I mean

@NotMoni Yes make it into 2 files.

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

NotMoni · 2020-05-20T20:43:14Z

@larson-carter and @tinovyatkin
Please look over my changes. 🦄

tinovyatkin · 2020-05-21T01:35:13Z

.github/workflows/lint.yml

+name: Lint CI
+
+on:
+  [push, pull_request]


It makes no sense to lint on a push as nobody will see/check that. Keep just pull_request here

tinovyatkin · 2020-05-21T01:35:49Z

.github/workflows/lint.yml

+  [push, pull_request]
+
+jobs:
+  build:


I think lint is better named for the task here. It will be visible in check and may confuse.

tinovyatkin · 2020-05-21T01:37:04Z

.github/workflows/lint.yml

+    runs-on: [ubuntu-latest, macos-latest, ubuntu-16.04]
+
+    strategy:
+      matrix:
+        node-version: [10.x, 12.x, 14.x]
+
+    steps:
+    - uses: actions/checkout@v2
+    - name: Use Node.js ${{ matrix.node-version }}
+      uses: actions/setup-node@v1
+      with:
+        node-version: ${{ matrix.node-version }}


It's a real overkill to lint on 9 platforms. Just run on Ununtu latest with the Node LTS (12.x) - we are not testing ESLint here, we linting our code.

tinovyatkin · 2020-05-21T01:37:56Z

.github/workflows/coverage.yml

@@ -0,0 +1,22 @@
+# This workflow will do a clean install of node dependencies, build the source code and run tests across different versions of node
+name: Coverage CI


I think the coverage is a bad name here. Just CI or Test. Coverage is a function from tests.

tinovyatkin · 2020-05-21T01:38:45Z

.github/workflows/coverage.yml

+name: Coverage CI
+
+on:
+  [push, pull_request]


I think just pull_request here is enough, we currently don't have any automatic deploy in place, so, CI result on a push is irrelevant.

tinovyatkin · 2020-05-21T01:41:57Z

.github/workflows/coverage.yml

+    runs-on: [ubuntu-latest, macos-latest, ubuntu-16.04]
+
+    strategy:
+      matrix:
+        node-version: [10.x, 12.x, 14.x]


Again, I believe 9 tests is too much for the library that doesn't have any platform-specific code.
We can test only on latest Node for macOs an test on windows instead of old Ubuntu:

strategy: matrix: os: [ubuntu-latest, windows-latest, macOS-latest] node: [14.x, 12.x, 10.x] exclude: # On Windows, run tests with only the LTS environments. - os: windows-latest node: 10.x - os: windows-latest node: 14.x # On macOS, run tests with only the LTS environments. - os: macOS-latest node: 10.x - os: macOS-latest node: 14.x runs-on: ${{ matrix.os }}

tinovyatkin · 2020-05-21T01:43:25Z

.github/workflows/coverage.yml

+      uses: actions/setup-node@v1
+      with:
+        node-version: ${{ matrix.node-version }}
+    - run: npm run coverage


npm run coverage does nothing if npm test wasn't run before.
Please check #805 as if it will be merged we will need a special step and condition to test on Node 10.x

Runs only on: Ubuntu & Node.js LTS

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

NotMoni · 2020-05-21T02:03:38Z

@tinovyatkin I think I fixed all your suggestions. 👀
Sorry in advance if I messed up somewhere 😭

.github/workflows/ci.yml

.github/workflows/lint.yml

xxczaki · 2020-05-21T06:57:39Z

@NotMoni Remember to update the badge in README before/just after merging ;)

NotMoni · 2020-05-21T10:57:45Z

@xxczaki Ahhhh yea, sure mate 👍

larson-carter

We just have 2 suggestions that need to be merged into this PR. Then it will be ready to be pulled.

NotMoni · 2020-05-21T13:47:22Z

Hey @larson-carter , I will update it later today. Busy rn.

Co-authored-by: Konstantin Vyatkin <tino@vtkn.io>

NotMoni · 2020-05-22T16:23:45Z

cc/ @larson-carter
The Needed changes have been made.

NotMoni · 2020-05-22T16:31:28Z

@NotMoni Remember to update the badge in README before/just after merging ;)

I have added the badge link, but it won't be rendered since we have not enable action on this repo until this PR gets merged.

Move to Github Actions

30b6d84

NotMoni assigned eaviles, bitinn, Richienb and xxczaki May 6, 2020

NotMoni added enhancement help wanted labels May 6, 2020

larson-carter approved these changes May 16, 2020

View reviewed changes

Richienb requested changes May 17, 2020

View reviewed changes

LinusU reviewed May 19, 2020

View reviewed changes

.github/workflows/nodejs.yml Outdated Show resolved Hide resolved

NotMoni and others added 2 commits May 19, 2020 18:21

remove travis in favor of github actions

5651acc

Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>

remove travis in favor of github acitons

b0080f7

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

tinovyatkin requested changes May 19, 2020

View reviewed changes

tinovyatkin reviewed May 19, 2020

View reviewed changes

tinovyatkin mentioned this pull request May 20, 2020

drop Babel (while keeping ESM) #805

Merged

NotMoni added 4 commits May 20, 2020 16:37

ci set to true by default

73afcf9

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

renamed to lint for lint CI

fceedea

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

fix comment

96208f8

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

added coverage CI

3f04b52

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

NotMoni requested a review from Richienb May 20, 2020 20:49

tinovyatkin requested changes May 21, 2020

View reviewed changes

NotMoni added 3 commits May 20, 2020 21:58

update ci.yml

c3f0af5

update lint.yml to run once

3e5e1d2

Runs only on: Ubuntu & Node.js LTS

remove whitespace

c627aa6

Signed-off-by: Moni <40552237+NotMoni@users.noreply.github.com>

tinovyatkin requested changes May 21, 2020

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

.github/workflows/lint.yml Outdated Show resolved Hide resolved

xxczaki mentioned this pull request May 21, 2020

v3 Roadmap #668

Closed

35 tasks

larson-carter reviewed May 21, 2020

View reviewed changes

NotMoni and others added 2 commits May 22, 2020 12:22

fix lit ci

fbc07d4

Co-authored-by: Konstantin Vyatkin <tino@vtkn.io>

fix ci with coverage and test

3a3f060

Co-authored-by: Konstantin Vyatkin <tino@vtkn.io>

NotMoni added 2 commits May 22, 2020 12:27

added github ci badge

bf16088

fix indent and node-version on ci.yml

9c2b1eb

tinovyatkin approved these changes May 22, 2020

View reviewed changes

tinovyatkin merged commit 29d7556 into node-fetch:master May 22, 2020

tinovyatkin mentioned this pull request May 22, 2020

Fix GitHub Actions #820

Merged

NotMoni deleted the NotMoni-github-actions branch May 22, 2020 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to Github Actions #785

Move to Github Actions #785

NotMoni commented Apr 23, 2020

larson-carter left a comment

Richienb left a comment

NotMoni commented May 19, 2020

tinovyatkin May 19, 2020

LinusU May 20, 2020

NotMoni May 20, 2020

tinovyatkin May 19, 2020

larson-carter May 20, 2020

NotMoni May 20, 2020

tinovyatkin May 20, 2020

larson-carter May 20, 2020

NotMoni May 20, 2020

NotMoni commented May 20, 2020 •

edited

Loading

tinovyatkin May 21, 2020

tinovyatkin May 21, 2020

larson-carter May 21, 2020

tinovyatkin May 21, 2020

tinovyatkin May 21, 2020

tinovyatkin May 21, 2020

tinovyatkin May 21, 2020

tinovyatkin May 21, 2020

NotMoni commented May 21, 2020

xxczaki commented May 21, 2020

NotMoni commented May 21, 2020

larson-carter left a comment

NotMoni commented May 21, 2020

NotMoni commented May 22, 2020

NotMoni commented May 22, 2020

		@@ -0,0 +1,22 @@
		# This workflow will do a clean install of node dependencies, build the source code and run tests across different versions of node
		name: Coverage CI

Move to Github Actions #785

Move to Github Actions #785

Conversation

NotMoni commented Apr 23, 2020

larson-carter left a comment

Choose a reason for hiding this comment

Richienb left a comment

Choose a reason for hiding this comment

NotMoni commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NotMoni commented May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NotMoni commented May 21, 2020

xxczaki commented May 21, 2020

NotMoni commented May 21, 2020

larson-carter left a comment

Choose a reason for hiding this comment

NotMoni commented May 21, 2020

NotMoni commented May 22, 2020

NotMoni commented May 22, 2020

NotMoni commented May 20, 2020 •

edited

Loading