Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Node 8 #4219

Merged
merged 1 commit into from
Jan 8, 2018
Merged

Node 8 #4219

merged 1 commit into from
Jan 8, 2018

Conversation

arizzitano
Copy link
Contributor

@arizzitano arizzitano commented Nov 27, 2017

Changelog here: https://nodejs.org/en/blog/release/v8.0.0/. No explicit upgrade guide, unfortunately, but here's what we can expect:

  • faster npm installs
  • faster builds
  • every project running node will need to check in a package-lock.json file (I've created an epic to track this)

Ops Ansible Checklist


Make sure that the following steps are done before merging:

  • A DevOps team member has approved the PR.
  • Are you adding any new default values that need to be overridden when this change goes live? If so:
    • Update the appropriate internal repo (be sure to update for all our environments)
    • If you are updating a secure value rather than an internal one, file a DEVOPS ticket with details.
    • Add an entry to the CHANGELOG.
  • If you are making a complicated change, have you performed the proper testing specified on the Ops Ansible Testing Checklist? Adding a new variable does not require the full list (although testing on a sandbox is a great idea to ensure it links with your downstream code changes).

@arizzitano
Copy link
Contributor Author

I think this is the failure:

TASK [edx_django_service : install development requirements] *******************
fatal: [127.0.0.1]: FAILED! => {"changed": true, "cmd": ["make", "requirements"], "delta": "0:00:07.209741", "end": "2017-11-27 19:16:47.613636", "failed": true, "rc": 2, "start": "2017-11-27 19:16:40.403895", "stderr": "npm ERR! Cannot read property '0' of undefined\n\nnpm ERR! A complete log of this run can be found in:\nnpm ERR!     /edx/app/ecommerce/.npm/_logs/2017-11-27T19_16_47_598Z-debug.log\nmake: *** [requirements.js] Error 1", "stdout": "npm install\nMakefile:34: recipe for target 'requirements.js' failed", "stdout_lines": ["npm install", "Makefile:34: recipe for target 'requirements.js' failed"], "warnings": []}
	to retry, use: --limit @/edx/app/edx_ansible/edx_ansible/docker/plays/ecommerce.retry

Going to debug in a devstack.

@feanil
Copy link
Contributor

feanil commented Nov 27, 2017

Looks like that error can happen when the version of node and the version of npm mismatch? We use nodeenv under the covers to install the correct version of node. I'm not sure how it determines which versions of npm to install by default so that might be the issue.

@arizzitano
Copy link
Contributor Author

Yeah, looks like 8.9.1 uses node 5.5.1, which has caused this issue for a number of other people. Per this thread, the go-to fix is pinning npm to 5.2.0. This is not a great solution as it gets our npm version out-of-sync with the stock npm version that comes with node, plus it adds additional complexity to our configuration.

For now I'm going to see if the 8.9.2 candidate (nodejs/node#17204) fixes this. If yes, let's wait for it to drop; if not, I'd say let's use an earlier, stable version of 8.x.

@arizzitano
Copy link
Contributor Author

arizzitano commented Dec 6, 2017

Check it out, 8.9.2 dropped yesterday! If tests pass let's go with that, otherwise here are our options (in order of preference)

  • Install an older version of Node that comes bundled with an older npm; add a follow-on ticket to check back in on this in a month or so. Interestingly, no Node release has ever bundled [email protected], which supposedly fixes this issue. Node versions to try, ranked:
    • 8.9.0
    • 8.8.1
    • 8.7.0
    • 8.6.0
    • 8.5.0
    • 8.4.0
    • 8.3.0
    • 8.2.1
    • 8.1.4
  • Stick with Node 8.9.2, but add a common_npm_version pointing at a different release of npm. We should first try 5.6.0 (latest and the npm team claims it fixes this issue), then 5.2.0 (old, but multiple users in the above linked thread claim it fixes the issue for them). Add an Ansible step to explicitly install that version of npm:
npm install -g npm@version

@arizzitano arizzitano changed the title WIP: Node 8 Node 8 Jan 3, 2018
bump node version to 8.9.1 [FEDX-410]

8.9.2

8.8.1

8.7.0

8.6.0

Update back to latest node.

The previous failures were probably due to ecommerce which should now be resolved.
@arizzitano arizzitano merged commit 6d6d186 into master Jan 8, 2018
@arizzitano arizzitano deleted the ari/node-8 branch January 8, 2018 19:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants