Adding support for OSD_NODE_HOME

[AbhishekReddy1127]

Signed-off-by: AbhishekReddy1127 [email protected]

Description

Added support for OSD_NODE_HOME for linux.
Tests are not included because we will have to set and unset env variable from node for testing this change.
Screenshot

Issues Resolved

#1219

Check List

• New functionality includes testing.
  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
• New functionality has been documented.
• Commits are signed per the DCO using --signoff

[codecov-commenter]

Codecov Report

Merging #2208 (300a715) into main (24b1a78) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2208      +/-   ##
==========================================
- Coverage   66.56%   66.56%   -0.01%     
==========================================
  Files        3203     3203              
  Lines       61325    61329       +4     
  Branches     9453     9453              
==========================================
+ Hits        40821    40823       +2     
- Misses      18249    18251       +2     
  Partials     2255     2255              

Flag	Coverage Δ
Linux	66.50% <0.00%> (-0.01%)	⬇️
Windows	66.50% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/setup_node_env/node_version_validator.js	53.33% <0.00%> (-19.40%)	⬇️
...ic/application/models/sense_editor/sense_editor.ts	64.88% <0.00%> (-0.89%)	⬇️
packages/osd-optimizer/src/node/cache.ts	53.94% <0.00%> (+2.63%)	⬆️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts	87.75% <0.00%> (+4.08%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[kavilla]

@KrooshalUX does this wording make sense?

[joshuarrrr]

How about:

Suggested change

	'\nOpenSearch Dashboards is running as OSD_NODE_HOME environment variable is set, ' +
	'\nBecause the OSD_NODE_HOME environment variable is set, any node version incompatibilities will be ignored.'

[kavilla]

Looking good! Just a couple of comments!

[seanneumann]

Can I get an update?

@kgcreative - Need some UX eyes for the warning.

@kavilla @dblock - Does OpenSearch support major version differences when this variable is set?

[kavilla]

I would like to get this change in but I think it might cause some conflicts with #2091.

Do we think we want to move forward and try to handle conflicts with the other PR? @joshuarrrr

[joshuarrrr]

Yeah, I think maintainers can help mitigate and resolve any conflicts regardless of which PR goes in first.

[ananzh]

@AbhishekReddy1127 I think you are very close to finish this PR. Couple of things left.

• I would also recommend to update the warning msg as suggested by @joshuarrrr.

'\nBecause the OSD_NODE_HOME environment variable is set, any node version incompatibilities will be ignored.'

• Rebase the PR and add it to the CHANGELOG
• Fix the unit test

The current unit test you have in this PR does not cover the case when OSD_NODE_HOME is set in the process.env. As you mentioned, the process is passed through executing this command line.

var command = `node -e "${processVersionOverwrite}require('./node_version_validator.js')"`;

Currently processVersionOverwrite only modifies the version but it hints a way to change the process. We could either modify it to use Object.defineProperties which could modify multiple properties or change the command directly.

I have a reference test file below including two test cases, the original one and the one with OSD_NODE_HOME. I modify the command in the testValidateNodeVersion function directly.

 if (osdNodeHome) {
    processOverwrite += `process.env.OSD_NODE_HOME = '${osdNodeHome}';`;
  }

Meanwhile I add osdNodeHome as an optional parameter. If a value is set then it will go to check the warning msg. Well, there are multiple ways to write the functions depending on ur preference. For example, osdNodeHome could be set as a boolean.

Here are the test details:

var exec = require('child_process').exec;
var pkg = require('../../package.json');
var REQUIRED_NODE_JS_VERSION = 'v' + pkg.engines.node;

describe('NodeVersionValidator without OSD_NODE_HOME defined in the process ', function () {
  it('should run the script WITHOUT error when the version is the same', function (done) {
    testValidateNodeVersion(done, REQUIRED_NODE_JS_VERSION);
  });

  it('should run the script WITHOUT error when only the patch version is higher', function (done) {
    testValidateNodeVersion(done, requiredNodeVersionWithDiff(0, 0, +1));
  });

  it('should run the script WITH error if the patch version is lower', function (done) {
    var lowerPatchversion = requiredNodeVersionWithDiff(0, 0, -1);
    testValidateNodeVersion(
      done,
      lowerPatchversion,
      REQUIRED_NODE_JS_VERSION !== lowerPatchversion
    );
  });

  it('should run the script WITH error if the major version is higher', function (done) {
    testValidateNodeVersion(done, requiredNodeVersionWithDiff(+1, 0, 0), true);
  });

  it('should run the script WITH error if the major version is lower', function (done) {
    var lowerMajorVersion = requiredNodeVersionWithDiff(-1, 0, 0);
    testValidateNodeVersion(
      done,
      lowerMajorVersion,
      REQUIRED_NODE_JS_VERSION !== lowerMajorVersion
    );
  });

  it('should run the script WITH error if the minor version is higher', function (done) {
    testValidateNodeVersion(done, requiredNodeVersionWithDiff(0, +1, 0), true);
  });

  it('should run the script WITH error if the minor version is lower', function (done) {
    var lowerMinorVersion = requiredNodeVersionWithDiff(0, -1, 0);
    testValidateNodeVersion(
      done,
      lowerMinorVersion,
      REQUIRED_NODE_JS_VERSION !== lowerMinorVersion
    );
  });
});

describe('NodeVersionValidator with OSD_NODE_HOME defined in the process ', function () {
  it('should run the script WITHOUT warning when the version is the same', function (done) {
    testValidateNodeVersion(done, REQUIRED_NODE_JS_VERSION, false, 'v14.0.0');
  });

  it('should run the script WITHOUT warning when only the patch version is higher', function (done) {
    testValidateNodeVersion(done, requiredNodeVersionWithDiff(0, 0, +1), false, 'v14.0.0');
  });

  it('should run the script WITH warning if the patch version is lower', function (done) {
    var lowerPatchversion = requiredNodeVersionWithDiff(0, 0, -1);
    testValidateNodeVersion(
      done,
      lowerPatchversion,
      REQUIRED_NODE_JS_VERSION !== lowerPatchversion,
      'v14.0.0'
    );
  });

  it('should run the script WITH warning if the major version is higher', function (done) {
    testValidateNodeVersion(done, requiredNodeVersionWithDiff(+1, 0, 0), true, 'v14.0.0');
  });

  it('should run the script WITH warning if the major version is lower', function (done) {
    var lowerMajorVersion = requiredNodeVersionWithDiff(-1, 0, 0);
    testValidateNodeVersion(
      done,
      lowerMajorVersion,
      REQUIRED_NODE_JS_VERSION !== lowerMajorVersion,
      'v14.0.0'
    );
  });

  it('should run the script WITH warning if the minor version is higher', function (done) {
    testValidateNodeVersion(done, requiredNodeVersionWithDiff(0, +1, 0), true, 'v14.0.0');
  });

  it('should run the script WITH warning if the minor version is lower', function (done) {
    var lowerMinorVersion = requiredNodeVersionWithDiff(0, -1, 0);
    testValidateNodeVersion(
      done,
      lowerMinorVersion,
      REQUIRED_NODE_JS_VERSION !== lowerMinorVersion,
      'v14.0.0'
    );
  });
});

function requiredNodeVersionWithDiff(majorDiff, minorDiff, patchDiff) {
  var matches = REQUIRED_NODE_JS_VERSION.match(/^v(\d+)\.(\d+)\.(\d+)/);
  var major = Math.max(parseInt(matches[1], 10) + majorDiff, 0);
  var minor = Math.max(parseInt(matches[2], 10) + minorDiff, 0);
  var patch = Math.max(parseInt(matches[3], 10) + patchDiff, 0);

  return `v${major}.${minor}.${patch}`;
}

function testValidateNodeVersion(
  done,
  versionToTest,
  expectErrorOrWarning = false,
  osdNodeHome = ''
) {
  var processOverwrite = `Object.defineProperty(process, 'version', { value: '${versionToTest}', writable: true });`;
  if (osdNodeHome) {
    processOverwrite += `process.env.OSD_NODE_HOME = '${osdNodeHome}';`;
  }
  var command = `node -e "${processOverwrite}require('./node_version_validator.js')"`;

  exec(command, { cwd: __dirname }, function (error, _stdout, stderr) {
    expect(stderr).toBeDefined();
    var specificErrorOrWarningMessage = `OpenSearch Dashboards was built with ${REQUIRED_NODE_JS_VERSION} and does not support the current Node.js version ${versionToTest}. `;

    if (expectErrorOrWarning) {
      if (!osdNodeHome) {
        expect(error.code).toBe(1);
        // Actions to apply when validation fails: error report + exit.
        specificErrorOrWarningMessage += `Please use Node.js ${REQUIRED_NODE_JS_VERSION} or a higher patch version.\n`;
      } else {
        specificErrorOrWarningMessage +=
          '\nBecause the OSD_NODE_HOME environment variable is set, any node version incompatibilities will be ignored.\n';
      }
      expect(stderr).toStrictEqual(specificErrorOrWarningMessage);
    } else {
      expect(error).toBeNull();
      expect(stderr).toHaveLength(0);
    }
    done();
  });
}

Could you pls update this PR with the above 3 points? Thank you.

[joshuarrrr]

Looks good! Only thing left: where are we planning to document this env variable? It needs to be discoverable somewhere other than the CHANGELOG for it to be useful.

[joshuarrrr]

@AbhishekReddy1127 It also looks like the DCO check is failing - if necessary, rebase or squash locally to make sure all your commits are signed.

[joshuarrrr]

I think the only thing this PR is missing is documentation of the env variable. @kavilla @dblock Any thoughts on where this alternative option should be included?

[AMoo-Miki]

This would need to be redone if #3402 is merged.

[AMoo-Miki]

Thinking a bit about this

1. I am not convinced that Dashboards should allow circumvention of the Node.js version guardrails.
2. OSD_NODE_HOME should point to the parent of bin/node; its usage would be NODE="${OSD_NODE_HOME}/bin/none"
3. All the files under src/dev/build/tasks/bin/scripts, batch or shell, will need to honor this.
4. These files should also honor the common-place, but non-standard, NODE_HOME variable.

[kavilla]

Thinking a bit about this

1. I am not convinced that Dashboards should allow circumvention of the Node.js version guardrails.

How come? This will reflect what OpenSearch has with all the responsibility being placed on the host to set the location of java home. I need to verify that this is how it works vs just utilized for location. I believe there is also logic that if there is no node bundled in the distribution then OSD will attempt to honor the global node.

[AMoo-Miki]

How come? This will reflect what OpenSearch has with all the responsibility being placed on the host to set the location of java home. I need to verify that this is how it works vs just utilized for location.

I don't remember seeing OpenSearch locking down their runtime versions to a specific version or range; I think I remember being able to run OpenSearch with JRE 11, 14, and 17. Looking at their code, I don't see OPENSEARCH_JAVA_HOME bypassing any limits either.

Furthermore, if we ever decide to allow bypassing the version limitation on Dashboards, we would just add a CLI param; an env variable would be tough to discover while documentation for CLI params are available right there via --help.

Additionally, any *_HOME variable is meant to help shell scripts find the right runtime binaries; adding functionality beyond the shell scripts (aka in our code) is unexpected.

I believe there is also logic that if there is no node bundled in the distribution then OSD will attempt to honor the global node.

Yep and if that global version doesn't satisfy the requirements of Dashboards, I would expect it to fail.

[joshuarrrr]

Converted to draft and removed version labels until we can agree on a path forward.

[AMoo-Miki]

This is superseeded by #3508