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

[area/nodejs] Take engines property into account when engine-strict appear in .npmrc file #9249

Merged
merged 4 commits into from Mar 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG_PENDING.md
Expand Up @@ -18,6 +18,9 @@
- [cli/plugins] - Improved error message for missing plugins.
[#5208](https://github.com/pulumi/pulumi/pull/5208)

- [sdk/nodejs] - Take engines property into account when engine-strict appear in npmrc file
[#9249](https://github.com/pulumi/pulumi/pull/9249)

### Bug Fixes

- [sdk/nodejs] - Fix uncaught error "ENOENT: no such file or directory" when an error occurs during the stack up.
Expand Down
49 changes: 48 additions & 1 deletion sdk/nodejs/cmd/run/run.ts
Expand Up @@ -17,6 +17,8 @@ import * as url from "url";
import * as minimist from "minimist";
import * as path from "path";
import * as tsnode from "ts-node";
import * as ini from "ini";
import * as semver from "semver";
import { parseConfigFileTextToJson } from "typescript";
import { ResourceError, RunError } from "../../errors";
import * as log from "../../log";
Expand Down Expand Up @@ -59,6 +61,28 @@ function packageObjectFromProjectRoot(projectRoot: string): Record<string, any>
}
}

// Reads and parses the contents of .npmrc file if it exists under the project root
// This assumes that .npmrc is a sibling to package.json
function npmRcFromProjectRoot(projectRoot: string): Record<string, any> {
const emptyConfig = {};
try {
const npmRcPath = path.join(projectRoot, ".npmrc");
if (!fs.existsSync(npmRcPath)) {
return emptyConfig;
}
// file .npmrc exists, read its contents
const npmRc = fs.readFileSync(npmRcPath, "utf-8");
// Use ini to parse the contents of the .npmrc file
// This is what node does as described in the npm docs
// https://docs.npmjs.com/cli/v8/configuring-npm/npmrc#comments
return ini.parse(npmRc);
} catch {
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we should just allow this error to bubble up. It's probably something like bad permissions and having that explict error might be better than silently ignoring if engine-strict is set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the same approach as with packageObjectFromProjectRoot where we just return an empty object if we are unable to read or parse it. Do you think it is likely that an error would occur when we are only reading the file?

Copy link
Member

Choose a reason for hiding this comment

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

Only errors I can think likely are permission issues. It's probably rare enough to not really worry either way.

Copy link
Member

Choose a reason for hiding this comment

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

Would we have a way of raising a warning here instead - to help the user if it is some kind of permissions issue?

// .npmrc file exists but we couldn't read or parse it
// user out of luck here
return emptyConfig;
}
}

function throwOrPrintModuleLoadError(program: string, error: Error): void {
// error is guaranteed to be a Node module load error. Node emits a very
// specific string in its error message for module load errors, which includes
Expand Down Expand Up @@ -262,7 +286,8 @@ ${defaultMessage}`);
// Now go ahead and execute the code. The process will remain alive until the message loop empties.
log.debug(`Running program '${program}' in pwd '${process.cwd()}' w/ args: ${programArgs}`);
try {
const packageObject = packageObjectFromProjectRoot(projectRootFromProgramPath(program));
const projectRoot = projectRootFromProgramPath(program);
const packageObject = packageObjectFromProjectRoot(projectRoot);

let programExport: any;

Expand Down Expand Up @@ -296,6 +321,28 @@ ${defaultMessage}`);
programExport = require(program);
}

// Check compatible engines before running the program:
const npmRc = npmRcFromProjectRoot(projectRoot);
if (npmRc["engine-strict"] && packageObject.engines && packageObject.engines.node) {
// found:
// - { engines: { node: "<version>" } } in package.json
// - engine-strict=true in .npmrc
//
// Check that current node version satistfies the required version
const requiredNodeVersion = packageObject.engines.node;
const currentNodeVersion = process.versions.node;
if (!semver.satisfies(currentNodeVersion, requiredNodeVersion)) {
const errorMessage = [
`Your current Node version is incompatible to run ${projectRoot}`,
`Expected version: ${requiredNodeVersion} as found in package.json > engines > node`,
`Actual Node version: ${currentNodeVersion}`,
`To fix issue, install a Node version that is compatible with ${requiredNodeVersion}`,
];

throw new Error(errorMessage.join("\n"));
}
}

// If the exported value was itself a Function, then just execute it. This allows for
// exported top level async functions that pulumi programs can live in. Finally, await
// the value we get back. That way, if it is async and throws an exception, we properly
Expand Down
10 changes: 6 additions & 4 deletions sdk/nodejs/package.json
Expand Up @@ -13,6 +13,7 @@
"@logdna/tail-file": "^2.0.6",
"@pulumi/query": "^0.3.0",
"google-protobuf": "^3.5.0",
"ini": "^2.0.0",
"js-yaml": "^3.14.0",
"minimist": "^1.2.0",
"normalize-package-data": "^2.4.0",
Expand All @@ -26,6 +27,7 @@
"upath": "^1.1.0"
},
"devDependencies": {
"@types/ini": "^1.3.31",
"@types/js-yaml": "^3.12.5",
"@types/minimist": "^1.2.0",
"@types/mocha": "^2.2.42",
Expand All @@ -36,12 +38,12 @@
"@types/split2": "^2.1.6",
"@typescript-eslint/eslint-plugin": "^4.29.0",
"@typescript-eslint/parser": "^4.29.0",
"mockpackage": "file:tests/mockpackage",
"eslint": "^7.32.0",
"eslint-plugin-header": "^3.1.1",
"eslint-plugin-import": "^2.23.4",
"nyc": "^15.1.0",
"mocha": "^9.0.0"
"mocha": "^9.0.0",
"mockpackage": "file:tests/mockpackage",
"nyc": "^15.1.0"
},
"pulumi": {
"comment": "Do not remove. Marks this as as a deployment-time-only package"
Expand All @@ -53,6 +55,6 @@
"require": ["ts-node/register", "source-map-support/register"]
},
"//": [
"NOTE: @types/node is pinned due to grpc/grpc-node#2002"
"NOTE: @types/node is pinned due to grpc/grpc-node#2002"
]
}