return counts from RestoreTask for PackageReference projects #6049
Conversation
| @@ -51,6 +51,9 @@ internal class AuditUtility | |||
| internal int DistinctAdvisoriesSuppressedCount { get; private set; } | |||
| internal int TotalWarningsSuppressedCount { get; private set; } | |||
|
|
|||
| /// <inheritdoc cref="RestoreSummary.AuditRan"/> | |||
| internal bool AuditRan { get; private set; } | |||
There was a problem hiding this comment.
I would just have CheckPackageVulnerabilitiesAsync() return Task<bool> and then save the value here to be returned. Just my vote
There was a problem hiding this comment.
I agree, it's pretty much done that way anyways in the 3 places we return.
| @@ -51,6 +51,9 @@ internal class AuditUtility | |||
| internal int DistinctAdvisoriesSuppressedCount { get; private set; } | |||
| internal int TotalWarningsSuppressedCount { get; private set; } | |||
|
|
|||
| /// <inheritdoc cref="RestoreSummary.AuditRan"/> | |||
| internal bool AuditRan { get; private set; } | |||
There was a problem hiding this comment.
I agree, it's pretty much done that way anyways in the 3 places we return.
| @@ -138,7 +138,8 @@ static bool HasProjectToRestore(DependencyGraphSpec dgSpec, bool restorePackages | |||
|
|
|||
| try | |||
| { | |||
| bool result = await BuildTasksUtility.RestoreAsync( | |||
| // todo: need to return Restore task output properties, like in NuGet.targets | |||
There was a problem hiding this comment.
TODO's need to be linked to issues.
Sorry I missed this in the first review.
zivkan
force-pushed
the
dev-zivkan-did-audit-run
branch
from
115008f
to
ae0b847
Compare
| /// <summary> | ||
| /// A boolean that specifies if NuGetAudit verified packages for known vulnerabilities | ||
| /// </summary> | ||
| /// <remarks>This could be false if NuGetAudit is disabled, if |
There was a problem hiding this comment.
| /// <remarks>This could be false if NuGetAudit is disabled, if | |
| /// <remarks>This could be false if: |
There was a problem hiding this comment.
I'll try to remember to fix this when I implement the static graph restore counts
| @@ -289,7 +289,7 @@ public static async Task<bool> RestoreAsync( | |||
| { | |||
| RestoreSummary.Log(log, restoreSummaries); | |||
| } | |||
| return restoreSummaries.All(x => x.Success); | |||
| return restoreSummaries; | |||
There was a problem hiding this comment.
Is this fixing other bugs with restore around logging what specifically failed during a failed restore? Before, if any summary indicated failure, then the entire collection wasn't returned, therefore client couldn't tell the customer any details of what failed.
There was a problem hiding this comment.
No, it's still expected that any project that does not have a successful restore to have logged its errors.
| // Second restore is no-op, so should be skipped, and audit not run | ||
| _dotnetFixture.RunDotnetExpectSuccess(pathContext.SolutionRoot, "restore -p:ExpectedRestoreProjectCount=1 -p:ExpectedRestoreSkippedCount=1 -p:ExpectedRestoreProjectsAuditedCount=0"); | ||
|
|
||
| mockServer.Stop(); |
There was a problem hiding this comment.
Dispose already does this.
| } | ||
|
|
||
| [Fact] | ||
| public async Task DotnetRestore_WithServerWithoutAuditData_RestoreTaskReturnsCountProperties() |
There was a problem hiding this comment.
I didn't follow this test name well, but reading the test, my interpretation is it's expecting 1 restore, then 1 no-op restore. In neither case is audit count expected to tick.
There was a problem hiding this comment.
Yes, you're right. It does two restores, the second is a no-op. No-op (should) never run NuGetAudit, and the first restore does not run audit because the server did not provide a vulnerability database ("audit data").
I couldn't think of a way to write this in a name without it being unreasonably long.
Bug
Fixes: NuGet/Home#13778
Description
Add properties output properties
RestoreProjectCount,RestoreSkippedCount, andRestoreProjectsAuditedCountto RestoreTask (non-static graph version). Static graph version will be implemented later, and including packages.config projects is waiting for customer feedback.The tests demonstrate how to to fail a restore if NuGetAudit was not enabled for at least 1 project.
PR Checklist