fix: vendor node-foreman to resolve Node.js deprecation warning#3742
Open
schneems wants to merge 7 commits into
Open
fix: vendor node-foreman to resolve Node.js deprecation warning#3742schneems wants to merge 7 commits into
schneems wants to merge 7 commits into
Conversation
schneems
changed the title
schneems
force-pushed
the
schneems/foreman-fork
branch
from
860dccf to
112253f
Compare
schneems
force-pushed
the
schneems/foreman-fork
branch
2 times, most recently
from
0ba6dcb to
36121b8
Compare
schneems
had a problem deploying
to
AcceptanceTests
GitHub Actions
Failure
schneems
force-pushed
the
schneems/foreman-fork
branch
from
36121b8 to
7103ba5
Compare
schneems
had a problem deploying
to
AcceptanceTests
GitHub Actions
Failure
Copy the 9 library files from node-foreman v3.0.1 (MIT license) into src/lib/local/foreman/ with .cjs extensions and updated internal require paths for the .cjs extension. No other changes to the upstream source. Refs #3736
Update run-foreman.cjs to import from ./foreman/*.cjs instead of foreman/lib/*. Hardcode the foreman version (3.0.1) instead of reading it from the package. Remove the foreman npm dependency from package.json and update the tsconfig.json comment to reflect the vendored state. Add eslint and cspell ignore patterns for the vendored foreman directory. Refs #3736
The deprecated `util._extend` API in foreman's envs module triggers a Node.js DEP0060 deprecation warning when running `heroku local`. Replace it with `Object.assign` and remove the unused `util` require. Fixes #3736
Cover keyValue parsing (basic, quoted, comments, equals in values), flattenJSON behavior, and loadEnvs with single and comma-separated env files. The multi-file merge test exercises the Object.assign replacement for the former util._extend call. Refs #3736
The exporter.cjs depends on mustache, causing an error in CI:
```
node:internal/modules/cjs/loader:1210
throw err;
^
Error: Cannot find module 'mustache'
Require stack:
- /home/runner/work/cli/cli/dist/lib/local/foreman/exporters.cjs
- /home/runner/work/cli/cli/dist/lib/local/run-foreman.cjs
at Module._resolveFilename (node:internal/modules/cjs/loader:1207:15)
at Module._load (node:internal/modules/cjs/loader:1038:27)
at Module.require (node:internal/modules/cjs/loader:1289:19)
at require (node:internal/modules/helpers:182:18)
at Object.<anonymous> (/home/runner/work/cli/cli/dist/lib/local/foreman/exporters.cjs:11:10)
at Module._compile (node:internal/modules/cjs/loader:1521:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1623:10)
at Module.load (node:internal/modules/cjs/loader:1266:32)
at Module._load (node:internal/modules/cjs/loader:1091:12)
at Module.require (node:internal/modules/cjs/loader:1289:19) {
code: 'MODULE_NOT_FOUND',
requireStack: [
'/home/runner/work/cli/cli/dist/lib/local/foreman/exporters.cjs',
'/home/runner/work/cli/cli/dist/lib/local/run-foreman.cjs'
]
}
```
The `heroku local` command does not use `export` therefore it doesn't need this module, so we can remove it and the need to have mustache.
These directives were left over from the vendored node-foreman code but the rules they suppress are never triggered, causing eslint to report unused-directive errors and fail CI. This was causing errors in CI ``` npx eslint src/lib/local/run-foreman.cjs 2>&1 /Users/rschneeman/Documents/projects/work/tmp/cli/src/lib/local/run-foreman.cjs 17:1 error Unused eslint-disable directive (no problems were reported from 'no-new') 18:1 error Unused eslint-disable directive (no problems were reported from 'radix') ✖ 2 problems (2 errors, 0 warnings) 2 errors and 0 warnings potentially fixable with the `--fix` option. ```
schneems
force-pushed
the
schneems/foreman-fork
branch
from
87a789a to
65be92e
Compare
Contributor
Author
|
While I cleaned up a little of the unused code, there's more that can be reduced/removed in the future. I went down that path but found that there are some codepaths that aren't exercised/tested via CI, and didn't feel comfortable continuing. Vendoring the library as-is and making minimal cleanup makes this easier to review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove warning from loading foreman by vendoring the library and fixing the offending code.
close #3736.
Type of Change
Breaking Changes (major semver update)
!after your change type to denote a change that breaks current behaviorFeature Additions (minor semver update)
Patch Updates (patch semver update)
Testing
Notes:
Steps:
Screenshots (if applicable)
Related Issues
GitHub issue: #3736
GUS work item: GUS-W-15785346