Skip to content

Add cli build checks, and circular dep check for types lib#752

Merged
mnaamani merged 4 commits intoJoystream:nicaeafrom
mnaamani:ci-types-cli
Jun 25, 2020
Merged

Add cli build checks, and circular dep check for types lib#752
mnaamani merged 4 commits intoJoystream:nicaeafrom
mnaamani:ci-types-cli

Conversation

@mnaamani
Copy link
Member

Adding CI checks for types and cli packages

@mnaamani mnaamani requested a review from Lezek123 June 22, 2020 12:29
@mnaamani mnaamani changed the title github actions: add type and cli checks Add @joystream/types and cli build checks Jun 22, 2020
@Lezek123
Copy link
Contributor

Lezek123 commented Jun 23, 2020

As discussed in the chat: it would probably be a good idea to include MacOS builds too (like it's already done for Pioneer), since with different filesystem etc. there may be some issues that will be undetected on Ubuntu.

@mnaamani
Copy link
Member Author

As discussed in the chat: it would probably be a good idea to include MacOS builds too (like it's already done for Pioneer), since with different filesystem etc. there may be some issues that will be undetected on Ubuntu.

Agreed. Added in 79a87d8

I also increased the TCP network timeout for OSX jobs to try to resolve the frequent issue with installing packages due to unstable network connections on the mac instances. 2067c50

@Lezek123
Copy link
Contributor

I noticed that since building @joystream/types is part of the yarn postInstall script, any build error in @joystream/types will actually cause all the checks to fail (Lezek123#7). This means that build checks for @joystream/types itself may currently be unnecessary and we also don't need to run yarn workspace @joystream/types build before the CLI build check. Perhaps we can add a circular dependency check instead for the types (with madge --circular types right after yarn install)?

The CLI build check seems to work fine, I was even able to produce an error that causes the build to fail only on Mac ( Lezek123#8)

Didn't run into any issues regarding network connection during Mac builds, so hopefully it's now fixed.

@mnaamani
Copy link
Member Author

Great feedback.
Will drop the jobs exlusively for types and add the circular check as suggested.

@mnaamani mnaamani changed the title Add @joystream/types and cli build checks Add cli build checks, and circular dep check for types lib Jun 24, 2020
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.
The circular dependencies are correctly spotted etc.
It doesn't seem necessary to do the circular dependency check in each job, but since @joystream/types are beeing built in each job too, it seems kind of logical (and this way we also avoid having too many serparate checks).

@mnaamani
Copy link
Member Author

After working on #785 I identified a useful check we can add for the types library which I can do in a future PR. Basically to ensure that the module can be published to npm and installed successfully buy using npm pack to test it locally.

@mnaamani mnaamani merged commit d13e31a into Joystream:nicaea Jun 25, 2020
@mnaamani mnaamani deleted the ci-types-cli branch June 26, 2020 05:28
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