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

Upgrade Node #3180

Merged
merged 5 commits into from Oct 16, 2019
Merged

Upgrade Node #3180

merged 5 commits into from Oct 16, 2019

Conversation

@rajadain
Copy link
Member

rajadain commented Oct 14, 2019

Overview

Upgrades Node to 12. This was originally done in anticipation of a new build process for the front-end, which has since been deemed extraneous. Yet, our node version is quite out of date and this upgrade has been a long time coming.

This aims to be a minimally disruptive update that gets us to the most current version without changing too much. It also fixes the Testem not exiting issue of #3110.

Connects #2323
Connects #3115

Demo

image

Notes

The ansible-yarn module installs an older version of Node / NPM, which gets overwritten by ansible-nodejs in the next step, which is why Yarn is installed first. Ideally we'd be able to specify to the Yarn module which version of Node we want and just use that, but that is not currently possible. I have an open pull request for that azavea/ansible-yarn#2.

Testing Instructions

  • Check out this branch
  • Remove the node_modules folder $ rm -rf src/mmw/node_modules
  • Destroy the app VM and recreate it $ vagrant destroy -f app && vagrant up
  • Run the tests $ ./scripts/test.sh
    • Ensure tests run
    • Ensure all tests pass
    • Ensure script exits successfully
  • Go to :8000/ and click around to ensure things still work correctly

Checklist

  • All JavaScript tests pass ./scripts/testem.sh
rajadain added 3 commits Oct 14, 2019
Also upgrade node-sass and browserify which depended
on the node version.
NPM had a number of problems running, so we switch to Yarn
@rajadain rajadain added the PA DEP label Oct 14, 2019
@rajadain rajadain requested a review from mmcfarland Oct 14, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 15, 2019

@azavea-bot rebuild

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 16, 2019

CI seems to be failing because it has an older version of Ansible installed that doesn't support recursive copying of a directory under the copy command:

TASK [model-my-watershed.app : Copy JavaScript test dependencies] **************

Tuesday 15 October 2019  07:56:33 -0400 (0:00:00.386)       0:01:24.720 ******* 

fatal: [app]: FAILED! => {"changed": false, "msg": "Remote copy does not support recursive copy of directory: /opt/app/node_modules/mocha"}

I'll change that to explicitly copying specific files.

rajadain added 2 commits Oct 14, 2019
The old version was not compatible with new node.

The latest version of Testem uses the configuration JSON to
set the browsers rather than command line arguments, so we
move "Firefox" from test.sh and testem.json

The `beforeEach` behavior was subtly changed so it only runs
every time for `it`s at the same level, but if they are nested
within `describe`s it only runs for each `describe`, so some
were nested out one level to restore the `beforeEach` behavior.

There were some subtle numerical calculation changes to some
tests under `turfArea`, even though `turfArea` itself was not
upgraded, but they are upgraded to match the new numbers.

Testem also removed embeded frameworks in v1.0.0, so we now
copy over the `mocha` files from node_modules to the static
directory so the custom test page has access to it. For details
see:

  - https://github.com/testem/testem/releases/tag/v1.0.0
  - testem/testem#706
This is a transitive dependency via Ulmo, which
unfortunately doesn't specify the versions of its
dependencies in its requirements.txt file, causing
the latest to be pulled down, sometimes resulting
in breakage of our provisioning.
@rajadain rajadain force-pushed the tt/upgrade-node branch from 77ba6fe to f44521a Oct 16, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 16, 2019

This is now ready for review.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 16, 2019

Enabled JS testing in CI. Going to build this again to exercise that.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 16, 2019

@azavea-bot rebuild

Copy link
Member

mmcfarland left a comment

Front end assets build and tests all ran successfully and exited correctly. Nice job.

@mmcfarland mmcfarland assigned rajadain and unassigned mmcfarland Oct 16, 2019
@rajadain rajadain merged commit 93a3d93 into develop Oct 16, 2019
2 checks passed
2 checks passed
default Build finished.
Details
model-my-watershed-pull-requests Build #4104 succeeded in 8 min 18 sec
Details
@rajadain rajadain deleted the tt/upgrade-node branch Oct 16, 2019
@rajadain rajadain mentioned this pull request Oct 16, 2019
0 of 3 tasks complete
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 16, 2019

Thanks for taking a look!

I made #3181 for following up on the Ansible-Yarn stuff if the PR there ever gets merged.
Also made #3182 for upgrading some other front-end dependencies as a follow up from this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.