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

chore(nodejs): exit installation if Node.js version is unsupported #1010

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

sanderblue
Copy link
Contributor

No description provided.

@sanderblue sanderblue force-pushed the chore/check-nodejs-version branch 2 times, most recently from 2c9302d to 4ed95ac Compare January 2, 2024 20:32
mbruzina
mbruzina previously approved these changes Jan 8, 2024
stevula
stevula previously requested changes Jan 8, 2024
Copy link

@stevula stevula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on an EC2 instance with node 14 and 16 managed via nvm and a test app running via pm2. I locally compiled newrelic-cli using the code from this branch and ran the following install script:

curl -Ls https://download.newrelic.com/install/newrelic-cli/scripts/install.sh | bash && sudo NEW_RELIC_API_KEY=NRAK-**** NEW_RELIC_ACCOUNT_ID=**** NEW_RELIC_CLI_SKIP_CORE=1 ./newrelic install -n node-agent-installer --debug

Node 14

Expected behavior:
Script should fail with error "Node.js version not supported [...]"

Actual behavior:
🔴 Script shows node agent as "Installed" but the entity does not show up in NR UI under All Entities view.
🔴 Debug output shows errors in the preInstall commands:

DEBUG "node": executable file not found in $PATH
DEBUG "node": executable file not found in $PATH
DEBUG 7:4: not a valid test operator: 16
DEBUG 7:4: not a valid test operator: 16
DEBUG 7:4: 16 must be followed by a word
DEBUG 7:4: 16 must be followed by a word

Node 16

Expected behavior:
Script should show Node agent as installed

Actual Behavior:
✅ Script shows Node agent as installed.
🟡 Debug output shows errors in the preInstall commands (same as above).


A possible problem here is that the install script is run with sudo and node/nvm might not be on the secure path.

@stevula stevula self-requested a review January 8, 2024 22:31
@sanderblue
Copy link
Contributor Author

I tested this on an EC2 instance with node 14 and 16 managed via nvm and a test app running via pm2. I locally compiled newrelic-cli using the code from this branch and ran the following install script:

curl -Ls https://download.newrelic.com/install/newrelic-cli/scripts/install.sh | bash && sudo NEW_RELIC_API_KEY=NRAK-**** NEW_RELIC_ACCOUNT_ID=**** NEW_RELIC_CLI_SKIP_CORE=1 ./newrelic install -n node-agent-installer --debug

Node 14

Expected behavior: Script should fail with error "Node.js version not supported [...]"

Actual behavior: 🔴 Script shows node agent as "Installed" but the entity does not show up in NR UI under All Entities view. 🔴 Debug output shows errors in the preInstall commands:

DEBUG "node": executable file not found in $PATH
DEBUG "node": executable file not found in $PATH
DEBUG 7:4: not a valid test operator: 16
DEBUG 7:4: not a valid test operator: 16
DEBUG 7:4: 16 must be followed by a word
DEBUG 7:4: 16 must be followed by a word

Node 16

Expected behavior: Script should show Node agent as installed

Actual Behavior: ✅ Script shows Node agent as installed. 🟡 Debug output shows errors in the preInstall commands (same as above).

A possible problem here is that the install script is run with sudo and node/nvm might not be on the secure path.

Should be working now after my recent changes. Let me know if you run into anymore issues. Thanks!

@sanderblue sanderblue dismissed stevula’s stale review January 10, 2024 21:09

Confirm this is working to the extent it can and is low risk.

@sanderblue sanderblue merged commit 4542e93 into main Jan 10, 2024
9 of 10 checks passed
@sanderblue sanderblue deleted the chore/check-nodejs-version branch January 10, 2024 21:14
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.

3 participants