Opt out Https everywhere in packages.config restore scenarios in all NuGet tools #5356

heng-liu · 2023-08-11T22:20:07Z

Bug

Regression? Last working version:

Description

When allowInsecureConnections property in packageSources section is set to true in NuGet.Config files, suppress warnings for HTTP sources in packages.config restore scenarios(including install and update) in all NuGet tools.

The PRs enable the HTTP warnings in those scenarios are:
#4583
#4633

PR Checklist

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

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

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

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

dtivel · 2023-08-14T15:04:17Z

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

+        [Theory]
+        [InlineData("true")]
+        [InlineData("TRUE")]
+        public async Task UpdateCommand_WithHttpSourceAndTrueAllowInsecureConnections_NoWarns(string allowInsecureConnections)


This and the following test look mostly the same. In an effort to dedupe code, you could have one test that parameterizes this. Also consider this feedback elsewhere.

[Theory] [InlineData("true", true]) [InlineData("TRUE", true]) [InlineData("false", false]) [InlineData("FALSE", false]) [InlineData("invalid", false]) ... public async Task UpdateCommand_WhenAllowInsecureConnectionsIsSet_WarnsAppropriately(string allowInsecureConnectionsValue, bool allowInsecureConnectionsValue) { ... const string warning = "You are running the 'update' operation with an 'http' source"; if (allowInsecureConnections) { r.AllOutput.Should().NotContain(warning, because: r.AllOutput); } else { r.AllOutput.Should().Contain(warning, because: r.AllOutput); } }

Thanks @dtivel ! There are also comments suggesting differently: #5343 (comment)
So I think people might have different preferences.

I wouldn't view it as a difference in preferences. The spirit of the comment is to deduplicate code.

I showed one path to deduplication. Even with the other approach, which is also fine, you can still deduplicate code by extracting reusable code into helper methods.

+1 to what @dtivel suggested.

The point I was making in the comment you linked is that Setup had many branches, making it hard to understand the path to a test failure. I think the proposal here is to extract setup helper methods, which is OK to consider, and a different topic, really. I did this in my recent PR (eg, SetupUIServiceWithPackageSearchMetadata) which had MUCH more complicated Setup, .so it's hard to say it's needed in your PR.

Another viewpoint I don't see in this thread, but which has been expressed in my PR reviews, is that deduping a few lines of code isn't necessarily making it easier for the reader. There's a bit of an art to it.

I'd say spend just a little effort (<30 minutes?) to see if you can extract methods that saves more than 3-4 lines or so per test. Sometimes you need to return objects and pass them around, and end up with nearly the same setup code. Given, there are 1000+ lines of tests in this file with far more complicated duplicated code, we may want to separately come up with a guideline as a Team, then apply that decision to new tests, and refactor old tests.

I agree that it would be good to have a guideline as a Team so we will have a consistent standard to follow.
Otherwise, we will have to keep addressing different comments and that is not efficient.
There are many existing tests have the same setup without deduplication: RestoreCommand_VerifyMinClientVersionV2SourceAsync and RestoreCommand_VerifyMinClientVersionV3SourceAsync
So it's not clear when shall we deduplicate code and when shall we keep duplicate code to make test easy to understand.

heng-liu · 2023-08-16T00:34:25Z

Thanks all for reviewing!
All comments are addressed. Could you please review it again? Thanks!

aortiz-msft · 2023-08-16T00:33:23Z

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

@@ -430,6 +430,7 @@
    <comment>{0} - package id</comment>
  </data>
  <data name="Warning_HttpServerUsage" xml:space="preserve">
-    <value>You are running the '{0}' operation with an 'http' source, '{1}'. Support for 'http' sources will be removed in a future version.</value>
+    <value>You are running the '{0}' operation with an 'HTTP' source, '{1}'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.</value>


I don't think this aligns to the new plan. You should check with @JonDouglas on what the message should be.

Thanks @aortiz-msft ! There are 5 Warning_HttpServerUsage in our codebase: https://github.com/search?q=repo%3ANuGet%2FNuGet.Client%20name%3D%22Warning_HttpServerUsage%22&type=code
And only one of the 5 is different. That's why I made this change to make all of them the same.
Hi @JonDouglas , could you help advise if this change is needed? Thanks!

@aortiz-msft could you point reviewers to what plan you're referring to?

https://github.com/NuGet/Home/pull/12542/files

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

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

aortiz-msft · 2023-08-16T00:40:20Z

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

+        [Theory]
+        [InlineData("true")]
+        [InlineData("TRUE")]
+        public async Task UpdateCommand_WithHttpSourceAndTrueAllowInsecureConnections_NoWarns(string allowInsecureConnections)


+1 to what @dtivel suggested.

aortiz-msft · 2023-08-16T23:12:33Z

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

+        [InlineData("", true)]
+        [InlineData("true", false)]
+        [InlineData("TRUE", false)]
+        public async Task Restore_WithHttpSourceAndFalseAllowInsecureConnections_WarnsCorrectly(string allowInsecureConnections, bool hasHttpWarning)


