Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make verify command consider default nuget.config file and nuget.config option #4396
Make verify command consider default nuget.config file and nuget.config option #4396
Changes from 7 commits
25d6041
e7dccd3
4563b0e
dd186a7
2b4fd91
c37f575
328ef60
fad5bb9
f5f0313
b96d082
87846fd
0e2dd8b
1352e50
0d243bd
c26dade
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 don't love this warning message, but I imagine this PR is just copying the string from NuGet.packaging?
In particular, the
so
conjunction makes it too informal imo, but I'd love to see if others think we should change it (can be done independently too).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.
Yes, I copied this text from
NuGet.Packing
. Sure, I'm open for any suggestion or I can address later independently.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 created follow up issue NuGet/Home#11518
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.
Use explicit type instead of
var
.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.
Sure, addressed.
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.
Previously, I recommended using
TrustedSignersProvider.GetAllowListEntries(...)
directly (instead ofClientPolicyContext.GetClientPolicy(...)
). My reasoning was thatClientPolicyContext.GetClientPolicy(...)
already does 2 things:TrustedSignersProvider.GetAllowListEntries(...)
....and that you only need # 2. However, with your latest changes, we're now setting signature validation mode --- which is fine, provided that we have good test coverage.
Make sure you delete this statement and replace
trustedSignerAllowList
withclientPolicyContext.AllowList
.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.
Addressed.
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 changed lines little bit make it more comparable to similar code in PackageRestore.
NuGet.Client/src/NuGet.Core/NuGet.Packaging/PackageExtractor.cs
Lines 998 to 1011 in b8fe2a0
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.
1 level of indentation is 4 spaces.
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.
Addressed
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.
For verify action repository signing allow list not considered.
NuGet.Client/src/NuGet.Core/NuGet.Packaging/PackageExtractor.cs
Lines 1019 to 1029 in b8fe2a0