-
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
Search version in set #43096
Search version in set #43096
Conversation
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.
So we've already added dotnet workload search version
which lists the available workload set versions, right? It looks like this adds dotnet workload search version <workloadSetVersion>
, and when the workload set version is specified it displays the manifests versions in that workload set.
I'm not sure about the scenario here. We do want to make more information available about what is in a workload set, but I think the manifest versions aren't too generally helpful here. Adding this now with this output format will make it harder to add more useful information (such as release notes, versions of tools such as XCode required, etc) to this command later.
<value>SEARCH_STRING</value> | ||
</data> | ||
<data name="WorkloadVersionArgumentDescription" xml:space="preserve"> | ||
<value>Output workload versions associated with the provided 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.
<value>Output workload versions associated with the provided workload version.</value> | |
<value>Output workload manifest versions associated with the provided workload version.</value> |
if (_workloadSetOutputFormat?.Equals("json", StringComparison.OrdinalIgnoreCase) == true) | ||
{ | ||
Reporter.WriteLine(JsonSerializer.Serialize( | ||
workloadSet.ManifestVersions.ToDictionary(kvp => kvp.Key.ToString(), kvp => $"{kvp.Value.Version}/{kvp.Value.FeatureBand}"))); |
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 think this is (and should be) the same format as the rollback file. So use WorkloadSet.ToJson
here instead of duplicating the logic of how it's formatted.
Triage: Summary is that the json format should have slightly more formatting so if we change this to add more data in the future, that is not a breaking change (ie put the manifest info in a subnode). |
- Generate error if workload search term and version subcommand are specified - If no workload sets are found, do not treat as an error, but print message to StdErr - Fix issue handling preview workload set versions when searching
Also split feature band out into separate column
I've made some updates to this and would appreciate a quick review. I added column headers to the table output and split the manifest feature band into a separate column. I've updated the PR description to show this. @baronfel, does this look good? |
Lovely! Thank you for updating the description. Totally agree with making the table output more user-friendly - with the json as the escape hatch we can freely change the user-facing views. |
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 VM tests look very helpful 👍
Progress on #42367
Current version: