bosco icon indicating copy to clipboard operation
bosco copied to clipboard

use nodeVersion, then .nvmrc and be semver about it

Open geophree opened this issue 9 years ago • 12 comments

Right now we just use nodeVersion from bosco-service.json and we aren't semver about what we use. We should allow nodeVersion to be semver and also fall back to .nvmrc when nodeVersion isn't defined. the nvm ls command may be helpful here, but it doesn't currently have machine-oriented output.

/cc @cressie176

geophree avatar Jan 08 '16 00:01 geophree

Agreed - but I think we should just make it use .nvmrc and remove the nodeVersion from bosco-service.json. Makes it consistent and less of a special case.

I assume the teams using newer versions of node are just using the same (latest?) node across all services, and then when packaging into the containers using specific ones? @tomruttle

cliftonc avatar Jan 08 '16 07:01 cliftonc

I filed this because @cressie176 ran into an issue when trying to run app-user-profile. His default node was something old and they didn't define a nodeVersion in the bosco-service.json (and would probably have been too specific if they had, it has to match an installed version exactly). They did have an .nvmrc though.

geophree avatar Jan 08 '16 07:01 geophree

I wish nvm respected nvmrc as it should. In Ruby land if you define a Ruby version via rvmrc or ruby-version and it will switch your version when you cd into the project and wont let you start if your version is not the correct one.

There have been talks to start node-version in node land but i dont think thats going to happen

gabceb avatar Jan 08 '16 15:01 gabceb

In the package.json for tes-service-es6-template we could prepend the npm run dev script with an nvm use && which will pick up the version of node required by that service from the .nvmrc file (if it exists) and run it? Not sure sustainable this would be, and how well it would work with multiple different services running different versions?

Also, FWIW, I tend to run the latest node stable on my mac (v5.4 at the time of writing) and simply add v5 to .nvmrc files when I write them. Though I see that we've pegged the .nvmrc file in tes-service-es6-template to v4.2 for some reason.

sometimeskind avatar Jan 08 '16 15:01 sometimeskind

I think this change is really small:

https://github.com/tes/bosco/blob/0497fddc6e84c97078807e3d59de7d859b40f817/src/RunWrappers/Node.js#L47

At the moment we get the node version (can be semver) from service.nodeVersion. We just need to read it from .nvmrc as well, and take one of them (say we prefer .nvmrc if it exists).

cliftonc avatar Jan 08 '16 15:01 cliftonc

The node version bosco uses has to be exact, otherwise it doesn't find the node executable because the path it creates is invalid. It needs to duplicate the logic used in nvm to find a maching installed node version before it builds the path to the node executable.

geophree avatar Jan 08 '16 21:01 geophree

We should also refuse to run it if your node version is lower than the one specified. I think a warning if it's higher is fine, because I imagine that will mostly be okay.

geophree avatar Jan 08 '16 21:01 geophree

@cliftonc With the most recent changes, we're quite a bit closer. I think our main problem now is going to be people running a project with .nvmrc of something like 4.2 when they have 4.4 installed and it complaining at them.

geophree avatar Mar 10 '16 22:03 geophree

Yeah, I'll see if I can be smarter about parsing the output of the nvm commands and having it warn the user vs just blow up.

We could have a 'rule' that says just use 4 or 5 in your .nvmrc, but the problem then is that arguably you have no idea what everyone is actually running the service on, so I do think we need to specify at least 4.3.

cliftonc avatar Mar 11 '16 07:03 cliftonc

Yeah, we really want "a 4 that's at least 4.3" e.g. ^4.2 in npm caret ranges: https://docs.npmjs.com/misc/semver#caret-ranges-123-025-004

geophree avatar Mar 11 '16 17:03 geophree

Looks like if we can give it a list of versions (a little parsing of nvm list), and pull the version from .nvmrc and throw a ^ on it we can use the semver package's maxSatisfying(versions, range) function. https://www.npmjs.com/package/semver#ranges-1

geophree avatar Mar 11 '16 17:03 geophree

no need to parse nvm list if you can reach out to https://nodejs.org/dist/index.json clean it with semver and then use the semver.maxSatisfying function against that list.

function getNodeDistVersions() {
  const https = require('https');
  return new Promise((resolve, reject) => {
    https.get('https://nodejs.org/dist/index.json', (res) => {
      let data = new Buffer('');
      res.on('data', (chunk) => {
        data = Buffer.concat([data, chunk]);
      });
      res.on('end', () => {
        try {
          let versions = JSON.parse(data);
          resolve(versions.map((v) => semver.clean(v.version)));
        } catch (e) {
          reject(e);
        }
      });
    }).on('error', reject);
  });
}

MerlinDMC avatar Jun 15 '16 08:06 MerlinDMC