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.

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

Duplicate NuGetAuditSuppress warnings use code NU1508 #5921

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

zivkan
Copy link
Member

@zivkan zivkan commented Jul 14, 2024

Bug

Fixes: NuGet/Home#13620

Description

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc. Add docs for NU1508 Home#13621

@zivkan zivkan requested a review from a team as a code owner July 14, 2024 22:49
Nigusu-Allehu
Nigusu-Allehu previously approved these changes Jul 14, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

@nkolev92 why does NuGet.exe disable duplicate checking?

AddProperty(args, "DisableCheckingDuplicateNuGetItems", bool.TrueString);

Copy link
Member

@nkolev92 nkolev92 Jul 15, 2024

Choose a reason for hiding this comment

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

Because TreatWarningsAsErrors would fail the dg spec generation and fail the operation with a message that's not very actionable.
It wasn't worth the effort back then.

Spec has more details: https://github.com/NuGet/Home/blob/dev/accepted/2022/duplicate-nuget-item-error-handling.md#technical-explanation.

@@ -359,7 +359,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<CheckForDuplicateNuGetItemsTask
Items="@(NuGetAuditSuppress)"
ItemName="NuGetAuditSuppress"
LogCode="NU1505"
Copy link
Member Author

Choose a reason for hiding this comment

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

@nkolev92 from NuGet/Home#13620

This should be a new log code.

Why do we use a different code for each MSBuild item type?

The docs are the same for each item type, since the resolution is the same. I feel like all this design decision does is cause us extra busy work. I feel like one error code, where in the docs we mention that this can happen for multiple item types, would just be easier, with no downside.

Copy link
Member

Choose a reason for hiding this comment

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

The owner of the action isn't always the same.
In particular the SDK sets PackageDownload items, so any issues are likely problems on the SDK side.

For example, if you search with NU1505, you end up with dotnet/sdk#24747.
NU1504 gives you something else.

It's duplicative sure, but I think there's an eventual limit to the number of items we'll be using :)

nkolev92
nkolev92 previously approved these changes Jul 16, 2024
@zivkan zivkan dismissed stale reviews from nkolev92 and Nigusu-Allehu via ed458cf July 16, 2024 21:44
@zivkan zivkan force-pushed the dev-zivkan-NuGetAuditSuppress-duplicate-code branch from 7239453 to ed458cf Compare July 16, 2024 21:44
@zivkan zivkan merged commit 82ed401 into dev Jul 17, 2024
28 of 29 checks passed
@zivkan zivkan deleted the dev-zivkan-NuGetAuditSuppress-duplicate-code branch July 17, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a log code NuGetAuditSuppress duplicate items
3 participants