Add --config-file param to dotnet nuget push command #4819

aortiz-msft · 2022-09-24T18:22:21Z

Bug

Regression? Last working version: No.

Description

Adding support for the --config-file option in dotnet nuget push.

As part of this change, adding automated test coverage for the --config-file (-configfile) option in both NuGet.exe and dotnet.exe as follows:

NuGetPackCommandTests now has test coverage for the -configfile option in NuGet.exe.
XPlatPushTests validates the new --config-file option in dotnet.exe. Because the implementation in NuGet.exe and dotnet.exe is shared (see PushRunner.cs), only added an additional test for an error message that differs in both executables.
TODO in this PR: Moved the CreateFile utility method in NuGet.CommandLine.Tests/Utils.cs to a project that is shared between both test suites, Test.Utility/TestFileSystemUtility.cs. (Note: I will do this once there is agreement on whether the TestFileSystemUtility class is appropriate or a new class.)

Also:

Improved performance of existing tests in XPlatPushTests by avoiding instantiation and enumeration of a List. Instead, test params are added directly to a string array which is used by the execution method.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- TODO: Manual ad-hoc testing (in progress)
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled: https://github.com/NuGet/docs.microsoft.com-nuget/issues/2898
- OR
- N/A

aortiz-msft · 2022-09-24T18:29:02Z

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

+                                string.Join(" ", args),
+                                true);
+
+                // TODO: The bad config file is ignored, so I need to confirm this is expected behavior.


@zivkan , @nkolev92 - Any input here?

As mentioned in chat, it does feel like a bug. My first guess would have been that the config file isn't actually being read, but then I'd expect the next test, PushCommand_ConfigFile_ExpectedFormat, to fail.

Anyway, this PR clearly didn't introduce that behaviour, so we should create a new issue, and skip this test until that other issue is fixed.

src/NuGet.Clients/NuGet.CommandLine/NuGetCommand.resx

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

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatPushTests.cs

kartheekp-ms · 2022-09-27T03:01:56Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/PushCommand.cs

@@ -105,7 +110,9 @@ public static void Register(CommandLineApplication app, Func<ILogger> getLogger)
                    }

 #pragma warning disable CS0618 // Type or member is obsolete
-                    var sourceProvider = new PackageSourceProvider(XPlatUtility.GetSettingsForCurrentWorkingDirectory(), enablePackageSourcesChangedEvent: false);
+                    var sourceProvider = new PackageSourceProvider(
+                        XPlatUtility.ProcessConfigFile(configurationFile.Value()),


I think we should add a validation to ensure file path exists before accessing it.

Also, does XPlatUtility.ProcessConfigFile fall back to GetSettingsForCurrentWorkingDirectory when configurationFile.Value() is null or empty (was not provided on the command line)?

This code, the way it's written, makes it look like a breaking change and that passing the --config-file is becoming mandatory. edit: reading the tests, it appears that it does maintain backwards compatibility. It's just not obvious to me.

zivkan · 2022-09-27T04:26:02Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/PushCommand.cs

@@ -105,7 +110,9 @@ public static void Register(CommandLineApplication app, Func<ILogger> getLogger)
                    }

 #pragma warning disable CS0618 // Type or member is obsolete
-                    var sourceProvider = new PackageSourceProvider(XPlatUtility.GetSettingsForCurrentWorkingDirectory(), enablePackageSourcesChangedEvent: false);
+                    var sourceProvider = new PackageSourceProvider(
+                        XPlatUtility.ProcessConfigFile(configurationFile.Value()),


Also, does XPlatUtility.ProcessConfigFile fall back to GetSettingsForCurrentWorkingDirectory when configurationFile.Value() is null or empty (was not provided on the command line)?

This code, the way it's written, makes it look like a breaking change and that passing the --config-file is becoming mandatory. edit: reading the tests, it appears that it does maintain backwards compatibility. It's just not obvious to me.

zivkan · 2022-09-27T04:48:26Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/PushCommand.cs

@@ -36,6 +36,11 @@ public static void Register(CommandLineApplication app, Func<ILogger> getLogger)
                    Strings.SymbolSource_Description,
                    CommandOptionType.SingleValue);

+                var configurationFile = push.Option(
+                    "--config-file <source>",


It looks like most other dotnet commands use --configfile, not --config-file

NuGet.Client/src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Commands.xml

Line 14 in 451713c

<SingleValueOption Name="configfile" Help="Option_ConfigFile" />

https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-restore?tabs=netcore2x#options
https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-add-source#options

zivkan · 2022-09-27T04:54:38Z

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

@@ -34,7 +40,7 @@ public void PushCommand_PushToV2FileSystemSource()
                var packageFileName = Util.CreateTestPackage("testPackage1", "1.1.0", packageDirectory);

                // Act
-                string[] args = new string[] { "push", packageFileName, "-Source", source };
+                string[] args = new string[] { Command_Push, packageFileName, Param_Source, source };


I'm sorry, but I don't find this easier. Command_Push is many more characters than just "push", similar for Param_Source. And given they're in constants, my gut feeling is that this means it's a non-obvious value. Like, maybe Command_Push is maybe not just "push", so now I feel obligated to go to the top of the file to find out what it is.

I get this this could be useful if we ever rename push to something else, then it's just a single place to change for all the tests in this file. However, renaming it would be a breaking change that I just don't see happening. So, my opinion is that we optimize for readability, which in my opinion, means prefer the string literals. But this is 100% opinionated.

zivkan · 2022-09-27T04:56:55Z

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

@@ -2078,6 +2084,339 @@ public void PushCommand_PushToServerV3_WithSymbols_ApiKey_SymbolApiKey_BothFromC
            }
        }

+        [Theory]
+        [InlineData(">")]


If there's only a single InlineData, I think we may as well just use [Fact] instead of [Theory].

zivkan · 2022-09-27T05:03:30Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/PushCommand.cs

@@ -36,6 +36,11 @@ public static void Register(CommandLineApplication app, Func<ILogger> getLogger)
                    Strings.SymbolSource_Description,
                    CommandOptionType.SingleValue);

+                var configurationFile = push.Option(
+                    "--config-file <source>",


Suggested change

"--config-file <source>",

"--config-file <file>",

In NuGet vocabulary, a source is a feed of packages. I don't know if this was a copy-paste issue, but source really doesn't feel right to me. I don't know if we should say path instead of file.

dotnet restore -h says --configile <FILE>, while dotnet list package -h says --config <CONFIG_FILE>. Clear as mud. As consistent as flipping a coin.

Just fyi --configfile <File> is used for dotnet nuget trust.

zivkan · 2022-09-27T05:16:52Z

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

+                                string.Join(" ", args),
+                                true);
+
+                // TODO: The bad config file is ignored, so I need to confirm this is expected behavior.


As mentioned in chat, it does feel like a bug. My first guess would have been that the config file isn't actually being read, but then I'd expect the next test, PushCommand_ConfigFile_ExpectedFormat, to fail.

Anyway, this PR clearly didn't introduce that behaviour, so we should create a new issue, and skip this test until that other issue is fixed.

zivkan · 2022-09-27T05:20:19Z

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

+                FileSystemUtils.CreateFile(
+                    configFilePath,
+                    configFileName,
+                    string.Format(
+                        CultureInfo.InvariantCulture,


I dislike nested method calls, because it's harder to read (where is the end ) for string.Format? Are there any additional parameters to FileSystemUtils.CreateFile, or just the 3? Also, when debugging, maybe I want to see the result of string.Format without stepping into the outer method call. If string.Format was a method from our own code, rather than the BCL (or if I disabled "Just My Code" in the debugger), then it's also harder to step into FileSystemUtils.CreateFile without stepping into the nested method call.

Therefore, I suggest extracting the string.Format into a variable, and then have a simple method call to FileSystemUtils.CreateFile.

zivkan · 2022-09-27T05:26:54Z

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

@@ -310,6 +310,7 @@ public static string CreateUAPProject(string directory, string projectJsonConten
            return projectFile;
        }

+        // TODO: Remove CreateFile methods from this utility class. Will require changes to all classes in NuGet.CommandLine.Test.


coding guidelines number 22: https://github.com/NuGet/NuGet.Client/blob/dev/docs/coding-guidelines.md

Adding TODOs is not recommended and should be avoided whenever possible. In cases in which they do get added each TODO needs to be linked to an issue. Use the full url to the GitHub issue.

zivkan · 2022-09-27T05:27:56Z

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatPushTests.cs

+        private const string Command_Push = "push";
+        private const string Param_ApiKey = "--api-key";
+        private const string Param_ConfigFile = "--config-file";
+        private const string Param_Source = "--source";


same comments as the other file. My personal opinion is that this does not aid readability, and the risk of any of these is so low that putting them all in a constant is not worth the reduction in readability.

zivkan · 2022-09-27T05:30:29Z

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatPushTests.cs

        [PackageSourceTheory]
        [PackageSourceData(TestSources.MyGet)]
        [PackageSourceData(TestSources.ProGet, Skip = "No such host is known")]
        [PackageSourceData(TestSources.Klondike, Skip = "401 (Invalid API key)")]
        [PackageSourceData(TestSources.NuGetServer, Skip = "No such host is known")]
        public async Task PushToServerSucceeds(PackageSource packageSource)
        {
-            // Arrange
+            // Setup


https://github.com/NuGet/NuGet.Client/blob/dev/docs/coding-guidelines.md#unit-test-structure

We use Arrange, Act, Assert as the coding guidelines unit test structure. This is known as the AAA pattern. Perhaps not well known (I never heard of it before joining NuGet), but doing a web search in multiple search engines finds many results, so I can't claim it's made up just for NuGet.

donnie-msft

Great to see push improvements! I'm blocking on the comments/upvotes I put on coding guideline violations and several code changes or refactorings.

donnie-msft · 2022-09-27T17:41:31Z

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

@@ -70,7 +76,7 @@ public void PushCommand_PushToV3FileSystemSource()
                var packageFileName = Util.CreateTestPackage(packageId, version, packageDirectory);

                // Act
-                string[] args = new string[] { "push", packageFileName, "-Source", source };
+                string[] args = new string[] { Command_Push, packageFileName, Param_Source, source };


As others have commented, I don't think introducing variables helps readability or maintainability, so should be reverted.

donnie-msft · 2022-09-27T17:44:09Z

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatPushTests.cs

+
+            Assert.True(
+                exitCode != 0,
+                "The run did not fail as desired. Simply got this output:" + log.ShowMessages());


Another area where I'm not sure adding strings is helpful.

Suggested change

"The run did not fail as desired. Simply got this output:" + log.ShowMessages());

log.ShowMessages());

ghost · 2022-10-04T19:01:30Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

aortiz-msft requested a review from a team as a code owner September 24, 2022 18:22

aortiz-msft commented Sep 24, 2022

View reviewed changes

aortiz-msft requested a review from zivkan September 26, 2022 05:32

jeffkl previously approved these changes Sep 26, 2022

View reviewed changes

src/NuGet.Clients/NuGet.CommandLine/NuGetCommand.resx Outdated Show resolved Hide resolved

erdembayar reviewed Sep 26, 2022

View reviewed changes

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

aortiz-msft dismissed jeffkl’s stale review via 3061517 September 26, 2022 22:08

erdembayar reviewed Sep 26, 2022

View reviewed changes

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatPushTests.cs Show resolved Hide resolved

aortiz-msft force-pushed the origin/dev-aortiz-pushcommandconfigfile branch from 3061517 to 4b2d8b6 Compare September 26, 2022 22:34

aortiz-msft added 2 commits September 26, 2022 17:37

Implement --config-file param in dotnet.exe

8f1559b

remove empty line

ef202fd

aortiz-msft force-pushed the origin/dev-aortiz-pushcommandconfigfile branch from 1fd1699 to ef202fd Compare September 27, 2022 00:41

kartheekp-ms reviewed Sep 27, 2022

View reviewed changes

zivkan reviewed Sep 27, 2022

View reviewed changes

donnie-msft requested changes Sep 27, 2022

View reviewed changes

ghost 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 Oct 4, 2022

ghost closed this Oct 11, 2022

nkolev92 deleted the origin/dev-aortiz-pushcommandconfigfile branch February 23, 2023 00:06

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --config-file param to dotnet nuget push command #4819

Add --config-file param to dotnet nuget push command #4819

aortiz-msft commented Sep 24, 2022 •

edited

Loading

aortiz-msft Sep 24, 2022

zivkan Sep 27, 2022

kartheekp-ms Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

erdembayar Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

zivkan Sep 27, 2022

donnie-msft left a comment

donnie-msft Sep 27, 2022

donnie-msft Sep 27, 2022

ghost commented Oct 4, 2022

	"The run did not fail as desired. Simply got this output:" + log.ShowMessages());
	log.ShowMessages());

Add --config-file param to dotnet nuget push command #4819

Add --config-file param to dotnet nuget push command #4819

Conversation

aortiz-msft commented Sep 24, 2022 • 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

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

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

ghost commented Oct 4, 2022

aortiz-msft commented Sep 24, 2022 •

edited

Loading