-
Notifications
You must be signed in to change notification settings - Fork 554
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
Parse javascript dependency trees #2142
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
@@ -13,6 +13,8 @@ func TestNpmPackageLockDirectory(t *testing.T) { | |||
sbom, _ := catalogDirectory(t, "test-fixtures/npm-lock") | |||
|
|||
foundPackages := internal.NewStringSet() | |||
// root pkg | |||
foundPackages.Add("npm-lock") |
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.
note: new top level root pkg being added
@@ -144,6 +144,8 @@ var dirOnlyTestCases = []testCase{ | |||
pkgType: pkg.NpmPkg, | |||
pkgLanguage: pkg.JavaScript, | |||
pkgInfo: map[string]string{ | |||
"yarn": "0.0.0", |
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.
note: new top level root pkg being added
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.
@noqcks Do you mind if I leave a couple comments?
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
@wagoodman I actually had some things to cleanup in this PR. I saw you just self-assigned. Mind if I push some commits over the next hour or so and then let you know when it's ready? |
have at it! (and just to be transparent we assign internally to track who's taking point on reviewing on the community github project) |
Signed-off-by: Benji Visser <[email protected]>
@wagoodman ready now |
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
Signed-off-by: Benji Visser <[email protected]>
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've got a rebased version of this branch locally that I'm reviewing -- I can push up my changes (might not be today though)
// NewJavaScriptCataloger returns a new JavaScript cataloger object based on detection | ||
// of npm based packages and lock files to provide a complete dependency graph of the | ||
// packages. | ||
func NewJavaScriptCataloger() *generic.GroupedCataloger { |
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 this would need to be reconciled with the NewLockCataloger()
above. At surface glance this cataloger would replace the lock cataloger.
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.
yep, that's true, was looking for guidance here on how to deprecate an existing cataloger on favor of a new cataloger and what the process would be for that
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.
@wagoodman any thoughts here? thanks!
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've just picked this back up for re-review today. I'm going to try and help get the PR rebased and working again, so I'll be pushing some commits. I'll get back to this question while I'm trying to square it with the current state on main.
Signed-off-by: Alex Goodman <[email protected]>
dont have the time to work on this anymore and get it over the line, but someone else can pick it up! will leave it in your hands @wagoodman |
implements the same functionality desired by #2020
This PR implements a new cataloger called javascript-cataloger that collects full dependency trees and packages with for javascript ecosystem pkg managers -- [pnpm, yarn, npm]
This allows one to see the relationships between packages such as with the dependencies field in CycloneDX https://cyclonedx.org/docs/1.5/json/#dependencies.
Note: all parsed package.json or lock files will now have a root package name and version. if
name
andversion
exist in the parsed file, then we use that, if no name or version exists, we use the dir name. This allows us to create a dep tree with a top level node.EDIT: Please hold off on reviewing this. I have some major updates to push.Ready for review.