-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(nodejs): add license parser to pnpm analyser #7036
Conversation
You can reference this page to pass tests. Please feel free to ask if you need assistance. |
Thanks for your contribution. LGTM. |
Thanks for your feedback! The tests are already consolidated into a minimal structure. The happy path test requires only two dependencies: Due to the nature of these dependencies, it might appear that we have several test data files. However, to keep things simple, everything except the |
I still feel like |
You're right. After giving it a second thought, I removed |
@DmitriyLewen Could you also take a look? |
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.
Hello @oscarbc96
Thanks for your work!
Add comments. Take a look please.
logger *log.Logger | ||
packageJsonParser *packagejson.Parser | ||
lockParser language.Parser | ||
comparer npm.Comparer |
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.
all package.json
files don't use version range.
We don't need comparer
to match version from package.json
and pnpm-lock.yaml
files.
packageJsonParser *packagejson.Parser | ||
lockParser language.Parser | ||
comparer npm.Comparer | ||
license *license.License |
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.
similar case.
package.json
file contain license name.
We don't need to use licenceClassifier
.
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.
do we need this link?
We skip this link in Required function.
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.
Nope, it's not necessary actually.
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.
remove it please.
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
cc. @knqyf263
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.
I think we should update an integration test similar to npm (adding node_modules
so licenses will be detected).
The test case is here.
trivy/integration/repo_test.go
Lines 105 to 112 in acbec05
{ | |
name: "pnpm", | |
args: args{ | |
scanner: types.VulnerabilityScanner, | |
input: "testdata/fixtures/repo/pnpm", | |
}, | |
golden: "testdata/pnpm.json.golden", | |
}, |
You can update the golden file with mage test:updateGolden
.
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.
You don't need to create a new gold file.
You need to update testdata/pnpm.json.golden
file.
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.
I'm not sure if I'm doing this right; the testdata/pnpm.json.golden
file is not updating. 🤔
I'm running mage test:updateGolden
. @DmitriyLewen Can you help me figure out what might be wrong?
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.
Sorry, i missed that list-all-pkgs
flag is not used for pnpm
testcase.
I added flag and updated golden file in 602b4d9
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.
Thank you!
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.
Thanks for your great contribution!
Co-authored-by: DmitriyLewen <[email protected]>
Description
pnpm-lock.yaml files do not contain dependency license information.
This PR parses */node_modules/<package_name>/package.json files alongside pnpm-lock.json and identify licenses.
Related Issues
Checklist