-
Notifications
You must be signed in to change notification settings - Fork 286
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
Fix CI Flakiness/build issues #50
Conversation
cb25538
to
d52c8e5
Compare
d52c8e5
to
5d4d5db
Compare
LGTM, will let @denis-yu-glotov review before merging (believe he has a bank holiday today) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one small comment
@@ -11,7 +11,7 @@ | |||
"start": "node index.js", | |||
"upgrade-interactive": "npm-check --update", | |||
"test": "cross-env NODE_ENV=test ./node_modules/.bin/mocha --ui bdd --reporter spec --colors routes/**/*.test.js --recursive --exit", | |||
"postinstall": "patch ./node_modules/web3-core-helpers/src/formatters.js ./web3_timestamp_fix.patch" | |||
"postinstall": "patch --verbose --force ./node_modules/web3-core-helpers/src/formatters.js ./web3_timestamp_fix.patch; echo Line 266 of formatters.js below:; sed '266!d' ./node_modules/web3-core-helpers/src/formatters.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to scare users with these messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could argue for and against it as well. (awareness > scared, UX > awareness)
The --force
is necessary because without it the patch application fails if patch was already applied (didn't want to force a zero exit code to avoid confusing silent failures).
--verbose
and the dumping of the source code line are there to make sure it's easy to notice/investigate issues, BUT I can refactor it in a way that it's part of a checklist in the static documentation that whenever a dependency is updated the developer must ensure the monkey patching is still working. Will update it in a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant only about what you echo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep --verbose
then, will document how to debug instead of echoing the code.
5d4d5db
to
47db54b
Compare
Also a list of other changes that were required to make the CI green: - clean up steps in the script: remove .git folder of websocket package - install npm dependencies in the fabric and quorum api folders - update web3 to 1.2.4 - update the post install patching of web3 formatter to match v1.2.4 - clean up: run "down" npm script for fabric and quorum - force volume re-creation when calling docker-compose up Also: the CI script is now in a separate .sh file so that it can be invoked locally on any dev machine that has the dependencies pre-installed Fixes #12 Fixes #36 Signed-off-by: Peter Somogyvari <[email protected]>
47db54b
to
c2019ae
Compare
…er.somogyvari/make-ci-pass Fix CI Flakiness/build issues
Fixes the CI.
test:bc is still not working but the flakiness is gone now.