-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add "not installed" and other messages for workloads #43472
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
Conversation
Specifically when the user runs `dotnet workload --version` or `dotnet workload --info` after requesting a workload set via the global.json but it isn't installed, finding only the baseline manifest, or switching to WorkloadInstallType.WorkloadSets and not finding a workload set.
(GC errors are reported as warnings to the console)
else | ||
{ | ||
Reporter.WriteLine(string.Format(LocalizableStrings.WorkloadSetVersion, _workloadListHelper.WorkloadResolver.GetWorkloadVersion() ?? "unknown")); | ||
Reporter.WriteLine(string.Format(LocalizableStrings.WorkloadSetVersion, _workloadListHelper.WorkloadResolver.GetWorkloadVersion().Version ?? "unknown")); |
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.
Should "unknown" be localized?
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.
It probably should have been at one time, but by now I think that Version should never be null, so the "unknown" string should never actually be used.
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.
It looks good to me, though I added a few nit questions.
<comment>{Locked="dotnet workload restore"}</comment> | ||
</data> | ||
<data name="ShouldInstallAWorkloadSet" xml:space="preserve"> | ||
<value>Workloads are configured to install and update using workload versions, but none were found. Run "dotnet workload restore" to install a workload version.</value> |
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.
Would a string like this also suffice? Workload sets are enabled, but none are installed. Run "dotnet workload restore" to install a workload set version.\n
(Either way I would suggest adding a new line to separate out the workload info list.
I'm out of date enough to not know the differentiation between workload sets and workload versions. But after reading the issue and this message, for someone with less context, this provided more clarity, but I was also still a bit confused.
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 thought we discussed moving that message to the bottom of the output from workload --info? Though perhaps I misremembered.
Workload sets are basically workload versions, but it was decided that calling them sets was confusing, so workload version is the customer-facing phrasing.
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 see. But the toggle switch (maybe internal only) uses the workload-set terminology. dotnet workload config --update-mode 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.
Workload sets and workload versions are the same thing. I try to use "workload versions" as the user-facing term because it can be decently understood without learning a new concept, but that didn't really work with the config setting so they both show up.
I've added an extra blank line.
{ | ||
public static AndConstraint<CommandResultAssertions> PassWithoutWarning(this CommandResultAssertions assertions) | ||
{ | ||
return assertions.Pass() |
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.
This is a good test to have! Thank you!
<value>Workload version {0}, which was specified in {1}, was not found. Run "dotnet workload restore" to install this workload version.</value> | ||
<comment>{Locked="dotnet workload restore"}</comment> | ||
</data> | ||
<data name="ShouldInstallAWorkloadSet" xml:space="preserve"> |
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.
Is there any chance this would break dependents on the workload --info output? It looks like there's already text that can be prepended to it, so probably 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 went over this with @baronfel and if I've correctly implemented what we talked about, he felt that these changes were not likely to cause much issue with people parsing the output.
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 new changes also lgtm. Thank you!
Fixes #42527.
This PR is an update to #42683, I rebased the changes so I couldn't push to @Forgind's original branch.
Sample output when SDK is in workload set update mode but no workload set is installed:
Sample output when global.json specifies a workload set that isn't installed: