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

NuGetAuditSuppress for packages.config in VS #5918

Merged
merged 5 commits into from
Jul 29, 2024

Conversation

zivkan
Copy link
Member

@zivkan zivkan commented Jul 12, 2024

Bug

Fixes: NuGet/Home#13575

Description

NuGetAuditSuppress for packages.config CLI scenarios was recently added. This PR makes it work in Visual Studio as well.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • docs: feature already documented

@zivkan zivkan marked this pull request as ready for review July 14, 2024 09:05
@zivkan zivkan requested a review from a team as a code owner July 14, 2024 09:05
@@ -131,5 +131,11 @@ private static async Task<IBuildDependencyProjectReferencesService> GetProjectRe
{
throw new NotSupportedException();
}

public async Task<IReadOnlyList<(string id, string[] metadata)>> GetItemsAsync(string itemTypeName, params string[] metadataNames)
Copy link
Member

Choose a reason for hiding this comment

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

Does CPS implement that API?
I remember it being added to the old project system when we wanted CPM to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. There's no code path that calls GetItemsAsync currently on CpsProjectSystemReferenceReader. It's just needed because it implements the same interface as I need for packages.config (so, software design bug).

I thought it might be better to call the same implementation as the other readers, because maybe it works, rather than throw a NotImplementedException. But I can be persuaded to throw.

Copy link
Member

Choose a reason for hiding this comment

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

If it's not implemented, would it crash when GetItems is called?
Unless I misread the code there.

Copy link
Member Author

Choose a reason for hiding this comment

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

same response as #5918 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Given my misunderstanding about MSBuildNuGetProject (not) being packages.config only, if I move the GetItemsAsync method out of IProjectSystemReferenceReader and into MSBuildNuGetProject, then CPS won't need to implement it, but CLI restore will (it can noop/throw, since it won't be called in code path).

It's a question of what we think the best design is, as every alternative I can see as feasible has drawbacks.

@zivkan zivkan merged commit 8e96cf5 into dev Jul 29, 2024
27 of 29 checks passed
@zivkan zivkan deleted the dev-zivkan-NuGetAudit-Suppress-packages.config-VS branch July 29, 2024 22:36
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.

NuGetAuditSuppress for packages.config
3 participants