Skip to content

Remove aggregation by Computer #3008

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 3, 2016
Merged

Remove aggregation by Computer #3008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions ...ts/Commands.OperationalInsights/Search/NewAzureOperationalInsightsComputerGroupCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,6 @@ protected override void ProcessRecord()
Tags = new List<Tag>() { new Tag() { Name = "Group", Value = "Computer" } }
};

if (!SearchCommandHelper.IsListOfComputers(this.Query))
{
throw new PSArgumentException("Query is not a list of computers. Please use aggregations such as: distinct Computer or measure count() by Computer.");
}

WriteObject(OperationalInsightsClient.CreateOrUpdateSavedSearch(ResourceGroupName, WorkspaceName, SavedSearchId, properties, Force, ConfirmAction), true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Expand Down
35 changes: 0 additions & 35 deletions ...rceManager/OperationalInsights/Commands.OperationalInsights/Search/SearchCommandHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,6 @@ namespace Microsoft.Azure.Commands.OperationalInsights

internal class SearchCommandHelper
{
/// <summary>
/// Return true if the provided query is a list of computers, return false otherwise.
/// </summary>
public static bool IsListOfComputers(string groupQuery)
{
if (string.IsNullOrEmpty(groupQuery))
{
return false;
}

// First, remove white spaces from group query.
string query = new string(
groupQuery.ToCharArray().Where(c => !Char.IsWhiteSpace(c)).ToArray());

if ((query.IndexOf("|measurecount()bycomputer", StringComparison.InvariantCultureIgnoreCase) >= 0) ||
(query.IndexOf("|distinctcomputer", StringComparison.InvariantCultureIgnoreCase) >= 0))
{
return true;
}

return false;
}

/// <summary>
/// Populate and validate SavedSearch.Properties.Tags from a Hashtable of tags specified in the cmdlet.
/// </summary>
Expand All @@ -57,7 +34,6 @@ public static IList<Tag> PopulateAndValidateTagsForProperties(Hashtable tags, st
return null;
}

bool hasGroupTag = false;
string groupKey = null;
IList<Tag> tagList = new List<Tag>();

Expand All @@ -69,7 +45,6 @@ public static IList<Tag> PopulateAndValidateTagsForProperties(Hashtable tags, st

if (key.Equals("group", System.StringComparison.InvariantCultureIgnoreCase))
{
hasGroupTag = true;
groupKey = key;
}
}
Expand All @@ -79,16 +54,6 @@ public static IList<Tag> PopulateAndValidateTagsForProperties(Hashtable tags, st
}
}

// If the saved search is tagged as a group of computers, do a sanity check on the query as it should be a list of computers.
if (hasGroupTag &&
tags[groupKey].ToString().Equals("computer", StringComparison.InvariantCultureIgnoreCase))
{
if (!IsListOfComputers(query))
{
throw new PSArgumentException("Query is not a list of computers. Please use aggregations such as: distinct Computer or measure count() by Computer.");
}
}

return tagList;
}
}
Expand Down