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: Lint & Test #470

Merged
merged 10 commits into from
Apr 12, 2021
Merged

CI: Lint & Test #470

merged 10 commits into from
Apr 12, 2021

Conversation

fvictorio
Copy link
Member

Also: upgrade solidity-comments-extractor

@fvictorio fvictorio requested review from Janther and mattiaerre April 10, 2021 01:18
Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

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

this is great!

steps:
- uses: actions/setup-node@v1
with:
node-version: 10
Copy link
Member

Choose a reason for hiding this comment

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

shall we use node 12 for this?

Copy link
Member Author

@fvictorio fvictorio Apr 11, 2021

Choose a reason for hiding this comment

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

I copied this from the hardhat repo, we can modify it if you want. For reference:

  • Node 10 will reach end of life in the coming weeks
  • Node 11 and 13 are not maintained anymore
  • Node 15 reaches end of life on June 1st

.github/workflows/CI.yml Outdated Show resolved Hide resolved
run: npm run test:all

test_macos:
name: Test on MacOS with Node 10
Copy link
Member

Choose a reason for hiding this comment

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

can we use Node 12 here too?

runs-on: ubuntu-latest
strategy:
matrix:
node: [10, 12, 14, 15]
Copy link
Member

Choose a reason for hiding this comment

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

I would just target 12, 14, and 15

Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

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

some more comments to you @fvictorio

with:
path: |
node_modules
key: ${{ runner.os }}-node-${{ matrix.node }}-${{ hashFiles('package-lock.json') }}
Copy link
Member

Choose a reason for hiding this comment

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

I do not see any matrix.node above

with:
path: |
node_modules
key: ${{ runner.os }}-node-${{ matrix.node }}-${{ hashFiles('package-lock.json') }}
Copy link
Member

Choose a reason for hiding this comment

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

I do not see any matrix.node above

with:
path: |
node_modules
key: ${{ runner.os }}-node-${{ matrix.node }}-${{ hashFiles('package-lock.json') }}
Copy link
Member

Choose a reason for hiding this comment

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

this matrix.node here makes sense to me

Comment on lines 60 to 61
- name: install vyper
run: docker pull ethereum/vyper:0.1.0b10
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this docker image?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, error from the copy-paste.

Copy link
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

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

it looks like GH actions are running so I'm happy w/ this

@mattiaerre mattiaerre changed the title Remove travis and use GH actions CI: Lint & Test Apr 10, 2021
@fvictorio
Copy link
Member Author

Fixed the remaining points. @mattiaerre feel free to merge if you want!

@mattiaerre
Copy link
Member

thanks @fvictorio I'll merge and publish a new version of the plugin right after

@mattiaerre mattiaerre merged commit 46551b4 into master Apr 12, 2021
@mattiaerre mattiaerre deleted the add-gh-actions branch April 12, 2021 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants