Skip to content

Warn when http sources are used in install/update scenarios for packages.config based projects#4633

Merged
nkolev92 merged 5 commits intodevfrom
dev-nkole92-packageInstallationAndUpdatingWarns
May 24, 2022
Merged

Warn when http sources are used in install/update scenarios for packages.config based projects#4633
nkolev92 merged 5 commits intodevfrom
dev-nkole92-packageInstallationAndUpdatingWarns

Conversation

@nkolev92
Copy link
Member

@nkolev92 nkolev92 commented May 18, 2022

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1629

Regression? Last working version:

Description

This changes comes down to the functionality in NuGetPackageManager.
It is disjointed and it can be a bit challenging to find a spot to warn.

In the end, changes were needed in only 2 places, the preview install and update codepaths on a project level for packages.config projects only.

PackageReference projects warn by different means and uninstall scenarios in packages.config do not directly use any sources, but rather if need be, restore will be called.

You can use https://github.com/NuGet/Client.Engineering/issues/1523 for reference of all the scenarios I analyzed.
In addition to the automated tests, I validated manually that all scenarios warn correctly.

Here's the long description of my analysis of all NuGetPackageManager methods.

Execute Actions:

ExecuteBuildIntegratedProjectActionsAsync - We don't really need to handle build integrated. PackageReference projets raise warnings in different places

ExecuteNuGetProjectActionsAsync - We won't warn in execute project actions, we'll do it in previews only.
ExecuteNuGetProjectActionsAsync - We won't warn in execute project actions, we'll do it in previews only.
ExecuteNuGetProjectActionsAsync - We won't warn in execute project actions, we'll do it in previews only.

PreviewBuildIntegratedProjectActionsAsync - We don't really need to handle build integrated. PackageReference projets raise warnings in different places

Install:

InstallPackageAsync - Calls one of the other ones.
InstallPackageAsync - Calls one of the other ones.
InstallPackageAsync - Calls one of the other ones.
InstallPackageAsync - Calls one of the other ones.
InstallPackageAsync - Calls one of the other ones.
InstallPackageAsync - Calls one of the other ones.
InstallPackageAsync - Calls bottom one
InstallPackageAsync - Calls PreviewInstallPackageAsync

PreviewInstallPackageAsync - Calls bottom one
PreviewInstallPackageAsync - Calls bottom one
PreviewInstallPackageAsync - Calls bottom one
PreviewInstallPackageAsync - Should be the one warning.

PreviewProjectsInstallPackageAsync - Calls one of the PreviewInstallPackageAsync eventually ending up in the same. By warning in the last PreviewInstallPackageAsync, we'd be warning everywhere.

Update:

PreviewUpdatePackagesAsync - All of these call PreviewUpdatePackagesAsync private method.
PreviewUpdatePackagesAsync - All of these call PreviewUpdatePackagesAsync private method.
PreviewUpdatePackagesAsync - All of these call PreviewUpdatePackagesAsync private method.
PreviewUpdatePackagesAsync - All of these call PreviewUpdatePackagesAsync private method.

PreviewUpdatePackagesAsync splits into build integrated and non-buiild integrated
We will warn in PreviewUpdatePackagesForClassicAsync for each project individually.

Uninstall:

UninstallPackageAsync -> Calls the preview ones.

PreviewProjectsUninstallPackageAsync - Calls PreviewUninstallPackageInternalAsync eventually. The trick is that in packages.config scenarios, there's no feed usage in Uninstall flows. Potentially packages may be restored prior to that, but that'll warn through a different codepath.

PreviewUninstallPackageAsync -> both of these call PreviewUninstallPackageInternalAsync.
PreviewUninstallPackageAsync -> both of these call PreviewUninstallPackageInternalAsync.

Note that the tests in question are enough to cover all installation scenarios as the warning is being raised from NuGetPackageManager and all operations share that.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't anymore :D

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 moved this test into a different class, because if you look at the other tests in this class they are all signing related CI only tests.

@nkolev92 nkolev92 changed the title Warn for install/update scenarios for packages.config based projects Warn when http sources are used in install/update scenarios for packages.config based projects May 18, 2022
@nkolev92 nkolev92 force-pushed the dev-nkole92-packageInstallationAndUpdatingWarns branch from 32cca57 to feb4bbe Compare May 18, 2022 23:24
@nkolev92 nkolev92 force-pushed the dev-nkole92-packageInstallationAndUpdatingWarns branch from 2bc400c to 5abdb0f Compare May 20, 2022 00:53
@nkolev92 nkolev92 marked this pull request as ready for review May 20, 2022 00:55
@nkolev92 nkolev92 requested a review from a team as a code owner May 20, 2022 00:55
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.

2 participants