Skip to content

Add github workflows for storage-node and network tests#1020

Merged
mnaamani merged 4 commits intoJoystream:nicaeafrom
mnaamani:ci-network-integration-storage-node
Jul 22, 2020
Merged

Add github workflows for storage-node and network tests#1020
mnaamani merged 4 commits intoJoystream:nicaeafrom
mnaamani:ci-network-integration-storage-node

Conversation

@mnaamani
Copy link
Member

@mnaamani mnaamani commented Jul 22, 2020

Added CI checks for linting and build for network-tests and storage-node.
The network-tests checks are expected to fail because the fixes are applied in a separate PR #1001

run: |
yarn install --frozen-lockfile --network-timeout 120000
yarn workspace storage-node checks
yarn workspace @joystream/storage-cli build
Copy link
Contributor

Choose a reason for hiding this comment

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

This one isn't part of the Ubuntu job, it also doesn't seem necessary, because yarn workspace storage-node run build (which does the same thing) is part of root package.json postinstall script already (so it's beeing run in each of the jobs after yarn install).

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, will remove

Copy link
Member Author

Choose a reason for hiding this comment

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

also I thought I'd already pushed a second commit to add it to ubuntu job :)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 000e2db

@mnaamani mnaamani requested a review from Lezek123 July 22, 2020 09:39
@Lezek123
Copy link
Contributor

Lezek123 commented Jul 22, 2020

It also looks like the storage-node's lint script is ignoring .ts files.
I suggest changing:
"lint": "eslint --ignore-path .gitignore .",
To:
"lint": "eslint --ext .js,.ts --ignore-path .gitignore .",
In storage-node's package.json scripts

@mnaamani
Copy link
Member Author

It also looks like the storage-node's lint script is ignoring .ts files.
I suggest changing:
"lint": "eslint --ignore-path .gitignore .",
To:
"lint": "eslint --ext .js,.ts --ignore-path .gitignore .",
In storage-node's package.json scripts

Aren't those extensions checked by default or do you always have to provide them explicitly?
I'm pretty sure those files have been scanned on prior runs, but I will test and confirm.

cc: @shamil-gadelshin

@Lezek123
Copy link
Contributor

I'm not sure how effective linter with Typescript-specific rules is for .js files, but since not all files are .ts in this workspace yet it may make sense to check both and just decrease some rules error level to warning (the way it's already done) until everything is converted to TypeScript.

@mnaamani
Copy link
Member Author

Yes we had to make quite a few rule tweaking to make the linter happy because that code base is still a hybrid. The plan is to gradually refactor all code to .ts and remove a lot of the rules that were turned off.

@Lezek123
Copy link
Contributor

Aren't those extensions checked by default or do you always have to provide them explicitly?

The documentation says that the default is just .js (https://eslint.org/docs/user-guide/command-line-interface).
Perhaps you've been running yarn workspace @joystream/storage-cli lint which has them configured (but not yarn workspace storage-node lint)

@mnaamani
Copy link
Member Author

okay so with current lint script: I get 328 problems (0 errors, 328 warnings)
and with your suggestion to add the extensions I get: 362 problems (0 errors, 362 warnings)
so clearly some files were not covered.
Will update.

@mnaamani
Copy link
Member Author

Yeah I think you were right about that.

Perhaps you've been running yarn workspace @joystream/storage-cli lint which has them configured (but not yarn workspace storage-node lint)

Copy link
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

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

Looks good!

@mnaamani mnaamani merged commit 93c31cf into Joystream:nicaea Jul 22, 2020
@mnaamani mnaamani deleted the ci-network-integration-storage-node branch July 22, 2020 20:30
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