-
Notifications
You must be signed in to change notification settings - Fork 11
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
chore: remove dependency shelljs #61
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like @mdjermanovic to review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. I also tested it with eslint/eslint project.
One suggestion about the tests.
tests/lib/release-ops.js
Outdated
|
||
beforeEach(() => { | ||
sandbox = sinon.sandbox.create(); | ||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "writeChangelog-")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to remove this directory in afterEach
so that running the tests doesn't leave temporary directories behind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in fc24c33.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This PR removes the dependency shelljs and replaces its usages with native Node.js API calls. It also adds unit tests to ensure that the behavior of
writeChangelog()
hasn't changed.ShellJS in versions prior to 0.8.5 has a high rated vulnerability: https://nvd.nist.gov/vuln/detail/CVE-2022-0144.