Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix NuGetAudit severity level mapping (#5313) #5315

Closed
wants to merge 1 commit into from
Closed

Fix NuGetAudit severity level mapping (#5313) #5315

wants to merge 1 commit into from

Conversation

zivkan
Copy link
Member

@zivkan zivkan commented Jul 17, 2023

Bug

Fixes: NuGet/Home#12743

Regression? No, no feature

Description

Cherry picks #5313 onto release-6.7.x branch.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@zivkan zivkan requested a review from a team as a code owner July 17, 2023 07:43
aortiz-msft
aortiz-msft reviewed Jul 17, 2023
View reviewed changes
src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs
@@ -368,31 +369,31 @@ private int ParseAuditLevel()

if (auditLevel == null)
{
return 1;
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 0;

Could we add a comment on when auditLevel is expected to be null? I'm guessing this is the default case, so it would be good to comment that and also comment why the default case was decided to be equivalent to low.

aortiz-msft
aortiz-msft reviewed Jul 17, 2023
View reviewed changes
src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs
@@ -368,31 +369,31 @@ private int ParseAuditLevel()

if (auditLevel == null)
{
return 1;
return 0;
}

if (string.Equals("low", auditLevel, StringComparison.OrdinalIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

I'm a bit worried that these are not constants defined somewhere. A typo in a later change could make the strings to be mismatched when written/read from the properties.

@zivkan zivkan mentioned this pull request Jul 19, 2023
Merged
8 tasks
@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 24, 2023
@ghost
Copy link

ghost commented Jul 24, 2023

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@zivkan zivkan closed this Jul 25, 2023
@nkolev92 nkolev92 deleted the dev-zivkan-6.7-NuGetAudit-severity branch July 25, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aortiz-msft aortiz-msft aortiz-msft left review comments

Assignees
No one assigned
Labels
Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zivkan @aortiz-msft