Add dotnet nuget apikey commands#7322
Conversation
dotnet-policy-service
Bot
added
the
Community
Saibamen
changed the title
dotnet nuget apikey commands
| Strings.Option_ConfigFile, | ||
| CommandOptionType.SingleValue); | ||
|
|
||
| CommandArgument apiKeyArgument = set.Argument( |
There was a problem hiding this comment.
Let's add a verbosity option here too.
| { | ||
| public static void Register(CommandLineApplication app, Func<ILogger> getLogger) | ||
| { | ||
| app.Command("apikey", apiKey => |
There was a problem hiding this comment.
this is great work, please create the spec where you enumerate all the options and follow the template.
We should be able to use that design for documentation generation and historical reasons.
There was a problem hiding this comment.
The PR description links to NuGet/Home#12321, which is a spec I forgot that I wrote years ago. I stopped tracking this dotnet nuget apikey command because I think #7087 is a better solution and expect that people want dotnet nuget apikey because they're not aware of the NUGET_API_KEY environment variable, rather than this new command enabling some scenario that is too difficult given NUGET_API_KEY is available. But the point is that there is a spec for this command.
Since the NUGET_API_KEY environment variable exists, I think the spec's -e and -f options are not needed. However, I don't see any justification in this PR why the spec was not followed.
But since the spec was merged in 2023, almost exactly 3 years ago, if we go ahead with this command, we should at least run it by the dotnet CLI PMs to see if they have feedback on the command names and structure. Make sure that the best practices and recommendations for command design haven't changed since the spec was written.
There was a problem hiding this comment.
Actually, the NUGET_API_KEY environment variable looks to be new to the .NET 10.0.300 SDK, so not even out yet. Once it ships I wonder how much demand there will be for a dotnet nuget apikey command.
There was a problem hiding this comment.
I missed the linked spec to be honest. There were a bunch of PRs with a missing spec, so I actually missed the one that did have it :D
I agree with your general sentiment.
Let's look at the spec again and drive to a final solution.
Sounds like we have 2 (?) options?
- Spec is re-reviewed and updated and is completely followed in this implementation.
- Spec/PR and issue are all updated saying NUGET_API_KEY should be used.
| @@ -0,0 +1,177 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | |||
|
|
|||
There was a problem hiding this comment.
All these tests run the runner, so integration and that's great, but I'd like to have at least 1 test that actually runs dotnet nuget apikey set or whatever the order actually is :D
You'd follow https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/ListPackageTests.cs.
We're in the process of changing up the NuGet/SDK CLI integration in how these things are set-up, so you'd need to make sure this integration is being added the same way the dotnet nuget why integration was added.
| { | ||
| public static void Register(CommandLineApplication app, Func<ILogger> getLogger) | ||
| { | ||
| app.Command("apikey", apiKey => |
There was a problem hiding this comment.
The PR description links to NuGet/Home#12321, which is a spec I forgot that I wrote years ago. I stopped tracking this dotnet nuget apikey command because I think #7087 is a better solution and expect that people want dotnet nuget apikey because they're not aware of the NUGET_API_KEY environment variable, rather than this new command enabling some scenario that is too difficult given NUGET_API_KEY is available. But the point is that there is a spec for this command.
Since the NUGET_API_KEY environment variable exists, I think the spec's -e and -f options are not needed. However, I don't see any justification in this PR why the spec was not followed.
But since the spec was merged in 2023, almost exactly 3 years ago, if we go ahead with this command, we should at least run it by the dotnet CLI PMs to see if they have feedback on the command names and structure. Make sure that the best practices and recommendations for command design haven't changed since the spec was written.
| public class ApiKeyCommandTests | ||
| { | ||
| private const string SourceName = "contoso"; | ||
| private const string SourceUrl = "https://nuget.contoso.org/v3/index.json"; |
There was a problem hiding this comment.
Tests should use domains ending in .test, since that's a reserved TLD and basically eliminates a test from accidentally opening HTTP or TCP connections to remote hosts. We have many old tests that don't follow this best practise, but for new tests I'd like to see it done.
dotnet-policy-service
Bot
added
the
Status:No recent activity
|
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 30 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. |
3 participants
Summary
dotnet nuget apikey setanddotnet nuget apikey unsetto persist and remove API keys for package sources.--configfilesupport.Fixes NuGet/Home#6437
Spec: NuGet/Home#12321
Test plan
dotnet test test\NuGet.Core.Tests\NuGet.CommandLine.Xplat.Tests\NuGet.CommandLine.Xplat.Tests.csproj --filter ApiKeydotnet format whitespace --verify-no-changes NuGet.sln --include "src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/ApiKeyCommand.cs" "src/NuGet.Core/NuGet.CommandLine.XPlat/Program.cs" "test/NuGet.Core.Tests/NuGet.CommandLine.Xplat.Tests/ApiKeyCommandTests.cs"dotnet test test\NuGet.Core.Tests\NuGet.Commands.Test\NuGet.Commands.Test.csproj --filter Pushpartially passed:net10.0push tests passed;net472host failed before matching tests due to strong-name validation ofMicrosoft.Internal.NuGet.Testing.SignedPackages.Notes
DependencyGraphResolver.cs.