Deprecate http usage: `restore` scenarios promote warning to error for http sources #5731

Nigusu-Allehu · 2024-03-27T18:22:21Z

Bug

Fixes: NuGet/Home#13353

Regression? Last working version:

Description

Promote http source usage warnings in restore scenarios to errors.

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
https://github.com/Add documentation for NU1302 Home#13502
- Documentation PR or issue filled
- OR
- N/A

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

src/NuGet.Core/NuGet.Common/Errors/NuGetLogCode.cs

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreRetryTests.cs

nkolev92

Apologies for the slow turnaround.
Product changes look good.

Some test questions.

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreRetryTests.cs

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetInstallCommandTest.cs

test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Caching/CachingTestContext.cs

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetInstallCommandTest.cs

src/NuGet.Core/NuGet.PackageManagement/Strings.resx

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs

donnie-msft · 2024-04-26T20:20:37Z

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs

-            Assert.Contains($"Added package 'A.1.0.0' to folder '{projectBPackages}'", result.Output);
-            Assert.DoesNotContain(warningForHttpsSource, result.Output);
-            if (hasHttpWarning)
+            Assert.DoesNotContain(errorForHttpsSource, result.AllOutput);


Instead of asserting a missing error, why can't this depend on result.Success.Should().BeTrue() ?

From what I have seen, it is possible for the restore command runner to log an error and still exit code of 0. This is just making sure that is not happening and this error is not logged.

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs

donnie-msft · 2024-04-26T20:22:38Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/FeedPackagePruningTests.cs

@@ -66,7 +66,7 @@ public async Task FeedPackagePruning_GivenThatAV3FeedPrunesAPackageDuringRestore
                server.Start();

                var feedUrl = server.Uri + "index.json";
-
+                pathContext.Settings.AddSource(feedUrl, feedUrl, allowInsecureConnectionsValue: "true");


Is there a tracking issue to change all the test dependencies to use HTTPS?

No. I think that is a good point.

test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs

test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs

src/NuGet.Core/NuGet.PackageManagement/Strings.Designer.cs

kartheekp-ms · 2024-05-06T23:23:55Z

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Caching/CachingTestContext.cs

 <configuration>
  <packageSources>
    <clear />
+    <add key=""http-feed"" value=""{source}"" allowInsecureConnections=""true""/>


It looks like allowInsecureConnections setting can also be configured through an environment variable. I guess we need a test to verify that behavior as well. I am okay with addressing this comment in a follow up pull-request.

https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/Settings/Items/SourceItem.cs#L24-L36

Are you referring to setting the config file as follows

<packageSources> <clear /> <add key="source" value="http://source" allowInsecureConnections="${Allow_Insecure_Connection_Value}" /> </packageSources>

Yes, you are correct Nigusu. However, environment variable syntax is a bit different as I explained below. NuGet uses Environment.ExpandEnvironment .NET API internally.

<packageSources> <clear /> <add key="source" value="http://source" allowInsecureConnections="%AllowInsecureConnectionValue%" /> </packageSources>

Feel free to address this comment in a separate PR.

Isn't env var vs a nuget.config value stylistic?
What's the benefit of the env var over the nguet.config.

I can't think of any benefit with the environment variable for this setting. I added the comment because currently code expands environment variable for this setting also. May be we could remove the environment variable support for this setting because it doesn't add much value to the users.

test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs

Nigusu-Allehu · 2024-05-09T04:36:10Z

@NuGet/nuget-client Any suggestions?

