Add spec of dotnet nuget config command #12172

heng-liu · 2022-10-19T17:02:27Z

Fixes: #12297
Add a spec of dotnet nuget config list command.

It's a part of epic issue:
Add a dotnet nuget config command #12296

proposed/2022/dotnet-nuget-config-spec.md

nkolev92

I'm really excited about this addition :)

proposed/2022/dotnet-nuget-config-spec.md

heng-liu · 2022-10-24T18:25:55Z

There are two types of implementations:

Treat list as subcommand. Example: dotnet nuget trust
Treat list as an option. Example: dotnet nuget locals

I followed the 1st in this spec. But I'm not quite sure if there is any preference. Please advise, thanks!

nkolev92 · 2022-10-24T18:37:45Z

I think it has to be a subcommand.

The subcommand matches dotnet list package, dotnet list reference etc.
It matches the direction of the dotnet CLI.

heng-liu · 2022-10-24T21:12:16Z

Hi @baronfel, thanks for your help offline on the guidance!
Could you please take a look and let me know if there is anything I need to change? Thanks! :)

proposed/2022/dotnet-nuget-config-spec.md

zivkan · 2022-10-25T18:51:47Z

proposed/2022/dotnet-nuget-config-spec.md

+## Solution
+The following command will be implemented in the `dotnet.exe` CLI.


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?

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?

proposed/2022/dotnet-nuget-config-spec.md

heng-liu · 2022-10-26T19:05:48Z

Hi @baronfel, I think I've addressed all comments for now.
May I know if you could take a look? Or do you prefer a review meeting late this week or early next week? Thanks!

proposed/2022/dotnet-nuget-config-spec.md

heng-liu · 2022-10-28T18:09:46Z

I just thought about another scenario, when there is an error when accessing the current directories/it's parent directories.
For now, nuget command just ignore the inaccessible config file and there will be no warning/error. As we check if the config file exists at this line, and according to doc:
The Exists method returns false if any error occurs while trying to determine if the specified file exists. This can occur in situations that raise exceptions such as passing a file name with invalid characters or too many characters, a failing or missing disk, or if the caller does not have permission to read the file.

So, for a folder as below, if I change "Solution" folder to inaccessible, the "NuGet.Config" file under this folder will be ignored and there will be no warning/error.

C:\TEST
│ NuGet.Config
│
└───Repos
    │ NuGet.Config
    │
    └───Solution
        │ NuGet.Config
        │   Solution.sln
        │
        └───Project
                Class1.cs
                Project.csproj

I'm assuming this is expected (Please let me know if you consider this is a bug). Shall I warn/error for this situation for "dotnet nuget config list" command? Or just ignore it and show the rest of NuGet.Config files without any warning/error(align with other NuGet commands)?

If the current behavior for nuget command is expected(ignore the inaccessible config files without any warning/error), my thoughts are doing this without any warning/error, but add this as a note into the doc of "dotnet nuget config" command. (Applied in the doc). Please let me know if you have any other thoughts. Thanks!

kartheekp-ms · 2022-10-28T19:01:32Z

proposed/2022/dotnet-nuget-config-spec.md

+
+#### Arguments
+
+- CURRENT_DIRECTORY


Incase if we are referring to CURRENT_DIRECTORY as working directory, dotnet store command has -w|--working-dir <WORKING_DIRECTORY> option https://learn.microsoft.com/dotnet/core/tools/dotnet-store#optional-options. I think it is better to use the same option name for this command also.

Thanks for reviewing! Renamed it as WORKING_DIRECTORY, but still an argument based on previous discussions. Pls let me know if you have any different thoughts :)

nkolev92 · 2022-10-31T20:59:34Z

I'm assuming this is expected (Please let me know if you consider this is a bug). Shall I warn/error for this situation for "dotnet nuget config list" command? Or just ignore it and show the rest of NuGet.Config files without any warning/error(align with other NuGet commands)?

I'm guessing we match what commands that use the configs do, for example running restore.

nkolev92

This feature is turning out bigger than originally expected, but I think that's for the better!
A couple of suggestion for tweaks.

Great job @heng-liu

nkolev92 · 2022-11-22T01:16:39Z

proposed/2022/dotnet-nuget-config-spec.md

+
+#### Commands
+
+- Path


Should the config be paths instead?
I'm not really sure what the guidance for plural vs singular is.

Thanks for reviewing! I used path in the spec and add this question into open question as #1.
Hi @baronfel, could you help advise? Thanks!

proposed/2022/dotnet-nuget-config-spec.md

nkolev92 · 2022-11-22T01:18:00Z

proposed/2022/dotnet-nuget-config-spec.md

+
+#### Options
+
+- -w|--working-dir <WORKING_DIRECTORY>


I left the feedback on the original thread. I don't think we want an option. A working directory argument is a natural solution.
--working-dir is redundant imo.

Replying to myself again, this isn't redundant but a way to differentiate between getting all and specifying a directory.

Thanks! Yes, @kartheekp-ms helped find an example of this option:
The Dotnet store command has -w|--working-dir <WORKING_DIRECTORY> option https://learn.microsoft.com/dotnet/core/tools/dotnet-store#optional-options
Since it can be short for -w, I think it should be okey to be an option?

The existence of one instance doesn't make it the best approach imo.

I'm also one of the few people even commenting on this concern, so I'm not really sure how I feel about it because of that. Whether others don't care, or others prefer the working directory approach.

proposed/2022/dotnet-nuget-config-spec.md

nkolev92 · 2022-11-22T01:26:43Z

proposed/2022/dotnet-nuget-config-spec.md

+
+3. `dotnet nuget config get -v d` will display source of each configuration setting key, in a format of comment in xml file. So that people can still redirect the output into a file without breaking any syntax. Does that sound good to you?
+
+3. `--working-dir` is changed from an argument into an option. Because we could not differentiate `dotnet nuget config get <WORKING_DIRECTORY>` and `dotnet nuget config get <CONFIG_KEY>`. Any better ideas?


I see the motivation for this change.

IMO we should add a magic word for getting all the config values, example all.

I added all and also add this as #2 in the open questions.
Hi @baronfel , could you pls advise? Thanks!

Is "all" necessary if there's no longer a 'working dir' argument? In that case, no argument value would mean all because the presence of an argument would be a filter.

my suggestion is we do the magic word all and make the argument the working directory.

--working-directory just seems off.

I'm also very conscious of the fact that only a few of us have commented on this topic in here, while more did in the meeting :D

nkolev92 · 2022-11-23T00:09:09Z

Great summary with the open questions @heng-liu
I know it's been a lot of iterations, but I really appreciate that the open questions are up to date :)

heng-liu · 2022-12-09T07:19:04Z

Hi @baronfel and @dsplaisted , there are 3 open questions are mainly about dotnet conventions. Could you please advise? Thanks!
Other comments are all addressed. (You may find the details in email I sent on 12/6 ).
Please let me know if you have any other concerns. Thanks!

zivkan · 2022-12-09T17:08:18Z

proposed/2022/dotnet-nuget-config-spec.md

+
+- --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)


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.

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).

dsplaisted · 2022-12-09T20:45:03Z

proposed/2022/dotnet-nuget-config-spec.md

+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?


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

Thanks @dsplaisted for reviewing! Changed into paths.

dsplaisted · 2022-12-09T20:45:15Z

proposed/2022/dotnet-nuget-config-spec.md

+## Open Questions
+1. Shall we use `dotnet nuget config path` or `dotnet nuget config paths` to get all configuration file 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? 


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

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

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.

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

dsplaisted · 2022-12-09T20:48:00Z

proposed/2022/dotnet-nuget-config-spec.md

+- 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 


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

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

dsplaisted · 2022-12-09T20:50:50Z

proposed/2022/dotnet-nuget-config-spec.md

+
+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? 
+
+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)


--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.

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

heng-liu · 2022-12-12T20:13:12Z

Thanks all for helping review this spec!
I know there are lots of iterations. Thanks a lot for your patience!

All comments are addressed in this spec. It's ready to review again.

proposed/2022/dotnet-nuget-config-spec.md

erdembayar · 2022-12-13T01:12:31Z

proposed/2022/dotnet-nuget-config-spec.md

+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.


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

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

erdembayar · 2022-12-13T01:20:50Z

proposed/2022/dotnet-nuget-config-spec.md

+
+- CONFIG_VALUE
+
+Set the value of the CONFIG_KEY to CONFIG_VALUE.


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.

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.

nkolev92

Love it.
Great job.

heng-liu · 2023-01-04T18:08:32Z

I'll merge the PR since it's approved several weeks ago and there is no new comments.
Please let me know if there is anything you'd like to discuss and I will create follow-up issues if necessary. Thanks!
FYI @dsplaisted @baronfel

heng-liu added 2 commits October 18, 2022 21:35

work

e8fdc34

fix

006cb0f

heng-liu requested a review from a team as a code owner October 19, 2022 17:02

erdembayar reviewed Oct 19, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

heng-liu added 2 commits October 19, 2022 14:59

address feedback

335e6ae

fix

204ba1a

erdembayar approved these changes Oct 19, 2022

View reviewed changes

nkolev92 reviewed Oct 21, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

zivkan reviewed Oct 21, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

heng-liu added 2 commits October 21, 2022 15:50

address feedback

5dff9f6

address feedback

abc436d

dominoFire approved these changes Oct 25, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

zivkan reviewed Oct 25, 2022

View reviewed changes

nkolev92 reviewed Oct 25, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

address comments

ffd5e71

heng-liu requested review from nkolev92 and zivkan October 27, 2022 17:02

erdembayar reviewed Oct 27, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

address feedback

a25ea5d

zivkan reviewed Oct 28, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Outdated Show resolved Hide resolved

kartheekp-ms reviewed Oct 28, 2022

View reviewed changes

address comments & add more examples

9da2a9f

heng-liu requested review from zivkan and kartheekp-ms October 31, 2022 17:02

heng-liu requested review from baronfel, zivkan and kartheekp-ms November 21, 2022 17:41

nkolev92 reviewed Nov 22, 2022

View reviewed changes

heng-liu added 4 commits November 22, 2022 12:56

address feedback

b679074

Reformat the output of dotnet nuget config all

3b4ef11

address feedback

46aec6c

address feedback

0836860

heng-liu requested a review from nkolev92 November 22, 2022 23:37

change WORKING_DIRECTORY from option to argument

e4b6ae5

zivkan reviewed Dec 9, 2022

View reviewed changes

dsplaisted reviewed Dec 9, 2022

View reviewed changes

heng-liu added 2 commits December 12, 2022 10:11

address feedbacks

160cda0

fix a typo

779cfc8

heng-liu requested review from dsplaisted and zivkan and removed request for dsplaisted December 12, 2022 19:31

address feedback

45f081f

erdembayar reviewed Dec 12, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Show resolved Hide resolved

erdembayar reviewed Dec 13, 2022

View reviewed changes

proposed/2022/dotnet-nuget-config-spec.md Show resolved Hide resolved

erdembayar reviewed Dec 13, 2022

View reviewed changes

nkolev92 approved these changes Dec 13, 2022

View reviewed changes

heng-liu merged commit daa2597 into dev Jan 4, 2023

heng-liu deleted the dev-hengliu-addSpec-dotnetNuGetConfig branch January 4, 2023 18:08

		## Solution
		The following command will be implemented in the `dotnet.exe` CLI.


		3. `dotnet nuget config get -v d` will display source of each configuration setting key, in a format of comment in xml file. So that people can still redirect the output into a file without breaking any syntax. Does that sound good to you?

		3. `--working-dir` is changed from an argument into an option. Because we could not differentiate `dotnet nuget config get <WORKING_DIRECTORY>` and `dotnet nuget config get <CONFIG_KEY>`. Any better ideas?


		- --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)


		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?

		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)


		- CONFIG_VALUE

		Set the value of the CONFIG_KEY to CONFIG_VALUE.

Add spec of dotnet nuget config command #12172

Add spec of dotnet nuget config command #12172

Conversation

heng-liu commented Oct 19, 2022 • edited Loading

nkolev92 left a comment

Choose a reason for hiding this comment

heng-liu commented Oct 24, 2022

nkolev92 commented Oct 24, 2022 • edited Loading

heng-liu commented Oct 24, 2022

Choose a reason for hiding this comment

heng-liu Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

heng-liu commented Oct 26, 2022

heng-liu commented Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 commented Oct 31, 2022

nkolev92 left a 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

nkolev92 commented Nov 23, 2022

heng-liu commented Dec 9, 2022

Choose a reason for hiding this comment

heng-liu Dec 12, 2022 • edited Loading

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

heng-liu commented Dec 12, 2022

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

nkolev92 left a comment

Choose a reason for hiding this comment

heng-liu commented Jan 4, 2023

heng-liu commented Oct 19, 2022 •

edited

Loading

nkolev92 commented Oct 24, 2022 •

edited

Loading

heng-liu Oct 26, 2022 •

edited

Loading

heng-liu commented Oct 28, 2022 •

edited

Loading

heng-liu Dec 12, 2022 •

edited

Loading