Merge pull request #7160 from gucalder/preview

[Insights] Fixing issues #6833 and #7102: failure during creation if categories are included

Author: cormacpayne

src/ResourceManager/Insights/Commands.Insights.Test/Diagnostics/SetDiagnosticSettingCommandTests.cs

@@ -203,14 +203,23 @@ public void SetSomeCategories()
             expectedSettings.Logs[0].Enabled = false;
 
             VerifyCalledOnce();
-            VerifySettings(expectedSettings, this.calledSettings);
+            VerifySettings(expectedSettings, this.calledSettings, suffix: "#1");
 
             // Testing the new categories must be known before the cmdlet can add them
+            expectedSettings.Logs.Add(
+                new LogSettings()
+                {
+                    Category = "TestCategory3",
+                    RetentionPolicy = new RetentionPolicy
+                    {
+                        Days = 0,
+                        Enabled = false
+                    }
+                });
             cmdlet.Categories = new List<string> { "TestCategory3" };
             cmdlet.Enabled = false;
             cmdlet.MyInvocation.BoundParameters[SetAzureRmDiagnosticSettingCommand.EnabledParamName] = false;
-            var argException = Assert.Throws<PSInvalidOperationException>(() => cmdlet.ExecuteCmdlet());
-            Assert.Contains("ArgumentException", argException.ToString());
+            cmdlet.ExecuteCmdlet();
 
             // Testing the new metric categories must be known before the cmdlet can add them
             expectedSettings.Metrics[0].Enabled = false;
@@ -220,14 +229,13 @@ public void SetSomeCategories()
             cmdlet.MyInvocation.BoundParameters[SetAzureRmDiagnosticSettingCommand.EnabledParamName] = false;
             cmdlet.ExecuteCmdlet();
 
-            VerifySettings(expectedSettings, this.calledSettings);
+            VerifySettings(expectedSettings, this.calledSettings, suffix: "#2");
 
             // Testing the new categories must be known before the cmdlet can add them
             cmdlet.MetricCategory = new List<string> { "MetricCat3" };
             cmdlet.Enabled = false;
             cmdlet.MyInvocation.BoundParameters[SetAzureRmDiagnosticSettingCommand.EnabledParamName] = false;
-            argException = Assert.Throws<PSInvalidOperationException>(() => cmdlet.ExecuteCmdlet());
-            Assert.Contains("ArgumentException", argException.ToString());
+            cmdlet.ExecuteCmdlet();
         }
 
         [Fact]
@@ -310,7 +318,8 @@ private void VerifyCalledOnce()
 
         private void VerifySettings(
             DiagnosticSettingsResource expectedSettings,
-            DiagnosticSettingsResource actualSettings)
+            DiagnosticSettingsResource actualSettings,
+            string suffix = "")
         {
             Assert.Equal(expectedSettings.StorageAccountId, actualSettings.StorageAccountId);
             Assert.Equal(expectedSettings.EventHubName, actualSettings.EventHubName);
@@ -321,7 +330,7 @@ private void VerifySettings(
             }
             else
             {
-                Assert.True(expectedSettings.Logs.Count == actualSettings.Logs.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Log settings", expectedSettings.Logs.Count, actualSettings.Logs.Count));
+                Assert.True(expectedSettings.Logs.Count == actualSettings.Logs.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Log settings {2}", expectedSettings.Logs.Count, actualSettings.Logs.Count, suffix));
                 for (int i = 0; i < expectedSettings.Logs.Count; i++)
                 {
                     var expected = expectedSettings.Logs[i];
@@ -338,7 +347,7 @@ private void VerifySettings(
             }
             else
             {
-                Assert.True(expectedSettings.Metrics.Count == actualSettings.Metrics.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Metric settings", expectedSettings.Metrics.Count, actualSettings.Metrics.Count));
+                Assert.True(expectedSettings.Metrics.Count == actualSettings.Metrics.Count, string.Format("Expected: {0}, Actual: {1}, no the same number of Metric settings {2}", expectedSettings.Metrics.Count, actualSettings.Metrics.Count, suffix));
                 for (int i = 0; i < expectedSettings.Metrics.Count; i++)
                 {
                     var expected = expectedSettings.Metrics[i];

src/ResourceManager/Insights/Commands.Insights/ChangeLog.md

@@ -19,6 +19,13 @@
 -->
 ## Current Release
 
+* Fixed issues #6833 and #7102 (Diagnostic Settings area)
+    - Issues with the default name, i.e. "service", during creation and listing/getting of diagnostic settings
+    - Issues creating diagnostic settings with categories
+
+* Added deprecation message for metrics time grains parameters
+    - Timegrains parameters are still being accepted (this is a non-breaking change,) but they are ignored in the backend since only PT1M is valid
+
 ## Version 5.1.3
 * Fixed issue with default resource groups not being set.
 * Updated common runtime assemblies

src/ResourceManager/Insights/Commands.Insights/Diagnostics/SetAzureRmDiagnosticSettingCommand.cs

@@ -364,8 +364,13 @@ private void SetSelectedTimegrains(DiagnosticSettingsResource properties)
                 throw new ArgumentException("Parameter 'Enabled' is required by 'Timegrains' parameter.");
             }
 
-            if (this.Timegrains.Count > 0)
+            if (this.Timegrains != null && this.Timegrains.Count > 0)
             {
+                if (properties.Metrics == null)
+                {
+                    properties.Metrics = new List<MetricSettings>();
+                }
+
                 WriteWarningWithTimestamp("Deprecation: The timegain argument for metrics will be deprecated since the back end supports only PT1M. Currently it is ignored for backwards compatibility.");
                 WriteDebugWithTimestamp("Setting Enabled property for metrics since timegrains argument is non-empty");
                 foreach (MetricSettings metric in properties.Metrics)
@@ -383,16 +388,35 @@ private void SetSelectedCategories(DiagnosticSettingsResource properties)
             }
 
             WriteDebugWithTimestamp("Setting log categories, including Enabled property");
+            if (properties.Logs == null)
+            {
+                properties.Logs = new List<LogSettings>();
+            }
+
             foreach (string category in this.Categories)
             {
                 LogSettings logSettings = properties.Logs.FirstOrDefault(x => string.Equals(x.Category, category, StringComparison.OrdinalIgnoreCase));
-
                 if (logSettings == null)
                 {
-                    throw new ArgumentException(string.Format(CultureInfo.InvariantCulture, "Log category '{0}' is not available", category));
-                }
+                    // if not there add it
+                    logSettings = new LogSettings()
+                    {
+                        Category = category,
+                        RetentionPolicy = new RetentionPolicy
+                        {
+                            Days = 0,
+                            Enabled = false
+                        },
+                        Enabled = this.Enabled
+                    };
 
-                logSettings.Enabled = this.Enabled;
+                    properties.Logs.Add(logSettings);
+                }
+                else
+                {
+                    // else update it
+                    logSettings.Enabled = this.Enabled;
+                }
             }
         }
 
@@ -404,16 +428,37 @@ private void SetSelectedMetricsCategories(DiagnosticSettingsResource properties)
             }
 
             WriteDebugWithTimestamp("Setting metric categories, including Enabled property");
+            if (properties.Metrics == null)
+            {
+                properties.Metrics = new List<MetricSettings>();
+            }
+
             foreach (string category in this.MetricCategory)
             {
                 MetricSettings metricSettings = properties.Metrics.FirstOrDefault(x => string.Equals(x.Category, category, StringComparison.OrdinalIgnoreCase));
 
                 if (metricSettings == null)
                 {
-                    throw new ArgumentException(string.Format(CultureInfo.InvariantCulture, "Metric category '{0}' is not available", category));
-                }
+                    // If not there add it
+                    metricSettings = new MetricSettings
+                    {
+                        Category = category,
+                        Enabled = this.Enabled,
+                        RetentionPolicy = new RetentionPolicy
+                        {
+                            Days = 0,
+                            Enabled = false
+                        },
+                        TimeGrain = null
+                    };
 
-                metricSettings.Enabled = this.Enabled;
+                    properties.Metrics.Add(metricSettings);
+                }
+                else
+                {
+                    // else update it
+                    metricSettings.Enabled = this.Enabled;
+                }
             }
         }
 
@@ -425,12 +470,22 @@ private void SetAllCategoriesAndTimegrains(DiagnosticSettingsResource properties
             }
 
             WriteDebugWithTimestamp("Setting Enabled property for logs");
+            if (properties.Logs == null)
+            {
+                properties.Logs = new List<LogSettings>();
+            }
+
             foreach (var log in properties.Logs)
             {
                 log.Enabled = this.Enabled;
             }
 
             WriteDebugWithTimestamp("Setting Enabled property for metrics");
+            if (properties.Metrics == null)
+            {
+                properties.Metrics = new List<MetricSettings>();
+            }
+
             foreach (var metric in properties.Metrics)
             {
                 metric.Enabled = this.Enabled;
@@ -470,7 +525,6 @@ private void SetEventHubRule(DiagnosticSettingsResource properties)
             }
         }
 
-
         private void SetStorage(DiagnosticSettingsResource properties)
         {
             if (this.isStorageParamPresent)