Remove aggregation by Computer #3008
Conversation
Remove aggregation by Computer from NewAzureOperationalInsightsComputerGroupCommand
|
Hi @lwang2016, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
|
Please clean up the description area and provide what is this PR is about. Please use the template by editing it, so that it can help to review your PR. |
| @@ -73,11 +73,6 @@ protected override void ProcessRecord() | |||
| Tags = new List<Tag>() { new Tag() { Name = "Group", Value = "Computer" } } | |||
| }; | |||
|
|
|||
| if (!SearchCommandHelper.IsListOfComputers(this.Query)) | |||
There was a problem hiding this comment.
So earlier, for query not using agrregate function inside query we use to throw PSArgumentException, but now for the same exact query we no longer throw?
There was a problem hiding this comment.
The verification is removed because of a recent new feature in our service backend. Previously, a computer group query must end with either "measure count() by Computer" or "distinct Computer". We recently added a new feature that allows customers to specify a computer simply by a list of comma-separated values, such as "C1, C2, C3". With the change, the original verification is no longer needed and thus removed in this change.
There was a problem hiding this comment.
I don't see any test coverage for the new query format? Also is there any tests that make sure the old query will still work. Also can a user combine the new format with the old? If yes would be great to add that in the test coverage.
There was a problem hiding this comment.
IMHO, I don't think extra test coverage is necessary in this change. What does the cmdlet do is to save the computer group Query to a table in OperationalInsights. The cmdlet doesn't care the format inside the query as long as it is saved to the table succesfully, which existing test cases in azure-powershell already covered.
There are test coverage on OperationalInsights side to make sure both the new and old query syntaxes work as expected. So the query functionalities are also covered.
|
Updated description. |
|
LGTM |
This pull request is to remove verification on the query field format in NewAzureOperationalInsightsComputerGroupCommand.
The verification is removed because of a recent new feature in our service backend. Previously, a computer group query must end with either "measure count() by Computer" or "distinct Computer". We recently added a new feature that allows customers to specify a computer simply by a list of comma-separated values, such as "C1, C2, C3". With the change, the original verification is no longer needed and thus removed in this change.