Deprecate http usage: `source` commands, promote warning to error for http sources #5702

Nigusu-Allehu · 2024-03-21T19:59:12Z

Bug

Fixes: NuGet/Home#13333

Regression? Last working version:

Description

promotes http warnings to errors in the following commands
- source add
- source update
- source enable
removes http warning in the following commands
- source list : I believe the list command should list out all the available sources and allow users to observe what they have rather than showing an error and blocking them. Instead warn them about the HTTP sources

PR Checklist

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs

nkolev92

Some test feedback.
Rest looks great!
Nicely done.

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

kartheekp-ms

Once all the PRs related to this effort are merged, I think we should remove NU1803 error code from the product. If we still would like to warn for HTTP sources in the list source command, then we can keep the error code.

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs

kartheekp-ms · 2024-04-19T01:47:59Z

src/NuGet.Core/NuGet.Commands/Strings.resx

@@ -1107,4 +1107,8 @@ Non-HTTPS access will be removed in a future version. Consider migrating to 'HTT
    <value>You are running the '{0}' operation with an 'HTTP' source: {1}. NuGet requires HTTPS sources. To use an HTTP source, you must explicitly set 'allowInsecureConnections' to true in your NuGet.Config file. Please refer to https://aka.ms/nuget-https-everywhere.</value>
    <comment>0 - The command name. Ex. Push/Delete. 1 - server URI.</comment>
  </data>
+  <data name="Error_HttpSource_Single_Short" xml:space="preserve">


Can we the existing Error_HttpSource_Single resource instead of adding a new one?

It is because they are a little different. One tells the user to configure allowInsecureConnections while the other one does not.

There is duplication in the strings for Error_HttpSource_Single, Error_HttpSources_Multiple and Error_HttpSource_Single_Short resources. If we ever have to change the error message, we have to update in all the places. I guess we should be okay good for now.

I think it's okay to do this, and the repo has a lot of duplicate strings in resource files. My understanding was that it's not a significant difference for the localization team. Perhaps I'm remembering it's even preferred to just create a duplicate resource?!

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

donnie-msft

Looks pretty good! I think the test content is well done, just needs a little separation/renaming.

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

donnie-msft · 2024-04-26T19:41:58Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

@@ -236,40 +259,38 @@ public void Sources_WarnWhenListHttpSource(string initialSource, string secondSo
                CommandRunnerResult result = _fixture.RunDotnetExpectSuccess(configFileDirectory, string.Join(" ", args));

                // Assert
-                Assert.Equal(shouldWarn, result.Output.Contains(warningMessage));
+                Assert.Contains(initialSource, result.AllOutput);


Is this scenario supposed to be an Error? If so, this assertion should be like the others and look for the Error output, not All Output.

The nuget list command, allows users to list configured sources. However, just because a user has an http source, we should not be throwing an error for the list command. Instead, we should list all the configured sources and warn them about it. As a result, I have not added an error for the list command.

donnie-msft · 2024-04-26T20:03:43Z

src/NuGet.Core/NuGet.Commands/Strings.resx

@@ -1107,4 +1107,8 @@ Non-HTTPS access will be removed in a future version. Consider migrating to 'HTT
    <value>You are running the '{0}' operation with an 'HTTP' source: {1}. NuGet requires HTTPS sources. To use an HTTP source, you must explicitly set 'allowInsecureConnections' to true in your NuGet.Config file. Please refer to https://aka.ms/nuget-https-everywhere.</value>
    <comment>0 - The command name. Ex. Push/Delete. 1 - server URI.</comment>
  </data>
+  <data name="Error_HttpSource_Single_Short" xml:space="preserve">


I think it's okay to do this, and the repo has a lot of duplicate strings in resource files. My understanding was that it's not a significant difference for the localization team. Perhaps I'm remembering it's even preferred to just create a duplicate resource?!

donnie-msft · 2024-04-26T20:05:57Z

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

+        [InlineData("http://source.test", "http://source.test.2")]
+        [InlineData("https://source.test", "http://source.test.2")]
+        [InlineData("https://source.test", "https://source.test.2")]
+        public void SourcesList_WithDefaultFormat_UsesDetailedFormat(string source, string secondSource)


When I read this test's assertions, I don't understand where the "Detailed format" comes in.
Also, shouldn't the test assert that no warning exists in the output?

jeffkl · 2024-04-29T15:38:07Z

src/NuGet.Core/NuGet.Commands/Properties/AssemblyInfo.cs

@@ -9,3 +9,4 @@
 [assembly: InternalsVisibleTo("NuGet.SolutionRestoreManager.Test, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
 [assembly: InternalsVisibleTo("NuGet.PackageManagement.VisualStudio.Test, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
 [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")]
+[assembly: InternalsVisibleTo("Dotnet.Integration.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]


Be aware that this will conflict with my recent change: #5744

You'll need to add an <InternalsVisibleTo /> item to the .csproj instead. I wonder if it would be worth rebasing your feature branch.

Thanks, I will rebase

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs

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

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

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

@NuGet/nuget-client Any suggestions?

… http sources (#5702)

* Deprecate http usage: `nuget search` promote warning to error for http sources (#5693) * Deprecate http usage: `nuget list` promote warning to error for http sources (#5698) * Deprecate http usage: `nuget push` promote warning to error for http sources (#5705) * Deprecate http usage: `delete` operations, promote warning to error for http sources (#5703) * Add `allowinsecureconnections` option (#5742) * remove integration test for now (#5758) * Deprecate http: Promote from warning to error in VS Options NuGet Package Manager (#5732) * Deprecate http usage: `source` commands, promote warning to error for http sources (#5702) * Deprecate http usage: `restore` scenarios promote warning to error for http sources (#5731) * use `--allow-insecure-connections` for dotnet nuget add command (#5853) * tests * clean up * clean up * white space * tests * comment

… http sources (#5702)

Nigusu-Allehu requested a review from a team as a code owner March 21, 2024 19:59

Nigusu-Allehu mentioned this pull request Mar 21, 2024

Deprecate http usage: source commands promote warning to error for http sources #5700

Merged

8 tasks

nkolev92 reviewed Mar 21, 2024

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs Outdated Show resolved Hide resolved

Nigusu-Allehu changed the title ~~Deprecate http usage: source commands promote warning to error for http sources~~ Deprecate http usage: source commands, promote warning to error for http sources Mar 21, 2024

Nigusu-Allehu self-assigned this Mar 21, 2024

dotnet-policy-service bot added Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed and removed Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed labels Mar 29, 2024

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-source branch from ef0d0ed to 9cd70ce Compare April 3, 2024 20:22

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Apr 11, 2024

Nigusu-Allehu force-pushed the dev-nyenework-http-error-source branch from 9cd70ce to 4272ac3 Compare April 16, 2024 17:57

dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Apr 16, 2024

Nigusu-Allehu requested a review from nkolev92 April 16, 2024 19:54

nkolev92 reviewed Apr 16, 2024

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs Outdated Show resolved Hide resolved

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs Outdated Show resolved Hide resolved

Nigusu-Allehu added 3 commits April 17, 2024 10:43

Promote http warning to error in source command

a05b3ae

fix test

2b55272

use allowInsecureConnections option

94fb612

Nigusu-Allehu force-pushed the dev-nyenework-http-error-source branch from 07c59fe to 94fb612 Compare April 17, 2024 17:43

cleanup

a607f44

kartheekp-ms reviewed Apr 19, 2024

View reviewed changes

Nigusu-Allehu added 2 commits April 19, 2024 11:48

clean up

469329f

Fix test

e4cc602

Nigusu-Allehu requested review from nkolev92 and kartheekp-ms April 19, 2024 20:26

kartheekp-ms reviewed Apr 25, 2024

View reviewed changes

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs Outdated Show resolved Hide resolved

donnie-msft reviewed Apr 26, 2024

View reviewed changes

jeffkl reviewed Apr 29, 2024

View reviewed changes

warn on list

1990db6

Nigusu-Allehu requested review from jeffkl and donnie-msft April 29, 2024 22:44

Nigusu-Allehu requested a review from kartheekp-ms April 29, 2024 22:44

Nigusu-Allehu added 2 commits May 2, 2024 08:33

cleanup tests

f5f3f37

Fix push test

7d7ef43

kartheekp-ms reviewed May 6, 2024

View reviewed changes

src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Commands/Strings.Designer.cs Show resolved Hide resolved

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

kartheekp-ms mentioned this pull request May 6, 2024

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

Merged

8 tasks

Clean up tests

b643b7f

Nigusu-Allehu requested a review from kartheekp-ms May 8, 2024 17:47

Clean up

024634b

nkolev92 approved these changes May 9, 2024

View reviewed changes

jeffkl approved these changes May 9, 2024

View reviewed changes

Nigusu-Allehu merged commit bd59edd into dev-feature-http-error May 9, 2024
13 of 16 checks passed

Nigusu-Allehu deleted the dev-nyenework-http-error-source branch May 9, 2024 17:10

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

Deprecate http usage: source commands, promote warning to error for…

1ba05af

… http sources (#5702)

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

Deprecate http usage: source commands, promote warning to error for…

d628c38

… http sources (#5702)

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

Deprecate http usage: source commands, promote warning to error for…

586687b

… http sources (#5702)

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

Deprecate http usage: source commands, promote warning to error for…

9a15ced

… http sources (#5702)

Nigusu-Allehu added a commit that referenced this pull request Jun 17, 2024

Deprecate http usage: source commands, promote warning to error for…

adaa04b

… http sources (#5702)

Nigusu-Allehu added a commit that referenced this pull request Jul 2, 2024

Deprecate http usage: source commands, promote warning to error for…

0321691

… http sources (#5702)

Nigusu-Allehu added a commit that referenced this pull request Jul 2, 2024

Deprecate http usage: source commands, promote warning to error for…

ea40d7e

… http sources (#5702)

Nigusu-Allehu added a commit that referenced this pull request Jul 3, 2024

Deprecate http usage: source commands, promote warning to error for…

e4cd034

… http sources (#5702)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate http usage: `source` commands, promote warning to error for http sources #5702

Deprecate http usage: `source` commands, promote warning to error for http sources #5702

Nigusu-Allehu commented Mar 21, 2024 •

edited

Loading

nkolev92 left a comment

kartheekp-ms left a comment •

edited

Loading

kartheekp-ms Apr 19, 2024

Nigusu-Allehu Apr 19, 2024

kartheekp-ms Apr 25, 2024

donnie-msft Apr 26, 2024

donnie-msft left a comment

donnie-msft Apr 26, 2024

Nigusu-Allehu Apr 29, 2024

donnie-msft Apr 26, 2024

donnie-msft Apr 26, 2024

jeffkl Apr 29, 2024

Nigusu-Allehu Apr 29, 2024

Nigusu-Allehu commented May 9, 2024

Deprecate http usage: source commands, promote warning to error for http sources #5702

Deprecate http usage: source commands, promote warning to error for http sources #5702

Conversation

Nigusu-Allehu commented Mar 21, 2024 • edited Loading

Bug

Description

PR Checklist

nkolev92 left a comment

Choose a reason for hiding this comment

kartheekp-ms left a comment • edited Loading

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

donnie-msft 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

Nigusu-Allehu commented May 9, 2024

Deprecate http usage: `source` commands, promote warning to error for http sources #5702

Deprecate http usage: `source` commands, promote warning to error for http sources #5702

Nigusu-Allehu commented Mar 21, 2024 •

edited

Loading

kartheekp-ms left a comment •

edited

Loading