Adding support for pushing multiple packages #3739

marcin-krystianc · 2020-10-27T13:49:53Z

Bug

Fix

Details: Following the advice from the NuGet/Home#4393 (comment).
After this change, the push command takes one or more paths. So it still works as expected even when the dotnet nuget push *.nupkg command is run from the shell that supports globbing and *.nupkg is being expanded to multiple arguments.

Testing/Validation

Tests Added: Yes

src/NuGet.Core/NuGet.Protocol/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt

donnie-msft

Thanks for your contribution! I'll go ahead and run CI on your branch to get some early feedback until you can address my comments...

In #3140, I added tests to ensure wildcard of * works correctly when automatically pushing .snupkg's. I'd like some sort of functional test (or additional assertions) to make sure your PR doesn't regress that behavior.

Specifically, how will these scenarios fit with your change? (see my PR above)
PushCommand_Server_Nupkg_ByWildcard_SnupkgDoesNotExist_NoFileNotFoundError()
PushCommand_Server_Snupkg_ByFilename_DoesNotExist_FileNotFoundError()

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

donnie-msft · 2020-10-28T21:17:44Z

The dotnet formatting analyzers are failing for some of your changes. Try running dotnet format --check --exclude submodules to see them.

CI failed to build your changes. It looks like your [Obsolete] methods are the culprits:

##[error]test\NuGet.Core.Tests\NuGet.Protocol.Tests\Resources\PackageUpdateResourceTests.cs(810,23): Error CS0618: 'PackageUpdateResource.Push(string, string, int, bool, Func<string, string>, Func<string, string>, bool, bool, SymbolPackageUpdateResourceV3, ILogger)' is obsolete: 'Use Push method which takes multiple package paths.'

Did you run .\build.ps1 (see docs)? Curious if that somehow succeeded.

…push

marcin-krystianc · 2020-10-29T08:35:48Z

Did you run .\build.ps1 (see docs)? Curious if that somehow succeeded.

No, my mistake. But still it shouldn't happen as I was running PackageUpdateResourceTests anyway. I cannot explain how I missed it 🤷

marcin-krystianc

@donnie-msft I hope I addressed all your comments, can you have another look ?

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

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

donnie-msft

Thanks for contributing! Running CI...

donnie-msft · 2020-10-30T15:52:38Z

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

+                {
+                    var errorMsg = string.Format(MESSAGE_FILE_DOES_NOT_EXIST, snupkgsToPush[i]);
+
+                    // Only the first file that is missing should be mentioned in logs


erdembayar · 2020-10-30T17:15:39Z

src/NuGet.Core/NuGet.Commands/CommandRunners/PushRunner.cs

@@ -68,5 +69,36 @@ public static class PushRunner
                symbolPackageUpdateResource,
                logger);
        }
+
+        [Obsolete("Use Run method which takes multiple package paths.")]
+        public static Task Run(


I can't understand logic of adding public also same time obsolete method. If it's doesn't exist before then why bother to add now.

It looks wrong but only because of how the diff is being shown.
Actually my change is deprecating the previously existing method which takes string packagePath and adding a new one which takes IList<string> packagePaths.

erdembayar · 2020-10-30T17:16:29Z

src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs

                }
            }
        }

+        [Obsolete("Use Push method which takes multiple package paths.")]


Same, it looks wrong but only because of how the diff is being shown.

donnie-msft · 2020-10-30T18:28:27Z

Failing tests (on both Windows/Mac):

PushCommand_PushToServer_ApiKeyAsNamedArgument

Pushing testPackage1.1.1.0.nupkg to 'http://localhost:50231/8efafaf5-fe52-4a70-82f4-ed6e1a06e164/push'...\r\n PUT http://localhost:50231/8efafaf5-fe52-4a70-82f4-ed6e1a06e164/push/\r\n Created http://localhost:50231/8efafaf5-fe52-4a70-82f4-ed6e1a06e164/push/ 5ms\r\nYour package was pushed.\r\n File does not exist (should-be-ignored).\r\n\r\nExpected: True\r\nActual: False
Stack trace
at NuGet.CommandLine.Test.NuGetPushCommandTest.PushCommand_PushToServer_ApiKeyAsNamedArgument() in test\NuGet.Clients.Tests\NuGet.CommandLine.Test\NuGetPushCommandTest.cs:line 1653

PushCommand_PushToServer_ApiKeyAsThirdArgument

Pushing testPackage1.1.1.0.nupkg to 'http://localhost:50231/7fcbeab0-5495-42dc-acc7-ed78fc2f6e0d/nuget'...\r\n PUT http://localhost:50231/7fcbeab0-5495-42dc-acc7-ed78fc2f6e0d/nuget/\r\n Created http://localhost:50231/7fcbeab0-5495-42dc-acc7-ed78fc2f6e0d/nuget/ 23ms\r\nYour package was pushed.\r\n File does not exist (10ac98b0-2bbc-4d92-98ea-44d1439be955).\r\n\r\nExpected: True\r\nActual: False
Stack trace
at NuGet.CommandLine.Test.NuGetPushCommandTest.PushCommand_PushToServer_ApiKeyAsThirdArgument() in test\NuGet.Clients.Tests\NuGet.CommandLine.Test\NuGetPushCommandTest.cs:line 1578

marcin-krystianc · 2020-11-02T13:45:16Z

Failing tests (on both Windows/Mac):
PushCommand_PushToServer_ApiKeyAsNamedArgument
PushCommand_PushToServer_ApiKeyAsThirdArgument

Well, it is more complicated than I thought.
I've updated both NuGet.exe (NuGet.CommandLine) and dotnet NuGet (NuGet.CommandLine.XPlat).
It turns out though, that extending push command for nuget.exe is rather not possible due to problems with backwards compatibility. There is some special ApiKey logic, which for cases where ApiKey is not specified, it assumes that second parameter is an ApiKey (https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.CommandLine/Commands/PushCommand.cs#L51-L58)
This behaviour is also documented here https://docs.microsoft.com/en-gb/nuget/reference/cli-reference/cli-ref-push#examples.

I think I'll revert changes for NuGet.exe and stick only to the dotnet nuget push scenario. Will such lack of symmetry be ok ?

donnie-msft · 2020-11-02T19:39:37Z

I think I'll revert changes for NuGet.exe and stick only to the dotnet nuget push scenario. Will such lack of symmetry be ok ?

Yes, I've checked with the team and it's good to move forward with your dotnet changes. I'll create a follow-up issue for the nuget.exe breaking change ~6.0 timeframe.
Thanks for being attentive and flexible!

CLI changes forthcoming

- Adding NuGet.XPlat tests

marcin-krystianc

All right, I've reverted changes related to the nuget.exe.

marcin-krystianc · 2020-11-02T10:07:44Z

src/NuGet.Core/NuGet.Commands/CommandRunners/PushRunner.cs

@@ -68,5 +69,36 @@ public static class PushRunner
                symbolPackageUpdateResource,
                logger);
        }
+
+        [Obsolete("Use Run method which takes multiple package paths.")]
+        public static Task Run(


It looks wrong but only because of how the diff is being shown.
Actually my change is deprecating the previously existing method which takes string packagePath and adding a new one which takes IList<string> packagePaths.

marcin-krystianc · 2020-11-02T10:09:19Z

src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs

                }
            }
        }

+        [Obsolete("Use Push method which takes multiple package paths.")]


Same, it looks wrong but only because of how the diff is being shown.

zkat

LGTM! I'm really excited that we're adding this, and also that we're not holding up dotnet.exe in order to do it. We'll deal with NuGet.exe eventually. Thanks for the PR!

AraHaan · 2021-04-25T10:30:15Z

Does this also include shells that by default does not do file globbing when you do dotnet nuget push *.nupkg?

marcin-krystianc · 2021-04-26T07:14:24Z

@AraHaan NuGet can do globbing internally so your scenario was supported even before my change,

AraHaan · 2021-04-26T07:20:05Z

@marcin-krystianc I was making sure as I will need some way to replace the github action that pushes since it only allows pushing one and it is not feasible as I would like to move my workflows and everything from a .github folder to an repository named .github as I heard it could provide defaults for all repositories on an "organization" on github.

However not sure if repositories could override only specific files from that repository if they need to while using the rest from that repository if need be.

marcin-krystianc added 4 commits October 27, 2020 10:05

support for pushing multiple packages

743b618

timeout for all packages

d34327a

tests

d04721c

update obsolete messages

78e0b57

marcin-krystianc requested a review from a team as a code owner October 27, 2020 13:49

marcin-krystianc added 2 commits October 27, 2020 14:53

reduce diff noise

83ee173

Revert "reduce diff noise"

431065d

zivkan assigned donnie-msft Oct 27, 2020

zivkan added the Community PRs created by someone not in the NuGet team label Oct 27, 2020

zivkan reviewed Oct 27, 2020

View reviewed changes

src/NuGet.Core/NuGet.Protocol/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt Show resolved Hide resolved

donnie-msft requested changes Oct 28, 2020

View reviewed changes

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

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

donnie-msft added Approved for CI and removed Approved for CI labels Oct 28, 2020

