Warn for http when used in source commands by martinrrm · Pull Request #4600 · NuGet/NuGet.Client

martinrrm · 2022-04-29T21:03:19Z

Bug

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

Regression? Last working version:

Description

Added a warn when using HTTP sources in dotnet and nuget commands.

PR Checklist

nkolev92 · 2022-04-29T21:07:12Z

Have we considered adding it to the list itself so that the warning is visible when the source isn't selected?

zivkan · 2022-04-29T22:41:49Z

The control is currently WinForms, with a custom renderer for each list item, so it's quite complex to change. I don't know how easy it would be to port this to WPF, which would make it a lot easier.

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourcesOptionsControl.cs

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

kartheekp-ms · 2022-05-06T17:54:10Z

I think we should split this PR into two. One PR for CLI experience and other one for VS experience. Looking at the screenshots in the PR description, I think it only addresses one scenario when user edits the URI scheme to HTTP. I think we should also consider displaying a warning icon next to the package source if it's URI Scheme is HTTP. Tagging @JonDouglas for inputs on VS experience.

martinrrm · 2022-05-10T17:12:05Z

Removed the UI changes and only left CLI changes

nkolev92 · 2022-05-10T17:13:39Z

@NuGet/nuget-client
Please re-review this.

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

donnie-msft · 2022-05-12T21:48:31Z

split this PR into two. One PR for CLI experience and other one for VS experience. Looking at the screenshots in the PR description, I think it only addresses one scenario when user edits the URI scheme to HTTP. I think we should also

@martinrrm - This PR doesn't seem to affect the VS options dialog, so I disagree that it will fix the issue # 1154
@nkolev92 - I don't understand the reasoning my issue for VS was changed to include CLI & VS. If you still feel that's the case, how do you suggest this PR avoid closing the VS portion of the issue marked as "Fixes"?

nkolev92 · 2022-05-12T22:00:50Z

@donnie-msft

We were capturing general scenarios for raising warnings. We want to do it all anyways.
We made that issue tackle source managing scenarios altogether.

I didn't want to create 2 or 3 different issues myself, as I didn't want to guess how many PRs these changes would take (by extension how many issues).

how do you suggest this PR avoid closing the VS portion of the issue marked as "Fixes"?

it's on @martinrrm as the issue assignee and PR author to ensure he has an issue for each PR as needed.

He can make that issue an epic and list 2-3 different issues, or he can just create one new issue and convert that particular issue back to a VS Options dialog specific.
This is bikeshedding imo.

donnie-msft

Nice! Just a few suggestions...

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

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

nkolev92 · 2022-05-12T22:07:32Z

For what is worth, I also added a comment when I changed the scope of the issue

https://github.com/NuGet/Client.Engineering/issues/1154#issuecomment-1082259969

donnie-msft · 2022-05-12T22:08:51Z

@martinrrm based on Nikolche's response, I went ahead and restored 1154 back to being VS Options focused. Please create a new GH issue for the CLI side, and make this PR resolve it rather than the VS one. Thanks

martinrrm · 2022-05-12T23:22:46Z

@donnie-msft Thanks for the review! About the issue, I decided to separate it into two PRs, thank you for updating the PRs title. New issue: https://github.com/NuGet/Client.Engineering/issues/1622

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

martinrrm · 2022-06-01T21:23:29Z

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

                var sourceItem = packageSourcesSection?.GetFirstItemWithAttribute<SourceItem>("key", "test_source");

-                Assert.Equal("http://test_source", sourceItem.GetValueAsPath());
+                Assert.Equal("https://test_source", sourceItem.GetValueAsPath());


Is this test fix correct? I think -Verbosity Quiet shows errors and warnings, right?

My understanding - this isn't testing output, but it's testing that a config file contains a particular URL. I'm actually not sure this assertion belongs here. The test is kind of overloaded.

I think -Verbosity Quiet shows errors and warnings, right?

Yup that is my understanding too.

I guess this change is required for assert at line#605 to pass.

donnie-msft · 2022-06-02T00:04:55Z

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

+        }
+
+        [PlatformTheory(Platform.Windows)]
+        [InlineData("http://source.test", true)]


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

kartheekp-ms · 2022-06-04T00:48:30Z

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

+                        if (httpPackageSources != null)
+                        {
+                            getLogger().LogWarning(
+                                string.Format(CultureInfo.CurrentCulture,


I think there are 2 different scenarios possible here. A single HTTP source or multiple HTTP sources. The following implementation has different messages for each case. Can we reuse the same functionality here?

NuGet.Client/src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

Lines 180 to 198 in d970aa3

if (httpPackageSources != null && httpPackageSources.Count != 0)

{

if (httpPackageSources.Count == 1)

{

listPackageArgs.Logger.LogWarning(

string.Format(CultureInfo.CurrentCulture,

Strings.Warning_HttpServerUsage,

"list package",

httpPackageSources[0]));

}

else

{

listPackageArgs.Logger.LogWarning(

string.Format(CultureInfo.CurrentCulture,

Strings.Warning_HttpServerUsage_MultipleSources,

"list package",

Environment.NewLine + string.Join(Environment.NewLine, httpPackageSources.Select(e => e.Name))));

}

}

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

erdembayar · 2022-06-04T00:56:04Z

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

+                                {
+                                    httpPackageSources = new();
+                                }
+                                httpPackageSources.Add(source);


By reading at code, it looks there is single source. If only single source is expected, then why bother creating new httpPackageSources list? it's unnecessary allocation.
Probably just do same thing you did in src/NuGet.Core/NuGet.Commands/SourcesCommands/SourceRunners.cs file.

This is the list command so there are multiple sources, here I'm iterating through all the sources and storing the HTTP sources in a new list so I can print them later

kartheekp-ms · 2022-06-04T01:04:19Z

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

                    "test_source",
                    "-Source",
-                    "http://test_source",
+                    "https://test_source",


I guess this change is required for assert at line#605 to pass.

martinrrm requested a review from a team as a code owner April 29, 2022 21:03

jeffkl reviewed May 2, 2022

View reviewed changes

src/NuGet.Clients/NuGet.PackageManagement.UI/Options/PackageSourcesOptionsControl.cs Outdated Show resolved Hide resolved

nkolev92 reviewed May 2, 2022

View reviewed changes

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

nkolev92 reviewed May 2, 2022

View reviewed changes

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

nkolev92 requested review from jeffkl and nkolev92 May 10, 2022 17:15

nkolev92 reviewed May 10, 2022

View reviewed changes

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

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

martinrrm requested a review from nkolev92 May 12, 2022 20:40

donnie-msft requested changes May 12, 2022

View reviewed changes

martinrrm changed the title ~~Warn for http when used in source commands and VS Package Source page~~ Warn for http when used in source commands May 17, 2022

kartheekp-ms reviewed May 20, 2022

View reviewed changes

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

jeffkl previously approved these changes May 24, 2022

View reviewed changes

donnie-msft previously approved these changes May 24, 2022

View reviewed changes

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

martinrrm dismissed stale reviews from donnie-msft and jeffkl via 335c38f May 26, 2022 20:37

martinrrm force-pushed the dev-martinrrm-warn-http-sources branch from 3c4d5d3 to 335c38f Compare May 26, 2022 20:37

martinrrm requested review from donnie-msft, erdembayar, jeffkl and kartheekp-ms May 26, 2022 20:43

martinrrm added 7 commits June 1, 2022 12:33

removed code from warning

69e6dc6

undo changes from UI

cb121bf

add check if source is not https

a20e41d

added more tests and improved message

cb18683

changed message and updated tests

b5891b7

removed empty line

e016c54

refactor tests

1492e08

martinrrm dismissed donnie-msft’s stale review via 1492e08 June 1, 2022 19:39

martinrrm force-pushed the dev-martinrrm-warn-http-sources branch from b5ad3c9 to 1492e08 Compare June 1, 2022 19:39

updated test

7baec42

martinrrm commented Jun 1, 2022

View reviewed changes

donnie-msft reviewed Jun 2, 2022

View reviewed changes

jeffkl previously approved these changes Jun 2, 2022

View reviewed changes

kartheekp-ms reviewed Jun 4, 2022

View reviewed changes

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

erdembayar reviewed Jun 4, 2022

View reviewed changes

kartheekp-ms reviewed Jun 4, 2022

View reviewed changes

pr comments

0fc4e8f

martinrrm dismissed jeffkl’s stale review via 0fc4e8f June 6, 2022 23:36

updated tests

580acbc

martinrrm force-pushed the dev-martinrrm-warn-http-sources branch from 4e07795 to 580acbc Compare June 7, 2022 14:40

martinrrm requested review from donnie-msft, erdembayar, jeffkl and kartheekp-ms June 7, 2022 16:55

jeffkl approved these changes Jun 7, 2022

View reviewed changes

erdembayar approved these changes Jun 7, 2022

View reviewed changes

martinrrm merged commit e243f3f into dev Jun 9, 2022

martinrrm deleted the dev-martinrrm-warn-http-sources branch June 9, 2022 18:08

heng-liu mentioned this pull request Aug 1, 2023

Opt-out of "HTTPS everywhere" : do not warn for HTTP sources when http sources are used in source command NuGet/Home#12791

Closed

	if (httpPackageSources != null && httpPackageSources.Count != 0)
	{
	if (httpPackageSources.Count == 1)
	{
	listPackageArgs.Logger.LogWarning(
	string.Format(CultureInfo.CurrentCulture,
	Strings.Warning_HttpServerUsage,
	"list package",
	httpPackageSources[0]));
	}
	else
	{
	listPackageArgs.Logger.LogWarning(
	string.Format(CultureInfo.CurrentCulture,
	Strings.Warning_HttpServerUsage_MultipleSources,
	"list package",
	Environment.NewLine + string.Join(Environment.NewLine, httpPackageSources.Select(e => e.Name))));
	}
	}

Conversation

martinrrm commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

nkolev92 commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zivkan commented Apr 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kartheekp-ms commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinrrm commented May 10, 2022

Uh oh!

nkolev92 commented May 10, 2022

Uh oh!

Uh oh!

Uh oh!

donnie-msft commented May 12, 2022

Uh oh!

nkolev92 commented May 12, 2022

Uh oh!

donnie-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkolev92 commented May 12, 2022

Uh oh!

donnie-msft commented May 12, 2022

Uh oh!

martinrrm commented May 12, 2022

Uh oh!

Uh oh!

Uh oh!

martinrrm Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

donnie-msft Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartheekp-ms Jun 4, 2022

Choose a reason for hiding this comment

Uh oh!

kartheekp-ms Jun 4, 2022

Choose a reason for hiding this comment

Uh oh!

donnie-msft Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kartheekp-ms Jun 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erdembayar Jun 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinrrm Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

kartheekp-ms Jun 4, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

martinrrm commented Apr 29, 2022 •

edited

Loading

nkolev92 commented Apr 29, 2022 •

edited

Loading

kartheekp-ms commented May 6, 2022 •

edited

Loading

donnie-msft Jun 2, 2022 •

edited

Loading

erdembayar Jun 4, 2022 •

edited

Loading