ci(NODE-5098): download node to local directory

[nbbeeken]

Description

What is changing?

Download nodejs to a local node-artifacts directory
Don't impact the global environment of the host

What is the motivation for this change?

nvm windows modifies the global environment

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[baileympearson]

This is a long script, could we consider breaking it into smaller functions? Or moving the logic that determines the download URL into a separate file? That might help with the readability and modularity of the script.

[nbbeeken]

It's shorter than the script that proceeded it, I'd like to keep it in one file to make the porting and future updating easier. Perhaps I could put functionality inside bash functions to organize it? but it will be longer as a result

[baileympearson]

I don't really care about the length, if it's well organized. This file is just dense and the code is opaque. Some logical grouping might make it easier to work with in the future?

[baileympearson]

If we use bash functions, we can then source this locally and test each component

[nbbeeken]

Is the green CI not testing enough? we can't proceed if any step in this script fails, which part is opaque, can I improve variable names?

[baileympearson]

All of the logic related to finding the correct version of Node and downloading it is a little confusing, and since there's no clear boundary as to when it starts and stops, it's hard to tell when you're reading the script when that portion of code is finished. This is why we use functions, to logically group related functionality and to make understanding the higher-level purpose and logic of a script easier to GROK at a glance

From a CI maintenance standpoint, ideally we never touch this code again. But if we do have to, I'd rather that it was logically group so that it's easier to reason about at the high level without reading and thoroughly understand the entire script.

And yes, you're right, a green CI means the script is passing. But if we ever have to touch this again, it would be nice if we could source the file and test it in a piecemeal fashion, as opposed to testing the script in its entirety or commenting out large chunks of code to test parts of it.

[nbbeeken]

from slack, we're aligned on the shortness of the script and since we plan to copy it to many places we're agreed that its fine as is, we may in the future improve this if there's a significant change needed to support a platform we don't currently predict here