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

Bumped NodeJS version #624

Merged
merged 1 commit into from
May 15, 2019
Merged

Bumped NodeJS version #624

merged 1 commit into from
May 15, 2019

Conversation

basraven
Copy link
Contributor

  • Bumped NodeJS version to 10 (container was building but not running any of the examples with the other version)
  • Added SSL ignore to fix ssl issues when performing npm install

@basraven
Copy link
Contributor Author

  • ci/circle CI is failing due to broken mocha tests, not due to this change
  • ci/circle CI already uses the bumped version 10 of NodeJS, this Dockerfile update conforms to this.
  • It aligns the Dockerfile with the Mocha tests NodeJS version (NVM10 is used in ci/circle).

@shamb0t
Copy link
Member

shamb0t commented May 15, 2019

Thanks @basraven! The failure was due to a race condition in one of the tests, they are passing now 👍 Can you elaborate on adding ssl ignore? What issues are you facing with npm install?

@basraven
Copy link
Contributor Author

Thanks @basraven! The failure was due to a race condition in one of the tests, they are passing now 👍 Can you elaborate on adding ssl ignore? What issues are you facing with npm install?

The npm install will complain about self-signed certificates when running npm install, prefix like such will ignore this (once). This will not happen when running the docker build for a second time due to caching in Docker (I expect), this is why it might have been missed. Happy to help!

npm ERR! errno SELF_SIGNED_CERT_IN_CHAIN
npm ERR! request to https://registry.npmjs.org/lodash failed, reason: self signed certificate in certificate chain

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/node/.npm/_logs/2019-05-15T08_47_21_915Z-debug.log`

@vvp
Copy link

vvp commented May 15, 2019

Thanks for the PR @basraven ! 🙂 Version bump is definitely ok but unfortunately we cannot disable the TLS certificate validation for security reasons.

Also, I built this image myself without the NODE_TLS_REJECT_UNAUTHORIZED, and npm install was successful. Therefore it seems that the problem you're seeing may be caused by something else. Perhaps some HTTPS proxy doing certificate manipulation?

- Bumped NodeJS version to 10 (container was building but not running any of the examples with the other version)
@basraven
Copy link
Contributor Author

Thanks for the PR @basraven ! 🙂 Version bump is definitely ok but unfortunately we cannot disable the TLS certificate validation for security reasons.

Also, I built this image myself without the NODE_TLS_REJECT_UNAUTHORIZED, and npm install was successful. Therefore it seems that the problem you're seeing may be caused by something else. Perhaps some HTTPS proxy doing certificate manipulation?

Did you do the build with a purged Docker environment?
It only occurs with a purged Docker environment (on Windows 10).
I'm not behind an https proxy, so this is interesting.

For now: I isolated the NodeJS version bump and force pushed it to have a clean isolated version bump PR.
Let's ignore the SSL issue for now.

@basraven basraven changed the title Bumped NodeJS version and added ssl ignore Bumped NodeJS version May 15, 2019
@aphelionz
Copy link
Member

I think this is OK to merge now, I see no issue with node:10 👍

@shamb0t
Copy link
Member

shamb0t commented May 15, 2019

Happy to merge this now, thank you @basraven! I created an issue to track the certificate error you're getting #626

@shamb0t shamb0t merged commit 396bfe2 into orbitdb:master May 15, 2019
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.

None yet

4 participants