Allow insights powershell to unselect data sinks in diagnosticsettings. #3452

vcanaa · 2017-02-01T19:20:36Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

cormacpayne

@vcanaa a few comments

Also, make sure you update the help for Set-AzureRmDiagnosticSetting.md (since you have changed the help in the dll-Help.xml file)

cormacpayne · 2017-02-02T19:51:02Z

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

-                    foreach (string category in this.Categories)
-                    {
-                        LogSettings logSettings = properties.Logs.FirstOrDefault(x => string.Equals(x.Category, category, StringComparison.OrdinalIgnoreCase));
+                throw new ArgumentException("Parameter 'Enabled' is required by 'Timegrains' parameter.");


@vcanaa why not create a parameter set where Enabled is required if Timegrains is used?

I studied the parameter set and I didn't find a configuration that satisfies all the requirements. I decided to not use them.

cormacpayne · 2017-02-02T19:51:59Z

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

+        {
+            if (!this.Enabled.HasValue)
+            {
+                throw new ArgumentException("Parameter 'Enabled' is required by 'Categories' parameter.");


@vcanaa why not create a parameter set where Enabled is required if Categories is used?

Parameter set doesn't cover all requirements.

cormacpayne · 2017-02-02T19:52:55Z

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

-                    {
-                        logSettings.RetentionPolicy = retentionPolicy;
-                    }
+                    throw new ArgumentException("WorkspaceId and DisableWorkspace cannot be both present.");


@vcanaa use parameter sets to ensure that these parameters are mutually exclusive

Parameter set doesn't cover all requirements.

cormacpayne · 2017-02-02T19:53:36Z

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

-                    {
-                        metricSettings.RetentionPolicy = retentionPolicy;
-                    }
+                    throw new ArgumentException("ServiceBusId and DisableServiceBus cannot be both present.");


@vcanaa use parameter sets to ensure that these parameters are mutually exclusive

Parameter set doesn't cover all requirements.

cormacpayne · 2017-02-02T19:54:50Z

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

+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "Disable workspace")]
+        [ValidateNotNullOrEmpty]
+        public SwitchParameter DisableWorkspace { get; set; }


@vcanaa how do you handle the case of the three Disable parameters being used with the Enabled parameter? If a user shouldn't be able to use all four together (or Enabled with any combination of the three Disable parameters), then you should consider creating two parameters - one that has Enabled, and another that has the three Disable parameters

The powershell is supposed to offer a way to easily build the request. If the request ends up not being valid, we want to the server to validate and respond with proper message.

cormacpayne · 2017-02-02T19:58:44Z

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

+        /// <summary>
+        /// Gets or sets the switch for workspace parameter of the cmdlet
+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "Disable workspace")]


@vcanaa please remove the ValueFromPipelineByPropertyName attribute from the three SwitchParameter parameters you have added

cormacpayne · 2017-02-02T19:59:41Z

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

        [ValidateNotNullOrEmpty]
-        public bool Enabled { get; set; }
+        public bool? Enabled { get; set; }


@vcanaa when can this be nullable?

Customer can run the following
Set-AzureRmDiagnosticSettings -ResourceId <> -DisableStorage

…h guidelines

shahabhijeet · 2017-02-13T23:20:33Z

@vcanaa please reopen this PR once you address the feedback and fix build failures.

…settings

markcowl · 2017-02-16T20:51:28Z

@azuresdkci retest this please

cormacpayne · 2017-02-17T00:46:59Z

@azuresdkci test this please

cormacpayne · 2017-02-17T17:28:36Z

On-demand: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1409/

markcowl

One minor change, otherwise, this looks good.

markcowl · 2017-02-17T18:49:25Z

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

@@ -31,6 +32,11 @@ namespace Microsoft.Azure.Commands.Insights.Diagnostics
    [Cmdlet(VerbsCommon.Set, "AzureRmDiagnosticSetting"), OutputType(typeof(PSServiceDiagnosticSettings))]


Change cmdlet name to update and place an alias to Set (or vice-versa) Also, please note in upcoming breaking changes that the cmdlet name is changing to update: https://github.com/Azure/azure-powershell/blob/dev/src/ResourceManager/Insights/documentation/upcoming-breaking-changes.md

Can we just leave the name as Set-DiagnsoticSettings? We are improving the api to support multiple diagnostic settings. After that we plan to retire this commandlet and have only Add-AzureRmDiagnosticSettings. I don't think we should change the name a command that is soon to be deprecated.

…s to Update-AzureRmDiagnosticSettings in the upcoming breaking changes doc

Allow insights powershell to unselect data sinks in diagnosticsettings.

dbf44c5

azurecla added the cla-not-required label Feb 1, 2017

cormacpayne suggested changes Feb 2, 2017

View reviewed changes

cormacpayne added the Changes Requested label Feb 2, 2017

vcanaa and others added 2 commits February 2, 2017 13:23

[Insights] Minor fixes in Set-AzureRmDiagnosticSettings to comply wit…

afc847c

…h guidelines

Merge branch 'dev' of https://github.com/Azure/azure-powershell into dev

333d27c

shahabhijeet closed this Feb 13, 2017

cormacpayne reopened this Feb 14, 2017

azurecla added the cla-not-required label Feb 14, 2017

cormacpayne self-assigned this Feb 14, 2017

vcanaa added 2 commits February 14, 2017 16:51

[Insights] Use explicit null value to disable data sink in diagnostic…

397ea87

…settings

[Insights] Making enabled parameter in diagnosticsettings non nullable

fc824db

cormacpayne changed the base branch from dev to release-3.6.0 February 16, 2017 23:18

[Insights] Fix mispelling on enabled parameter of diagnosticsettings

d1f1d47

markcowl requested changes Feb 17, 2017

View reviewed changes

[Insights] Include notice about renaming Set-AzureRmDiagnosticSetting…

232fbce

…s to Update-AzureRmDiagnosticSettings in the upcoming breaking changes doc

markcowl approved these changes Feb 18, 2017

View reviewed changes

cormacpayne approved these changes Feb 21, 2017

View reviewed changes

cormacpayne merged commit 9b61619 into Azure:release-3.6.0 Feb 21, 2017

		@@ -31,6 +32,11 @@ namespace Microsoft.Azure.Commands.Insights.Diagnostics
		[Cmdlet(VerbsCommon.Set, "AzureRmDiagnosticSetting"), OutputType(typeof(PSServiceDiagnosticSettings))]

Allow insights powershell to unselect data sinks in diagnosticsettings. #3452

Allow insights powershell to unselect data sinks in diagnosticsettings. #3452

Uh oh!

Conversation

vcanaa commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahabhijeet commented Feb 13, 2017

Uh oh!

markcowl commented Feb 16, 2017

Uh oh!

cormacpayne commented Feb 17, 2017

Uh oh!

cormacpayne commented Feb 17, 2017

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vcanaa commented Feb 1, 2017 •

edited

Loading