…r http sources (#5731)

Nigusu-Allehu self-assigned this Mar 27, 2024

Nigusu-Allehu requested a review from a team as a code owner March 27, 2024 18:22

Nigusu-Allehu changed the title ~~Deprecate http usage: nuget push promote warning to error for http sources~~ Deprecate http usage: restore scenarios promote warning to error for http sources Mar 27, 2024

nkolev92 reviewed Mar 28, 2024

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated Show resolved Hide resolved

Nigusu-Allehu force-pushed the dev-feature-http-error branch from 18162b4 to faf0a89 Compare April 3, 2024 20:15

Nigusu-Allehu force-pushed the dev-nyenework-http-error-restore branch from 77e06d4 to 1851cf0 Compare April 3, 2024 21:14

Nigusu-Allehu requested a review from nkolev92 April 3, 2024 21:44

jeffkl reviewed Apr 9, 2024

View reviewed changes

src/NuGet.Core/NuGet.Common/Errors/NuGetLogCode.cs Show resolved Hide resolved

nkolev92 reviewed Apr 9, 2024

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs Show resolved Hide resolved

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreRetryTests.cs Show resolved Hide resolved

Nigusu-Allehu and others added 9 commits April 17, 2024 10:42

restore: http-error

f54a6da

fix tests

8e29cd8

update Nu code

3dabfd4

Fix tests

70172bb

Fix tests

0706a11

fix tests

36ad7b2

white space

06cd6b0

cleanup

07caf2c

white space

1fbf7b3

Nigusu-Allehu force-pushed the dev-nyenework-http-error-restore branch from d2054fb to 1fbf7b3 Compare April 17, 2024 17:42

Nigusu-Allehu changed the title ~~Deprecate http usage: restore scenarios promote warning to error for http sources~~ Deprecate http usage: restore / install scenarios promote warning to error for http sources Apr 19, 2024

Nigusu-Allehu requested a review from nkolev92 April 19, 2024 21:11

nkolev92 reviewed Apr 23, 2024

View reviewed changes

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs Outdated Show resolved Hide resolved

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreRetryTests.cs Show resolved Hide resolved

jebriede reviewed Apr 23, 2024

View reviewed changes

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetInstallCommandTest.cs Outdated Show resolved Hide resolved

test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Apr 25, 2024

View reviewed changes

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Caching/CachingTestContext.cs Outdated Show resolved Hide resolved

kartheekp-ms reviewed Apr 25, 2024

View reviewed changes

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetInstallCommandTest.cs Outdated Show resolved Hide resolved

Nigusu-Allehu added 3 commits April 24, 2024 18:11

localize test string

575dffe

Remove install scenarios

3fb0e04

Localize

2efed9d

Nigusu-Allehu changed the title ~~Deprecate http usage: restore / install scenarios promote warning to error for http sources~~ Deprecate http usage: restore scenarios promote warning to error for http sources Apr 25, 2024

Nigusu-Allehu requested a review from nkolev92 April 25, 2024 23:16

Nigusu-Allehu requested review from jeffkl, kartheekp-ms and jebriede April 25, 2024 23:16

donnie-msft reviewed Apr 26, 2024

View reviewed changes

Nigusu-Allehu added 3 commits April 26, 2024 13:55

Error instead of warning

f03ddb1

Return empty restore result if not secure connection

70a2e67

Cleanup test

a7bc389

Nigusu-Allehu requested a review from donnie-msft April 26, 2024 22:55

fix tests that use http-sources

9f3263f

dotnet-policy-service bot added the Status:No recent activity No recent activity. label May 6, 2024

kartheekp-ms reviewed May 6, 2024

View reviewed changes

dotnet-policy-service bot removed the Status:No recent activity No recent activity. label May 6, 2024

Split test

b6b2c8a

Nigusu-Allehu requested a review from kartheekp-ms May 9, 2024 04:35

nkolev92 approved these changes May 9, 2024

View reviewed changes

cleanup

1501d69

kartheekp-ms approved these changes May 9, 2024

View reviewed changes

Nigusu-Allehu merged commit 079b9ea into dev-feature-http-error May 9, 2024
12 of 16 checks passed

Nigusu-Allehu deleted the dev-nyenework-http-error-restore branch May 9, 2024 22:02

Nigusu-Allehu added a commit that referenced this pull request May 22, 2024

Deprecate http usage: restore scenarios promote warning to error fo…

c053b9d

…r http sources (#5731)

Nigusu-Allehu added a commit that referenced this pull request May 22, 2024

Deprecate http usage: restore scenarios promote warning to error fo…

7a82ae2

…r http sources (#5731)

Nigusu-Allehu added a commit that referenced this pull request May 22, 2024

Deprecate http usage: restore scenarios promote warning to error fo…

3cf2401

…r http sources (#5731)

Nigusu-Allehu added a commit that referenced this pull request May 22, 2024

Deprecate http usage: restore scenarios promote warning to error fo…

ede8fe5

…r http sources (#5731)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate http usage: `restore` scenarios promote warning to error for http sources #5731

Deprecate http usage: `restore` scenarios promote warning to error for http sources #5731

Nigusu-Allehu commented Mar 27, 2024 •

edited

nkolev92 left a comment

donnie-msft Apr 26, 2024

Nigusu-Allehu Apr 26, 2024

donnie-msft Apr 26, 2024

Nigusu-Allehu Apr 26, 2024

kartheekp-ms May 6, 2024

Nigusu-Allehu May 8, 2024

kartheekp-ms May 9, 2024 •

edited

kartheekp-ms May 9, 2024

nkolev92 May 9, 2024

kartheekp-ms May 10, 2024 •

edited

Nigusu-Allehu commented May 9, 2024

Deprecate http usage: restore scenarios promote warning to error for http sources #5731

Deprecate http usage: restore scenarios promote warning to error for http sources #5731

Conversation

Nigusu-Allehu commented Mar 27, 2024 • edited

Bug

Description

PR Checklist

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms May 10, 2024 • edited

Choose a reason for hiding this comment

Nigusu-Allehu commented May 9, 2024

Deprecate http usage: `restore` scenarios promote warning to error for http sources #5731

Deprecate http usage: `restore` scenarios promote warning to error for http sources #5731

Nigusu-Allehu commented Mar 27, 2024 •

edited

kartheekp-ms May 9, 2024 •

edited

kartheekp-ms May 10, 2024 •

edited