-
Notifications
You must be signed in to change notification settings - Fork 485
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
✨ Support Nuget Pinned Dependency with RestoreLockedMode attribute #4351
base: main
Are you sure you want to change the base?
Conversation
Support pinning dependency in .NET using lockfile by declaring the RestoreLockedMode attribute in csproj Co-authored-by: Liam Moat <[email protected]> Co-authored-by: Ioana A <[email protected]> Co-authored-by: Mélanie Guittet <[email protected]> Signed-off-by: balteraivshay <[email protected]>
Signed-off-by: balteraivshay <[email protected]>
Signed-off-by: balteraivshay <[email protected]>
7d30eeb
to
c50d2b8
Compare
Signed-off-by: balteraivshay <[email protected]>
Signed-off-by: balteraivshay <[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.
Not a full review, just wanted to tackle a question around the approach.
if isStagedNugetDepsUnpinned(&results) { | ||
if err := collectInsecureNugetCsproj(c, &results); err != nil { | ||
return checker.PinningDependenciesData{}, err | ||
} | ||
} else { | ||
promoteStagedNugetDependencies(&results, false) | ||
} |
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 to have separate slices for staged vs. promoted deps? Can we just in-place mark nuget deps as pinned?
The code would roughly be updated to: (dont have my IDE in front of me so might have some syntax errors in here).
if hasUnpinnedNuget(&results) && lockedProj(/* args */) {
markNugetPinned(&results)
}
func markNugetPinned(r *results) {
for i := range r.Dependencies {
if isNuget(r.Dependencies[i]) {
r.Dependencies[i].Pinned = asBoolPointer(true)
r.Dependencies[i].Remediation = nil
}
}
}
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.
We considered having these on the same slice and your code makes sense for updating found nuget dependencies.
However, when we would need to remove nuget dependencies from the Dependencies slice and replacing them with new ones that are found during the post processing stage, the implementation seemed less clean.
an example for that is when some of the csproj files in a repo enable lock mode and some do not, we will not use the dependencies found during shell_validate_download, and instead build new ones at the post processing phase and stage them onto the Dependencies slice.
what are your thoughts?
Signed-off-by: balteraivshay <[email protected]>
Signed-off-by: balteraivshay <[email protected]>
What kind of change does this PR introduce?
Support pinning dependency in .NET using lockfile by declaring the RestoreLockedMode attribute in csproj
What is the current behavior?
checking for nuget pinned dependency attributes only CLI locked mode flags (i.e. --locked-mode) and if not found the score is 0.
What is the new behavior (if this is a feature change)?**
if nuget cli command found that is not locked with the CLI flag, a post-processing method checks for csproj files in the repo and scores according to the number of them that are declaring RestoreLockedMode attribute set to true.
Which issue(s) this PR fixes
Fixes #4251
Special notes for your reviewer
This implementation for this fix, using post processing method, was discussed with @spencerschrock in the Scorecard community call.
There is an upcoming PR that will compliment the behaviour by adding support for Central Package Management that will also be performing post processing to look for another file type (Directory.Packages.props) where this feature is enabled.
Does this PR introduce a user-facing change?
For user-facing changes, please add a concise, human-readable release note to
the
release-note
(In particular, describe what changes users might need to make in their
application as a result of this pull request.)