[PAN-11212] release/3.0.0 fix: get npm audit to pass - #125
Conversation
The only way to fix the vulnerabilities with better-docs was to get rid of it and use an alternative: the default JSDoc template
…ncies-v2' into PAN-11212-fix-vulnerable-dependencies-v3
Review Summary by QodoReplace request library with axios and fix npm audit vulnerabilities
WalkthroughsDescription• Replace vulnerable request library with axios for HTTP calls • Update Node.js requirement from 10+ to 18+ and upgrade dependencies • Remove better-docs dependency and use default JSDoc template • Refactor API and auth modules to use axios promise-based API • Update test mocks to work with axios instead of request library Diagramflowchart LR
A["request library<br/>vulnerable deps"] -->|replace| B["axios HTTP client"]
C["Node 10+ support"] -->|upgrade| D["Node 18+ requirement"]
E["better-docs template"] -->|remove| F["default JSDoc template"]
B --> G["API module refactored"]
B --> H["Auth module refactored"]
D --> I["Updated dependencies"]
I --> J["npm audit passes"]
File Changes1. lib/api.js
|
Code Review by Qodo
|
There was a problem hiding this comment.
Pull request overview
Updates the SDK’s HTTP stack and build toolchain to address npm audit findings by removing the deprecated request dependency and upgrading key packages, aligning the repo with a modern Node runtime baseline.
Changes:
- Replace
requestusage withaxiosin the auth and core API modules. - Upgrade runtime/tooling dependencies (axios, jest, eslint, jsdoc, etc.) and remove
request/ better-docs. - Bump the supported Node version to
>=18and update.nvmrcaccordingly.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/auth.js |
Migrates token retrieval from request to axios, preserving retry/error behavior. |
lib/api.js |
Migrates core API calls from request to axios and preserves callback signature (incl. request-like object). |
test/auth.test.js |
Updates auth URL unit tests to spy on axios instead of request. |
package.json |
Removes request, upgrades dependencies/tooling, and raises Node engine requirement to >=18. |
jsdoc.json |
Removes better-docs template configuration to match dependency removal. |
.nvmrc |
Updates pinned Node version to match the new engine baseline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ncies-v2' into PAN-11212-fix-vulnerable-dependencies-v3
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ncies-v2' into PAN-11212-fix-vulnerable-dependencies-v3
npm audit to passrelease/3.0.0 fix: get npm audit to pass
imlytvynenko
left a comment
There was a problem hiding this comment.
Thanks, it looks good to me.
|
Are tests failing? I see Buildkite failing? |
Tests are passing locally. Do you have access to Buildkite (to see why it's failing) @k163791? @21LukeOs @imlytvynenko and I don't, and my access request has been approved but not yet implemented. |
…tests Buildkite might have been failing because it used an older version of Node. We wanted to swap Buildkite for GitHub Actions anyway, and this was the easiest way to make it use a newer version of Node.
Examples are user-facing setup code, not library code. They shouldn't need to pass the same linting rules as the core library.
Please re-review this PR @k163791 @imlytvynenko . Buildkite might have been using a Docker image with an older version of Node and failing because of that. I've replaced the Buildkite pipeline with a GitHub Action used to run tests. This is something we want to do across the board anyway. At some point in time we can totally remove Buildkite from this repo and make the GitHub Action a required check to merge PRs. |
|
We cannot merge this until buildkite is passing, the buildkite page shows 404 to me. |
Buildkite is failing because we deleted the buildkite folder |
|
@DanielStevenLewis from the |
@k163791 Buildkite was failing before I deleted the Buildkite folder. |
@k163791 I believe we should remove Buildkite altogether and rely on the new GitHub action instead, but I believe this is something to do after we merge this PR. It's likely just a config change to finish off getting rid of Buildkite, after we revert 72c143d |
@k163791 do you mean we technically can't merge this PR unless Buildkite is passing, or that we shouldn't? I can, as I have privileges to do so. |
Yeah, but it is working in your other PR, which means that there was something in this PR that broke it, better to check what is happening there before merging it. |
I believe we shouldn't better to get access to this buildkite project so we can see exactly what is failing, then we can decide if it's ok to merge it or not with buildkite failing. |
OK, I've marked https://rewardops.atlassian.net/browse/PAN-11212 as blocked by me being granted Buildkite access (in https://rewardops.atlassian.net/browse/SD-18769) @kcearns-ro @k163791 based on the Docker image's naming conventions, and based on me likely having found the Dockerfile used to produce that image (I'm guessing it's an older version of https://github.com/rewardops/docker_pangea_ember_ci/blob/master/Dockerfile), I think it's very highly likely that the image Buildkite uses has an old version of Node, and that would cause Buildkite to fail for this PR (which requires a newer version of Node), but I can't know with absolute certainty without access to see the failures in Buildkite. |
If we can confirm that it's only because of the node version after getting access, we can go ahead and remove Buildkite from this repo all together. |
I got Buildkite access. I believe Buildkite failed because it's using an older version of Node @k163791 . I've copied and pasted the logs below. Here are the relevant parts of Copilot's response to my question about it.
Here are the Buildkite logs: |
This reverts commit 72c143d.
…rable-dependencies-v3


PAN-11212
This PR uses #124 as a starting point.
This change is a major version upgrade because it requires higher versions of Node.
This PR also replaces Buildkite with GitHub Actions as the continuous integration system to run tests.