Suggestion: isHttpWarningExpected?

aortiz-msft · 2023-08-16T23:13:25Z

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

+        [InlineData("", true)]
+        [InlineData("true", false)]
+        [InlineData("TRUE", false)]
+        public async Task UpdateCommand_WithHttpSourceAndAllowInsecureConnections_WarnsCorrectly(string allowInsecureConnections, bool hasHttpWarning)


aortiz-msft · 2023-08-16T23:13:47Z

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

+        [InlineData("", true)]
+        [InlineData("true", false)]
+        [InlineData("TRUE", false)]
+        public async Task MsbuildRestore_PackagesConfigDependency_WithHttpSourceAndAllowInsecureConnections_WarnsCorrectly(string allowInsecureConnections, bool hasHttpWarning)


aortiz-msft

…NuGet tools (#5356)

heng-liu requested a review from a team as a code owner August 11, 2023 22:20

donnie-msft approved these changes Aug 14, 2023

View reviewed changes

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

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

dtivel approved these changes Aug 14, 2023

View reviewed changes

jeffkl approved these changes Aug 14, 2023

View reviewed changes

heng-liu and others added 2 commits August 15, 2023 13:05

suppress http warning when AllowInsecureConnections is set to true

6710b68

address feedback

324c440

heng-liu force-pushed the dev-hengliu-optoutHttpWarning-packagesConfigRestore branch from f4b8f9f to 324c440 Compare August 15, 2023 22:27

heng-liu added 3 commits August 15, 2023 15:43

address comments

04a04f7

fix whitespace

60b4dd4

Fix tests

7951e33

heng-liu requested review from dtivel, jeffkl and donnie-msft August 16, 2023 00:33

aortiz-msft requested changes Aug 16, 2023

View reviewed changes

heng-liu added 3 commits August 15, 2023 21:18

change back unnecessary change

997a9b8

deduplicate test code

46e2d34

fix whitespace formatting

466f635

heng-liu requested a review from aortiz-msft August 16, 2023 14:54

aortiz-msft reviewed Aug 16, 2023

View reviewed changes

aortiz-msft approved these changes Aug 16, 2023

View reviewed changes

heng-liu changed the title ~~Suppress Http warnings in packages.config restore scenarios in all NuGet tools~~ Opt out Https everywhere in packages.config restore scenarios in all NuGet tools Aug 17, 2023

donnie-msft approved these changes Aug 17, 2023

View reviewed changes

heng-liu merged commit 8027512 into dev-feature-optoutHttpWarnings Aug 17, 2023

heng-liu deleted the dev-hengliu-optoutHttpWarning-packagesConfigRestore branch August 17, 2023 21:18

heng-liu added a commit that referenced this pull request Aug 21, 2023

Opt out Https everywhere in packages.config restore scenarios in all …

fafa0c1

…NuGet tools (#5356)

heng-liu added a commit that referenced this pull request Aug 21, 2023

Opt out Https everywhere in packages.config restore scenarios in all …

d8ab6a3

…NuGet tools (#5356)

heng-liu added a commit that referenced this pull request Aug 28, 2023

Opt out Https everywhere in packages.config restore scenarios in all …

3c831d0

…NuGet tools (#5356)

heng-liu added a commit that referenced this pull request Sep 8, 2023

Opt out Https everywhere in packages.config restore scenarios in all …

99fa6e0

…NuGet tools (#5356)

heng-liu added a commit that referenced this pull request Sep 8, 2023

Opt out Https everywhere in packages.config restore scenarios in all …

e9dd553

…NuGet tools (#5356)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opt out Https everywhere in packages.config restore scenarios in all NuGet tools #5356

Opt out Https everywhere in packages.config restore scenarios in all NuGet tools #5356

heng-liu commented Aug 11, 2023 •

edited

Loading

dtivel Aug 14, 2023

heng-liu Aug 16, 2023

dtivel Aug 16, 2023

aortiz-msft Aug 16, 2023

donnie-msft Aug 16, 2023

heng-liu Aug 16, 2023 •

edited

Loading

heng-liu commented Aug 16, 2023

aortiz-msft Aug 16, 2023

heng-liu Aug 16, 2023

donnie-msft Aug 16, 2023

aortiz-msft Aug 16, 2023

aortiz-msft Aug 16, 2023

aortiz-msft Aug 16, 2023

aortiz-msft Aug 16, 2023

aortiz-msft Aug 16, 2023

aortiz-msft left a comment

Opt out Https everywhere in packages.config restore scenarios in all NuGet tools #5356

Opt out Https everywhere in packages.config restore scenarios in all NuGet tools #5356

Conversation

heng-liu commented Aug 11, 2023 • edited Loading

Bug

Description

PR Checklist

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

heng-liu Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

heng-liu commented Aug 16, 2023

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

Choose a reason for hiding this comment

aortiz-msft left a comment

Choose a reason for hiding this comment

heng-liu commented Aug 11, 2023 •

edited

Loading

heng-liu Aug 16, 2023 •

edited

Loading