marcin-krystianc added 3 commits October 29, 2020 07:59

Merge remote-tracking branch 'origin/dev' into 20201027-marcink-multi…

0441270

…push

update call sites

3bafbeb

addressing PR comments

23cc501

marcin-krystianc added 3 commits October 29, 2020 10:19

formatting

9c61f3d

add test for one or more pushed paths

e27ac2b

reduce the diff noise

00fd04b

marcin-krystianc commented Oct 29, 2020

View reviewed changes

donnie-msft requested changes Oct 29, 2020

View reviewed changes

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

marcin-krystianc added 2 commits October 30, 2020 12:35

testing push by filename and by wildcard

5d05094

Delete result.txt

003b5cb

donnie-msft previously approved these changes Oct 30, 2020

View reviewed changes

donnie-msft added the Approved for CI label Oct 30, 2020

erdembayar reviewed Oct 30, 2020

View reviewed changes

donnie-msft removed the Approved for CI label Oct 30, 2020

marcin-krystianc added 3 commits November 2, 2020 12:34

revert change for NuGet.exe

82f6d5e

Merge branch 'dev' into 20201027-marcink-multipush

a8003b7

test correct path

1e04079

donnie-msft mentioned this pull request Nov 2, 2020

nuget.exe push *.nupkg doesn't push more than one file NuGet/Home#10217

Open

marcin-krystianc added 2 commits November 3, 2020 06:55

- Reverting NuGet.CommandLine tests

3b00b21

- Adding NuGet.XPlat tests

- Reverting NuGet.CommandLine tests

535ae3b

- Adding NuGet.XPlat tests

marcin-krystianc commented Nov 3, 2020

View reviewed changes

donnie-msft approved these changes Nov 3, 2020

View reviewed changes

donnie-msft added the Approved for CI label Nov 3, 2020

donnie-msft requested review from zkat and dominoFire November 3, 2020 19:56

zkat approved these changes Nov 4, 2020

View reviewed changes

donnie-msft merged commit fa9fbcb into NuGet:dev Nov 4, 2020

donnie-msft added Approved for CI and removed Approved for CI labels Nov 16, 2020

marcin-krystianc deleted the 20201027-marcink-multipush branch December 15, 2020 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for pushing multiple packages #3739

Adding support for pushing multiple packages #3739

marcin-krystianc commented Oct 27, 2020

donnie-msft left a comment

donnie-msft commented Oct 28, 2020

marcin-krystianc commented Oct 29, 2020

marcin-krystianc left a comment

donnie-msft left a comment

donnie-msft Oct 30, 2020

erdembayar Oct 30, 2020

marcin-krystianc Nov 2, 2020

erdembayar Oct 30, 2020

marcin-krystianc Nov 2, 2020

donnie-msft commented Oct 30, 2020

marcin-krystianc commented Nov 2, 2020

donnie-msft commented Nov 2, 2020

marcin-krystianc left a comment

marcin-krystianc Nov 2, 2020

marcin-krystianc Nov 2, 2020

zkat left a comment

AraHaan commented Apr 25, 2021

marcin-krystianc commented Apr 26, 2021

AraHaan commented Apr 26, 2021

Adding support for pushing multiple packages #3739

Adding support for pushing multiple packages #3739

Conversation

marcin-krystianc commented Oct 27, 2020

Bug

Fix

Testing/Validation

donnie-msft left a comment

Choose a reason for hiding this comment

donnie-msft commented Oct 28, 2020

marcin-krystianc commented Oct 29, 2020

marcin-krystianc left a comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

donnie-msft Oct 30, 2020

Choose a reason for hiding this comment

erdembayar Oct 30, 2020

Choose a reason for hiding this comment

marcin-krystianc Nov 2, 2020

Choose a reason for hiding this comment

erdembayar Oct 30, 2020

Choose a reason for hiding this comment

marcin-krystianc Nov 2, 2020

Choose a reason for hiding this comment

donnie-msft commented Oct 30, 2020

PushCommand_PushToServer_ApiKeyAsNamedArgument

PushCommand_PushToServer_ApiKeyAsThirdArgument

marcin-krystianc commented Nov 2, 2020

donnie-msft commented Nov 2, 2020

marcin-krystianc left a comment

Choose a reason for hiding this comment

marcin-krystianc Nov 2, 2020

Choose a reason for hiding this comment

marcin-krystianc Nov 2, 2020

Choose a reason for hiding this comment

zkat left a comment

Choose a reason for hiding this comment

AraHaan commented Apr 25, 2021

marcin-krystianc commented Apr 26, 2021

AraHaan commented Apr 26, 2021