2148: Add dotnet nuget push command and arguments #2229

atlemann · 2018-12-21T19:05:03Z

Add dotnet nuget push command

matthid · 2019-01-10T11:40:41Z

Question is if we can somehow re-use existing code. Similar to how we did in msbuild. Basically dotnet msbuild is CLI equivalent to msbuild.exe. The same is true for dotnet nuget and nuget.exe.... Ideally we update them together and not have to fix bugs twice...

atlemann · 2019-01-17T07:46:32Z

Yes, it would be nice to share some code. I can't seem to find what code is shared between msbuild and dotnet msbuild though.

matthid · 2019-01-29T18:53:59Z

Fake.DotNet.Cli references Fake.DotNet.MSBuild: https://github.com/fsharp/FAKE/blob/e7e562866510351452427c3a13833fd4d6bf9f89/src/app/Fake.DotNet.Cli/Fake.DotNet.Cli.fsproj#L25

And some refactoring was done to use a common structure (in order to convert between old and new structure and have a single logic):
https://github.com/fsharp/FAKE/blob/e7e562866510351452427c3a13833fd4d6bf9f89/src/app/Fake.DotNet.MSBuild/MSBuild.fs#L326-L419

I'm not sure if this is viable approach for NuGet. Honestly I'm not all to happy with the NuGet module API...

Release 5.12.1

…nn/FAKE into 2148-add.dotnet.nuget.push.delete

…uget.push.delete

atlemann · 2019-02-22T22:36:24Z

I'm trying to figure out if anything can be shared. The NuGet.exe module combines pack and push in the same arguments. However, NuGet pack doesn't build the project like dotnet pack does, so if we want to do something similar for the DotNet.Cli module, we would also need the CliArguments.

atlemann · 2019-02-22T22:37:32Z

The Process.withFramework also makes reuse of the NuGet publish hard, since the whole point is not to use Mono.

https://github.com/fsharp/FAKE/blob/2ec240f1d954500dab852bb4aacb6bd1b49b4e1e/src/app/Fake.DotNet.NuGet/NuGet.fs#L344

atlemann · 2019-02-22T22:54:12Z

But I guess we could split the pack and push specific parts of the current NuGetParams into seperate records and have some conversion between the current NuGetParams (like for msbuild) and a new one which holds the pack and push ones? Then they would reuse the same records at least.

atlemann · 2019-02-22T23:09:22Z

Maybe we need to update DotNet.Cli.PackOptions to hold some that new pack specific record as well then. Like Authors, Tags, Description etc.

…with DotNetCli nuget push. Add new push function which takes this record instead of NuGetParams.

…li args. Make push function usable from DotNet.Cli module. Remove the old publish function.

atlemann · 2019-03-02T21:45:52Z

@matthid Not sure I'm going in the right direction here, but at least most of the code is shared between the two modules.

Another thing, the NuGet.publishSymbols function seems to call publish if it fails and not itself, is that a bug?

…ommon Options. Rename options in NuGet module to NuGetPushParams.

matthid

Because I'm lacking a better approach, I think the one proposed in this PR is fine. I have added some detail comments which we need to address/answer

matthid · 2019-03-16T18:19:01Z

src/app/Fake.DotNet.NuGet/NuGet.fs

@@ -39,6 +39,18 @@ type NugetSymbolPackage =
    /// Build a symbol package using the nuspec file
    | Nuspec = 2

+type ToolOptions =


Can you add short docs for this or mark it internal/private?

I'll make it internal, since I'm not reusing it in the DotNet module anymore.

matthid · 2019-03-16T18:22:31Z

src/app/Fake.DotNet.NuGet/NuGet.fs

+        param.Timeout |> Option.map string |> Option.map (sprintf "-Timeout %s")
+    ]
+    |> List.choose id
+    |> String.concat " "


Can you use the proper APIs for this (Arguments.ofList for example)?

matthid · 2019-03-16T18:28:50Z

src/app/Fake.DotNet.NuGet/NuGet.fs

+
+module Private =
+    /// For internal use
+    let rec push (options : ToolOptions) (parameters : NuGetPushParams) (toCliArgs : NuGetPushParams -> string) nupkg =


So this only currently handles dotnet nuget push we should think about other commands as well.

Do we add all operations by adding new functions one by one on demand?

Should we use a similar model as dotnet uses where you have a "low"-level DotNet.exec function where others like dotnet test build on top.

Something different?

Personally I'd lean to something along the second option, but if there are good reasons to do it differently I'm all ears. Note that I'm not proposing to implement all options in this PR, but whatever we decide to release should be extendible later.

The second option sounds like a good idea so I'll not reuse this in the DotNet module, but rather use the exec in there instead. I'll for now make this NuGet.Private.push internal instead and maybe it could be generalized to be the low level one?

matthid · 2019-03-16T18:30:04Z

src/app/Fake.DotNet.NuGet/NuGet.fs

+
+        try
+            let result =
+                let tracing = Process.shouldEnableProcessTracing()


If we really want to disable tracing we should use the corresponding CreateProcess functionality. We should remove this class to Process.shouldEnableProcessTracing to reduce side effects.

Right, I just copied some other code in the same module. I can just remove it I guess.

matthid · 2019-03-16T18:35:17Z

Sorry for the late reply.

But I guess we could split the pack and push specific parts of the current NuGetParams into seperate records and have some conversion between the current NuGetParams (like for msbuild) and a new one which holds the pack and push ones?

I think I have answered this in the comment regarding which API surface we want, we could try a similar approach as dotnet itself but we need good documentation as the syntax will become a bit messy (to edit nested record fields)

Another thing, the NuGet.publishSymbols function seems to call publish if it fails and not itself, is that a bug?

To be honest I don't know. I'm not really using this module myself :)

Make NuGet.Private.push private Use DotNet.exec for DotNet.nugetPush instead of NuGet.Private.push.

matthid · 2019-03-17T17:18:33Z

Changes look reasonable, is there anything left as this is still marked WIP?

atlemann · 2019-03-17T20:50:19Z

Not really, I guess I was to tired to notice 😁

atlemann · 2019-03-17T20:52:44Z

How do I edit the title from a phone...hmmm...

atlemann · 2019-03-17T20:56:04Z

I kind of feel the NuGet module is obsolete now that there is dotnet CLI. But I guess it might be a bit drastic to deprecate it?

matthid · 2019-03-17T22:33:51Z

But I guess it might be a bit drastic to deprecate it?

Indeed, but we could do it eventually. Sorry, just one more request, can we add a simple test for this (ie similar to others a single test checking if command line generation is OK)?

atlemann · 2019-03-18T06:33:19Z

Yes, I can add a test.

- Add nupkg to arguments - Convert timeout TimeSpan to seconds

atlemann · 2019-03-20T21:31:29Z

Added some tests and fixed some bugs. I also added retry like in the NuGet module.

matthid

Thanks again. One more minor nit (but I can do that myself once I'm ready to merge this in a couple of days, so don't worry):
I think we should add a bit of docs for NuGetPushOptions and a single example changing some parameters on top of the DotNet.nugetPush function (just like we have for the DotNet.msbuild function), because people might not realize how to change nested records in F#.

Add dotnet nuget push command and arguments

c4afb75

atlemann changed the title ~~[WIP] 2148: Add dotnet nuget push command and arguments~~ 2148: Add dotnet nuget push command and arguments Dec 28, 2018

matthid changed the base branch from master to release/next January 12, 2019 16:22

matthid added the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Feb 14, 2019

matthid and others added 3 commits February 15, 2019 14:26

Merge pull request fsprojects#2234 from fsharp/release/next

ab77b6a

Release 5.12.1

Merge branch '2148-add.dotnet.nuget.push.delete' of github.com:atlema…

d4eae30

…nn/FAKE into 2148-add.dotnet.nuget.push.delete

Merge remote-tracking branch 'upstream/master' into 2148-add.dotnet.n…

7170367

…uget.push.delete

matthid removed the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Feb 26, 2019

atlemann changed the title ~~2148: Add dotnet nuget push command and arguments~~ [WIP] 2148: Add dotnet nuget push command and arguments Feb 28, 2019

atlemann added 5 commits March 1, 2019 00:16

[WIP] Add NugetPushOptions record in NuGet module that can be shared …

7024c21

…with DotNetCli nuget push. Add new push function which takes this record instead of NuGetParams.

Use the NuGet module's NugetPushOptions in DotNet.Cli module

3dc8147

Remove addOnExited and get ExitCode straight from ProcessResult

4820c2f

Added function for converting a NugetPushOptions to NuGet.exe style C…

eaaf2f1

…li args. Make push function usable from DotNet.Cli module. Remove the old publish function.

Use the NuGet.push function from DotNet.Cli module.

b480598

Make a NuGetPushOptions in DotNet.Cli module with also contains the c…

7fcbd65

…ommon Options. Rename options in NuGet module to NuGetPushParams.

matthid requested changes Mar 16, 2019

View reviewed changes

Make NuGet.ToolOptions internal

500272d

Make NuGet.Private.push private Use DotNet.exec for DotNet.nugetPush instead of NuGet.Private.push.

matthid self-requested a review March 17, 2019 17:17

atlemann changed the title ~~[WIP] 2148: Add dotnet nuget push command and arguments~~ 2148: Add dotnet nuget push command and arguments Mar 17, 2019

matthid previously approved these changes Mar 17, 2019

View reviewed changes

Fix bugs and add unit-tests.

026954c

- Add nupkg to arguments - Convert timeout TimeSpan to seconds

atlemann dismissed matthid’s stale review via 026954c March 20, 2019 21:01

atlemann added 2 commits March 20, 2019 22:07

Use Expect.isEmpty

abf2615

Retry DotNet.nugetPush PushTrials number of times

45a26d1

matthid approved these changes Mar 21, 2019

View reviewed changes

matthid merged commit c38a472 into fsprojects:release/next Apr 4, 2019

matthid mentioned this pull request Apr 8, 2019

Release 5.13.0 #2279

Merged

matthid mentioned this pull request Apr 24, 2019

Invalid syntax of NuGet push after 5.13.0 #2299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2148: Add dotnet nuget push command and arguments #2229

2148: Add dotnet nuget push command and arguments #2229

atlemann commented Dec 21, 2018

matthid commented Jan 10, 2019

atlemann commented Jan 17, 2019

matthid commented Jan 29, 2019

atlemann commented Feb 22, 2019

atlemann commented Feb 22, 2019

atlemann commented Feb 22, 2019

atlemann commented Feb 22, 2019

atlemann commented Mar 2, 2019

matthid left a comment

matthid Mar 16, 2019

atlemann Mar 16, 2019

matthid Mar 16, 2019

matthid Mar 16, 2019

atlemann Mar 16, 2019

matthid Mar 16, 2019

atlemann Mar 16, 2019

matthid commented Mar 16, 2019

matthid commented Mar 17, 2019

atlemann commented Mar 17, 2019

atlemann commented Mar 17, 2019

atlemann commented Mar 17, 2019

matthid commented Mar 17, 2019

atlemann commented Mar 18, 2019

atlemann commented Mar 20, 2019

matthid left a comment

2148: Add dotnet nuget push command and arguments #2229

2148: Add dotnet nuget push command and arguments #2229

Conversation

atlemann commented Dec 21, 2018

matthid commented Jan 10, 2019

atlemann commented Jan 17, 2019

matthid commented Jan 29, 2019

atlemann commented Feb 22, 2019

atlemann commented Feb 22, 2019

atlemann commented Feb 22, 2019

atlemann commented Feb 22, 2019

atlemann commented Mar 2, 2019

matthid left a comment

Choose a reason for hiding this comment

matthid Mar 16, 2019

Choose a reason for hiding this comment

atlemann Mar 16, 2019

Choose a reason for hiding this comment

matthid Mar 16, 2019

Choose a reason for hiding this comment

matthid Mar 16, 2019

Choose a reason for hiding this comment

atlemann Mar 16, 2019

Choose a reason for hiding this comment

matthid Mar 16, 2019

Choose a reason for hiding this comment

atlemann Mar 16, 2019

Choose a reason for hiding this comment

matthid commented Mar 16, 2019

matthid commented Mar 17, 2019

atlemann commented Mar 17, 2019

atlemann commented Mar 17, 2019

atlemann commented Mar 17, 2019

matthid commented Mar 17, 2019

atlemann commented Mar 18, 2019

atlemann commented Mar 20, 2019

matthid left a comment

Choose a reason for hiding this comment