-
Notifications
You must be signed in to change notification settings - Fork 593
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: add cataloger for NuGet packages #3484
base: main
Are you sure you want to change the base?
feat: add cataloger for NuGet packages #3484
Conversation
Signed-off-by: Kemosabert <[email protected]>
Signed-off-by: Kemosabert <[email protected]>
Signed-off-by: Kemosabert <[email protected]>
Signed-off-by: Kemosabert <[email protected]>
Signed-off-by: Kemosabert <[email protected]>
2fa3de5
to
bf60621
Compare
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 the PR @Kemosabert! I hope you don't mind I added some questions to get a better handle on the package lock specification.
for name, dep := range allDependencies { | ||
parentPkg, ok := pkgMap[name] | ||
if !ok { | ||
log.Debugf("unable to find package in map: %s", name) |
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 might need a better abstraction for this debug message to be helpful. Which map? What does the pkgMap represent? Are allDependencies only inclusive of dotNetPkgs
or can they be other types?
Apologies for all the question on this review. Is there a good reference or specification document I can look at for the lockfile so it's easier to maybe build a mental model of what we're cataloging here? That might aid in the review and help me understand if the relationships being created are correct.
From what I am reading the lockfile is:
dependencies[topPackages]map[subPackages]metadata
where topPackages and subPackages can be > 1
Is pkg map just inclusive of topPackages
?
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 went mainly off off this document here: https://github.com/NuGet/Home/wiki/Enable-repeatable-package-restore-using-lock-file and looking at lockfiles that I could generate myself with examples.
I updated the debug message and the logic as well. The test packages.lock.json file is one I generated using dotnet commands and includes a few examples of how it handles conflicting dependencies and versions.
Signed-off-by: Kemosabert <[email protected]>
Signed-off-by: Kemosabert <[email protected]>
@spiffcs would you mind to have another look at this when you have some time, happy to hear more suggestions/feedback 👍 |
Thanks @Kemosabert - I'm traveling this afternoon, but put this back at the top to add some commits for the SA/Unit tests that are failing. The changes look good and I appreciate the link to documentation showing what you followed for implementation! |
Description
This PR adds a cataloger for NuGet lockfiles in the form of packages.lock.json files.
Type of change
Checklist: