-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 workload config command #39888
Add workload config command #39888
Conversation
The replacement is `dotnet workload config --update-mode <true|false>`.
For these kinds of config-lookup commands there are generally two patterns:
I think using one of these forms would be easier for users to understand, because they would have experience in that form from other tools. This form would also scale to a more general To me, we should use one of these forms and use CliArguments to model these inputs. The CliArguments can use the same validation as currently described but make it easier to detect if the argument was even provided (can be controlled via Arity) and the values can be validated once the settings key is known using a validator in much the same way that you have already implemented. In teams chat today, @marcpopMSFT mentioned that rollback and download link printing could be moved to this config command - I could see that, but I could also see versions like:
I don't see as clear of an answer for that question. |
@baronfel Correct me if I'm wrong, but doesn't the dotnet CLI have its own style of command structure? Like, a general command format that most of the commands use. I know we don't have rules/regulations against the command format. But I think your comment makes sense in the context of a brand new In the context of just adding a small set of options for |
We talked today and we're going to keep the existing format in this PR, keeping a more broad treatment of the CLI grammar of a config command for a larger, centralized config effort. |
{ | ||
_workloadInstaller.UpdateInstallMode(_sdkFeatureBand, false); | ||
} | ||
else if (string.IsNullOrEmpty(_updateMode)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _hasUpdateMode check doesn't eliminate this possibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, _hasUpdateMode
specifies whether the --update-mode
option was specified, but it's possible to specify the option without it's optional argument value, which we use to print the current value.
{ | ||
_workloadInstaller.UpdateInstallMode(_sdkFeatureBand, true); | ||
} | ||
else if (_updateMode == WorkloadConfigCommandParser.UpdateMode_Manifests) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Equals for strings, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed (also with invariant ignore case)
{ | ||
// dotnet workload config --update-mode workload-set | ||
|
||
public static readonly string UpdateMode_WorkloadSet = "workload-set"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typing - is hard. workloadset?
Also, I thought we were trying to not include "workload set" terminology? Not that I have a better proposal in this case...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the guidance I got from @baronfel was to use dashes to separate words. I think that's the style we're going with in the CLI.
src/Cli/dotnet/commands/dotnet-workload/config/WorkloadConfigCommandParser.cs
Outdated
Show resolved
Hide resolved
@@ -159,11 +159,6 @@ public void RefreshWorkloadManifests() | |||
return _workloadSet?.Version!; | |||
} | |||
|
|||
if (InstallStateContents.FromPath(Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkVersionBand, _sdkRootPath), "default.json")).UseWorkloadSets == true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd want this to change to echo the version for the current SDK, per our discussion this morning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've filed an issue to track the further changes needed: #40005
bool _hasUpdateMode; | ||
string? _updateMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private
would be redundant here. I know it's inconsistent here and probably is throughout our codebase. Do we have a convention we are trying to follow for whether we should include private
or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally prefer including it...not technically an answer, but it's my preference :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure explicit access modifiers for fields are the convention in the dotnet org. I'm surprised we don't have that enabled in our editorconfig.
IWorkloadResolverFactory? workloadResolverFactory = null | ||
) : base(parseResult, CommonOptions.HiddenVerbosityOption, reporter) | ||
{ | ||
// TODO: Is it possible to check the order of the options? This would allow us to print the values out in the same order they are specified on the command line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can only get the list of tokens via the Tokens
property. The list should be in the order in which they are read. But, that doesn't have a relationship with the CliArguments
in any way, unfortunately. You'd have to make custom logic to figure that out.
dotnet workload config
command to set the workload update modedotnet --info
anddotnet workload --version
when in workload set mode but no workload set is installedHere's the how the new
dotnet workload config
command works:dotnet workload config --update-mode
- Prints the current workload update modedotnet workload config --update-mode workload-set
- Switches to workload set update modedotnet workload config --update-mode manifests
- Switches to loose manifest update modeHere's the command-line help:
This replaces the previous syntax, which was
dotnet workload update --mode workloadset
anddotnet workload update --mode loosemanifest
. Note that the previous option was hidden from command-line help, where currently thedotnet workload config
command is not hidden.