Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spec of dotnet nuget config command #12172

Merged
merged 30 commits into from
Jan 4, 2023
Merged
Changes from 27 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
366 changes: 366 additions & 0 deletions proposed/2022/dotnet-nuget-config-spec.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,366 @@
# NuGet configuration CLI for dotnet.exe

- Author: [Heng Liu](https://github.com/heng-liu)
- GitHub Issue [8420](https://github.com/NuGet/Home/issues/8420)

## Problem Background

Currently, there is no NuGet configuration CLI for dotnet.exe. It's inconvenient for NuGet users to know about the NuGet configuration file locations and figure out where is the merged configuration coming from.

## Who are the customers

This feature is for dotnet.exe users.

## Goals
Design and implement `dotnet nuget config` command.

## Non-Goals
Design and implement in the same way for NuGet.exe command.

## Solution
The following command will be implemented in the `dotnet.exe` CLI.
Comment on lines +20 to +21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish GitHub had threaded/resolvable conversations for comments without a specific file/line number 😔

I'd like to know what the design is for error scenarios, specifically when one or more of the XML files contains syntax errors (from an implementation point of view, this means that Settings.Load* apis will fail, so we can't use loadedConfig.GetConfigFilePaths()).

Is dotnet nuget config list going to report an error and fail to list all the files? Will it at least tell us the filename of the XML that couldn't be parsed? Or will all the XML files be listed despite the parsing failure? Will it tell us which file(s) contain parsing errors?

Copy link
Contributor Author

@heng-liu heng-liu Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question!
I tested with NuGet.exe and dotnet.exe, and both of them are able to tell us the filename of the XML that couldn't be parsed, as below:

nuget list source
NuGet.Config is not valid XML. Path: 'C:\Users\username\Source\Repos\NuGet.Config'.
  The 'disabledPackageSources' start tag on line 19 position 4 does not match the end tag of 'configuration'. Line 20, position 3.
dotnet nuget list source
error: NuGet.Config is not valid XML. Path: 'C:\Users\username\Source\Repos\NuGet.Config'.
error:   The 'disabledPackageSources' start tag on line 19 position 4 does not match the end tag of 'configuration'. Line 20, position 3.

So I think the dotnet nuget config list is able to do this as well.

If we'd like to list all config files despite the parsing failure, we'd consider if it worth doing.
Since running all nuget commands will encounter this invalid XML error, users need to fix this invalid XML before running any NuGet commands. So this seems to be a prerequisite and there is no harm if user needs to do this first before knowing the list of config files.
In addition, this error message clearly points out the file path and location, so it should be easy for users to correct the invalid XML.

The only scenario needs to be considered is, with the invalid XML error, user should able to specifying a specific correct config file to run nuget commands. And if we list all config file locations despite the parsing failure, it might help people to find the path easier. But it seems "listing all config file locations despite the parsing failure" adds very limited value.

Those are my preliminary thoughts. Please let me know if you have any other thoughts :)

The other error scenario I'd like to talk about is, the specified working directory doesn't exist. Shall we go ahead listing user-wide and machine-wide config files? Or show a warning saying the working directory doesn't exist? Or both?


### `dotnet nuget config`

### Commands

### Path

List all the paths of NuGet configuration files that will be applied, when invoking NuGet command from the current working directory path.
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved

The listed NuGet configuration files are in priority order. So the order of loading those configurations is reversed, that is, loading order is from the bottom to the top. So the configuration on the top will apply.
You may refer to [How settings are applied](https://learn.microsoft.com/en-us/nuget/consume-packages/configuring-nuget-behavior#how-settings-are-applied) for more details.

#### Arguments

- WORKING_DIRECTORY

Run this command as if working directory is set to the specified directory. If it's not specified, the current working directory will be used.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved

If the specified `WORKING_DIRECTORY` doesn't exist, an error is displayed indicating the `WORKING_DIRECTORY` doesn't exist.

> [!Note]
> If `WORKING_DIRECTORY` (or its parent directories) is not accessible, the command will ignore any NuGet configuration files under those directories without any warning/error. This is aligned with other NuGet commands.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here not accessible means "path exist, but don't enough privilege/permission" right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! Yes, it means the path exist but don't have enough privilege/permission to access.


#### Options

- -?|-h|--help

Prints out a description of how to use the command.

#### Examples

- List all the NuGet configuration file that will be applied, when invoking NuGet command in the current directory.

```
dotnet nuget config path

C:\Test\Repos\Solution\NuGet.Config
C:\Test\Repos\NuGet.Config
C:\Test\NuGet.Config
C:\Users\username\AppData\Roaming\NuGet\NuGet.Config
C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.FallbackLocation.config
C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.Offline.config
```

- List all the paths of NuGet configuration files that will be applied, when invoking NuGet command in the specific directory.

```
dotnet nuget config path C:\Test\Repos

C:\Test\Repos\NuGet.Config
C:\Test\NuGet.Config
C:\Users\username\AppData\Roaming\NuGet\NuGet.Config
C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.FallbackLocation.config
C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.Offline.config
```

- List all the NuGet configuration file that will be applied, but passing a non-existing argument `WORKING_DIRECTORY`.

```
dotnet nuget config path C:\Test\NonExistingRepos

Error: The path "C:\Test\NonExistingRepos" doesn't exist.
```

- List all the NuGet configuration file that will be applied, but passing an inaccessible `WORKING_DIRECTORY`: C:\Test\AccessibleRepos\NotAccessibleSolution.

The configuration file under C:\Test\AccessibleRepos\NotAccessibleSolution\NuGet.Config will be ignored without any warning or error.

```
dotnet nuget config path C:\Test\AccessibleRepos\NotAccessibleSolution

C:\Test\AccessibleRepos\NuGet.Config
C:\Test\NuGet.Config
C:\Users\username\AppData\Roaming\NuGet\NuGet.Config
C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.FallbackLocation.config
C:\Program Files (x86)\NuGet\Config\Microsoft.VisualStudio.Offline.config
```

### Get

Get the NuGet configuration settings that will be applied.

#### Arguments

- ALL

Get all merged NuGet configuration settings from multiple NuGet configuration files that will be applied, when invoking NuGet command from the working directory path.

- CONFIG_KEY

Get the effective value of the specified configuration settings of the [config section](https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file#config-section).


> [!Note]
> The CONFIG_KEY could only be one of the valid keys in config section.
For other sections, like package source section, we have/will have specific command [dotnet nuget list source](https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-list-source).

- WORKING_DIRECTORY

Run this command as if working directory is set to the specified directory. If it's not specified, the current working directory will be used.

If the specified `WORKING_DIRECTORY` doesn't exist, an error is displayed indicating the `WORKING_DIRECTORY` doesn't exist.

> [!Note]
> If `WORKING_DIRECTORY` (or its parent directories) is not accessible, the command will ignore any NuGet configuration files under those directories without any warning/error. This is aligned with other NuGet commands.

#### Options

- -?|-h|--help

Prints out a description of how to use the command.

- -v|--verbosity <LEVEL>

Sets the verbosity level of the command. Allowed values are q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic]. The default is minimal.

When the verbosity level is detailed or diagnostic, the source(NuGet configuration file path) will be show besides the configuration settings.

#### Examples

- Get all the NuGet configuration settings that will be applied, when invoking NuGet command in the current directory.

```
dotnet nuget config get all

packageSources:
clear
add key="source1" value="https://test/source1/v3/index.json"
add key="source2" value="https://test/source2/v3/index.json"

packageSourceMapping:
clear
packageSource key="source1" pattern="microsoft.*","nuget.*"
packageSource key="source2" pattern="system.*"

packageRestore:
add key="enabled" value="False"
add key="automatic" value="False"

```
- Get all the NuGet configuration settings that will be applied, when invoking NuGet command in the specific directory.

```
dotnet nuget config get all C:\Test\Repos

packageSources:
clear
add key="source1" value="https://test/source1/v3/index.json"
add key="source2" value="https://test/source2/v3/index.json"

packageSourceMapping:
clear
packageSource key="source1" pattern="microsoft.*","nuget.*"
packageSource key="source2" pattern="system.*"

packageRestore:
add key="enabled" value="False"
add key="automatic" value="False"

```

- Get all the NuGet configuration settings that will be applied, when invoking NuGet command in the current directory. Show the source(nuget configuration file path) of each configuration settings/child items.

```
dotnet nuget config get all -v d

packageSources:
add key="source1" value="https://test/source1/v3/index.json" file: C:\Test\Repos\Solution\NuGet.Config
add key="source2" value="https://test/source2/v3/index.json" file: C:\Test\Repos\NuGet.Config

packageSourceMapping:
clear file: C:\Users\username\AppData\Roaming\NuGet\NuGet.Config
packageSource key="source1" pattern="microsoft.*","nuget.*" file: C:\Test\Repos\Solution\NuGet.Config
packageSource key="source2" pattern="system.*" file: C:\Test\Repos\NuGet.Config

packageRestore:
add key="enabled" value="False" file: C:\Users\username\AppData\Roaming\NuGet\NuGet.Config
add key="automatic" value="False" file: C:\Users\username\AppData\Roaming\NuGet\NuGet.Config

```

- Get `http_proxy` from config section, when invoking NuGet command in the current directory.

```
dotnet nuget config get http_proxy

http://company-squid:3128@contoso.com"

```

- Get `http_proxy` from config section, when invoking NuGet command in the current directory. Show the source(nuget configuration file path) of this configuration setting.

```
dotnet nuget config get http_proxy

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there supposed to be a "show path" or verbosity parameter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Added --show-path. Thanks!


http://company-squid:3128@contoso.com" file: C:\Test\Repos\Solution\NuGet.Config

```

- Get `http_proxy` from config section, when `http_proxy` is not set in any of the NuGet configuration files.

```
dotnet nuget config get http_proxy

Key 'http_proxy' not found.

```
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved

### Set

Set the NuGet configuration settings.

This command will set the value of a specified NuGet configuration setting.

Please note this command only manages settings in [config section](https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file#config-section).
For other settings not in config section, we have/will have other dedicated commands. E.g., for [trustedSigners section](https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file#trustedsigners-section), we have [dotnet nuget trust](https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-trust) command.

#### Arguments

- CONFIG_KEY

Specify the key of the settings that are to be set.

If the specified `CONFIG_KEY` is not a key in [config section](https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file#config-section), a message is displayed indicating the `CONFIG_KEY` could not be set.

- CONFIG_VALUE

Set the value of the CONFIG_KEY to CONFIG_VALUE.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value type doesn't match with expected value type, then does it show appropriate message?
If you include 1 unhappy path scenario output for each use case, then that would be great. I learned this from dotnet list package --format json spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @erdembayar ! That's a good suggestion!
However, after checking, NuGet.exe config -Set doesn't check the key at all. So if we run NuGet.exe config -Set ABC=abc, the following will be added into NuGet.Config file:

  <config>
    <add key="ABC" value="abc" />
  </config>

Created a follow-up issue #12345, added it to the epic #12296.


#### Options

- --configfile <FILE>

The NuGet configuration file (nuget.config) to use. If specified, only the settings from this file will be used. If it's not specified, `%AppData%\NuGet\NuGet.Config` (Windows), or `~/.nuget/NuGet/NuGet.Config` or `~/.config/NuGet/NuGet.Config` (Mac/Linux) is used. See [On Mac/Linux, the user-level config file location varies by tooling.](https://learn.microsoft.com/en-us/nuget/consume-packages/configuring-nuget-behavior#on-maclinux-the-user-level-config-file-location-varies-by-tooling)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, even if there's a nuget.config in the current directory, if --configfile is not specified, the local directory file will not be modified?

While I think it's good that there's an easy way to modify the user-profile nuget.config, I feel like there will be more customers who expect the local directory file to be edited by default, so I think this will be non-intuitive.

Copy link
Contributor Author

@heng-liu heng-liu Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is the behavior in NuGet.exe config command right now.
You may refer to the doc , or the code .
The code indicates if there is no clear tag, default to furthest from the user, which is the user-wide configuration file (machine wide configuration files are excluded as they have IsReadOnly flag set to true, so they are not writable).
Unless this is not the expected behavior in NuGet.exe, otherwise we've discussed to keep the behavior consistent between NuGet.exe and dotnet.exe, and we only consider changing the behavior when major version changes, if there are enough upvotes. (Please refer to Future Work #2 in the spec).


- -?|-h|--help

Prints out a description of how to use the command.

#### Examples

- Set the `signatureValidationMode` to true in the closest NuGet configuration file.

```
dotnet nuget config set signatureValidationMode require

```

- Set the `defaultPushSource` in the specified NuGet configuration file.

```
dotnet nuget config set defaultPushSource https://MyRepo/ES/api/v2/package --configfile C:\Users\username\AppData\Roaming\NuGet\NuGet.Config

```

### Unset

Remove the NuGet configuration settings.

This command will remove the key-value pair from a specified NuGet configuration setting.

Please note this command only manages settings in [config section](https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file#config-section).
For other settings not in config section, we have/will have other commands. E.g. for [trustedSigners section](https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file#trustedsigners-section), we have [dotnet nuget trust](https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-trust) command.

#### Arguments

- CONFIG_KEY

Specify the key of the settings that are to be removed.

If the specified `CONFIG_KEY` is not a key in [config section](https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file#config-section), a message is displayed indicating the `CONFIG_KEY` could not be unset.

#### Options

- --configfile <FILE>

The NuGet configuration file (nuget.config) to use. If specified, only the settings from this file will be used. If it's not specified, `%AppData%\NuGet\NuGet.Config` (Windows), or `~/.nuget/NuGet/NuGet.Config` or `~/.config/NuGet/NuGet.Config` (Mac/Linux) is used. See [On Mac/Linux, the user-level config file location varies by tooling.](https://learn.microsoft.com/en-us/nuget/consume-packages/configuring-nuget-behavior#on-maclinux-the-user-level-config-file-location-varies-by-tooling)

- -?|-h|--help

Prints out a description of how to use the command.

#### Examples

- Unset the `signatureValidationMode` in the user-wide NuGet configuration file.

```
dotnet nuget config unset signatureValidationMode

```

- Unset the `defaultPushSource` in the specified NuGet configuration file.

```
dotnet nuget config unset defaultPushSource --configfile C:\Users\username\AppData\Roaming\NuGet\NuGet.Config

```

## Future Work
1. The `dotnet nuget config path/get` is a community ask. We will discuss if adding this command into NuGet.exe CLI, in the future.

2. NuGet.exe [config command](https://learn.microsoft.com/en-us/nuget/reference/cli-reference/cli-ref-config) is implemented.
But the behavior is confusing: the `set` command will set the property which appears last when loading(for all writable files), so it's not updating the closest NuGet configuration file, but the user-wide NuGet configuration file.(Related code: https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/Settings/Settings.cs#L229)
We keep the behavior of `dotnet nuget config set` the same with above for now.
But we might need to change this behavior in the future.
Considering this will be a breaking change, we will only consider doing it in main version change, if there are enough votes.

## Open Questions
1. Shall we use `dotnet nuget config path` or `dotnet nuget config paths` to get all configuration file paths?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with the plural (dotnet nuget config paths) since it returns multiple paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dsplaisted for reviewing! Changed into paths.


2. `WORKING_DIRECTORY` is changed from an option to an argument. So we have `dotnet nuget config path <WORKING_DIRECTORY>` and `dotnet nuget config get <ALL|CONFIG_KEY> <WORKING_DIRECTORY>`. Is it okey if we have two arguments in the second command? And if it's key, only the second argument could be optional, right?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems OK to me, but I'd like to hear from and defer to @baronfel, as he's the expert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @baronfel , could you help with this open question? Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye on the potential for ambiguity of the grammar here. Let me make sure I have the proposed grammar correct:

dotnet nuget config get <all|CONFIG_KEY> [<WORKING_DIRECTORY>]

meaning

the first argument is required, and will either be the string 'all' or the name of an existing config key. the second key is optional and if present represents the working directory to locate the nuget config file from

Is that correct? if so, I see no problem and am ok with the proposal. in cases where there is some potential for ambiguity, consider adding a flag to allow for explicit disambiguation (i.e. -w <path> like we rejected earlier), but we've already had that conversation so I don't think we need to revisit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's correct :)
Great! Thanks for the confirmation! I moved this open question to Considerations.


3. To show configuration files locations, is it better to use `--verbosity` option or some option named like `--show-path`? (git command is using `--show-origin` for similar purpose)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--show-path is better in my opinion. I think of verbosity as something to apply to logging, not so much the output of a command like this. Also in most cases of the .NET CLI, the verbosity doesn't do anything unless it's a command that ends up running MSBuild.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Make sense! Changed into --show-path.



## Considerations
1. Will this command help with diagnosing incorrect setting format?
<br />No. Incorrect NuGet settings should have separate error/warning message to tell the customer what's wrong in the setting file. If we have incorrect NuGet settings, all NuGet command, including `dotnet nuget config` command, should display the same error/warning message.
<br />E.g., if we have an invalid XML problem in one of the NuGet configuration file, running all NuGet command will get an error as following:
```
dotnet nuget list source
error: NuGet.Config is not valid XML. Path: 'C:\Users\username\Source\Repos\NuGet.Config'.
error: The 'disabledPackageSources' start tag on line 19 position 4 does not match the end tag of 'configuration'. Line 20, position 3.
```

2. Shall we use `dotnet nuget config get all` or `dotnet nuget config get` to get all configuration settings?
<br />We will use `dotnet nuget config get all` to get all configuration settings, or else, we could not differentiate `dotnet nuget config get <CONFIG_KEY>` and `dotnet nuget config get <WORKING_DIRECTORY>`.


3. `dotnet nuget config get <CONFIG_KEY>` will get the value of the specific config key (which is aligned with NuGet config command). `dotnet nuget config get` will get all merged configuration settings(not XML format). Since many sections are not simple key-value pairs, we need to define the format of outputs when getting all merged configuration settings. We need to define formats for all kinds of config sections. Here is an example:
```
packageSources:
clear
add key="source1" value="https://test/source1/v3/index.json"
add key="source2" value="https://test/source2/v3/index.json"

packageSourceMapping:
clear
packageSource key="source1" pattern="microsoft.*","nuget.*"
packageSource key="source2" pattern="system.*"

packageRestore:
add key="enabled" value="False"
add key="automatic" value="False"
```