-
Notifications
You must be signed in to change notification settings - Fork 131
Fix install / build by using latest LTS node #364
Conversation
Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Joe Farro <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #364 +/- ##
=======================================
Coverage 99.12% 99.12%
=======================================
Files 44 44
Lines 1717 1717
Branches 337 337
=======================================
Hits 1702 1702
Misses 15 15
Continue to review full report at Codecov.
|
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 aside from one dependency question.
General question that doesn't need to be addressed in this PR:
some of these changes seem stylistic, is it worth bolstering our eslint / prettier usage for this repo?
package.json
Outdated
"opentracing": "^0.13.0", | ||
"uuid": "^3.2.1" | ||
"prom-client": "^11.0.0", |
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 don't see this dependency in use in the PR, is it required by using a different node version?
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.
Awesome catch! npm now saves packages, by default!
npm install saves any specified packages into dependencies by default.
– npm help install
Signed-off-by: Joe Farro <[email protected]>
package.json
Outdated
"opentracing": "^0.13.0", | ||
"uuid": "^3.2.1" | ||
"prom-client": "^11.0.0", |
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.
this isn't a required runtime dependency
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.
The newer version of npm installs by default. This has been removed and the Makefile updated to not save the dependency when it's installed.
console.warn( | ||
`You are using deprecated env variable ${env}. Use ${ | ||
deprecatedEnvVars[env] | ||
} instead. \nDeprecated env variable will be removed in the next major release (4.x.x)` |
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.
this split doesn't look right
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.
It's automatic formatting via prettier. I suggest we leave as is.
closes jaegertracing#364 Signed-off-by: Olivier Albertini <[email protected]>
Which problem is this PR solving?
Fix #363.
Short description of the changes
Use the current LTS version of node (see releases).
This adds adds a package-lock.json and causes two prettier formatting changes.