[Insights] Introduce announced breaking changes, implement ShouldProcess, rename parameters to singluar names and add aliases to keep it non-breaking #4875

gucalder · 2017-10-27T21:06:31Z

Description

Deprecation of two cmdlets as announced in April 2017: Get-AzureRmUsage (API not being used), Add-AzureRmLogAlertRule (functionality moved to Azure ActivityLogAlerts API introduced in Ignite)
Change the output type of the following cmdlets as announced in April 2017: Get-AzureRmAlertRule (flattening output,) Remove-AzureRmAlertRule (from list to single object,) Get-AzurRmAutoscaleSetting (minor change,) Remove-AzureRmLogProfile (from Boolean to object,) and Get-AzureRmLog, Get-AzureRmAlertHistory, and Get-AzureRmAutoscaleHistory (minor changes)
Add the ShouldProcess functionality to these cmdlets Add-AzureRmMetricAlertRule, Add-AzureRmWebtestAlertRule, Remove-AzureRmAlertRule, Add-AzureRmAutoscaleSetting, Remove-AzureRmAutoscaleSetting, Add-AzureRmLogProfile, and Remove-AzureRmLogProfile. Remove the cmdlets from the exceptions list. Keep exceptions for the Force parameter.
Add aliases (i.e. singular nouns) to those parameters that are currently using plural nouns. Also add deprecation warnings for the plural names.

This replaces PR #4873 issued from AuxMon organization.

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.

…RmUsage

…t require it

… and adding aliases to keep the change non-breaking, implementing ShouldProcess, announcing deprecation of plural parameter names # Conflicts: # tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv

gucalder · 2017-10-27T22:29:57Z

I think there are two empty lines at the end of the file Exceptions\BreakingChangeIssues.csv and they are breaking the StaticAnalysis.

gucalder · 2017-10-27T23:03:15Z

I do not know what the issue was this time. It failed very quickly.

cormacpayne

@gucalder some initial comments

cormacpayne · 2017-10-29T21:41:30Z

src/ResourceManager/Insights/Commands.Insights.Test/Utilities.cs

        {
            // Calling with detailed output
            cmdlet.DetailedOutput = true;
            cmdlet.ExecuteCmdlet();
-            Assert.Null(selected); // Incorrect nameOrTargetUri clause with detailed output on
+            // Assert.Null(selected); // Incorrect nameOrTargetUri clause with detailed output on


@gucalder please remove this line

cormacpayne · 2017-10-29T21:43:21Z

src/ResourceManager/Insights/Commands.Insights/ActivityLogs/GetAzureRmLogCommand.cs

@@ -104,7 +104,8 @@ public class GetAzureRmLogCommand : LogsCmdletBase
        /// <returns>The query filter with the conditions for particular parameters added</returns>
        protected override string ProcessParticularParameters(string currentQueryFilter)
        {
-            this.SetMaxEventsIfPresent(currentQueryFilter, this.MaxEvents);
+            WriteWarning("Parameter name change: The parameter plural names for the parameters will be deprecated in May 2018 in favor of the singular versions of the same names.");


@gucalder please update the warning message to include the name of the cmdlet - this will help users running this cmdlet in a script easily find which cmdlet the warning message is coming from. Please do this for all warnings in cmdlets.

Done. I am including the cmdlet name in all warning messages.

cormacpayne · 2017-10-29T21:43:52Z

src/ResourceManager/Insights/Commands.Insights/Alerts/AddAzureRmAlertRuleCommandBase.cs

@@ -53,7 +53,7 @@ public abstract class AddAzureRmAlertRuleCommandBase : ManagementCmdletBase
        /// </summary>
        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name")]
        [ValidateNotNullOrEmpty]
-        public string ResourceGroup { get; set; }
+        public string ResourceGroupName { get; set; }


@gucalder please add an alias to ResourceGroup to avoid undocumented breaking changes

Alias added... everywhere including documentation

cormacpayne · 2017-10-29T21:45:04Z

src/ResourceManager/Insights/Commands.Insights/Alerts/GetAzureRmAlertRuleCommand.cs

@@ -41,7 +41,7 @@ public class GetAzureRmAlertRuleCommand : ManagementCmdletBase
        [Parameter(ParameterSetName = GetAzureRmAlertRuleWithNameParamGroup, Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name")]
        [Parameter(ParameterSetName = GetAzureRmAlertRuleWithUriParamGroup, Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name")]
        [ValidateNotNullOrEmpty]
-        public string ResourceGroup { get; set; }
+        public string ResourceGroupName { get; set; }


@gucalder same comment about adding an alias to ResourceGroup

cormacpayne · 2017-10-29T21:45:35Z

src/ResourceManager/Insights/Commands.Insights/Autoscale/AddAzureRmAutoscaleSettingCommand.cs

@@ -63,7 +64,7 @@ public class AddAzureRmAutoscaleSettingCommand : ManagementCmdletBase
        [Parameter(ParameterSetName = AddAzureRmAutoscaleSettingCreateParamGroup, Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name")]
        [Parameter(ParameterSetName = AddAzureRmAutoscaleSettingUpdateParamGroup, Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name")]
        [ValidateNotNullOrEmpty]
-        public string ResourceGroup { get; set; }
+        public string ResourceGroupName { get; set; }


@gucalder same comment about adding an alias to ResourceGroup

cormacpayne · 2017-10-29T21:46:07Z

src/ResourceManager/Insights/Commands.Insights/Autoscale/GetAzureRmAutoscaleSettingCommand.cs

@@ -37,7 +37,7 @@ public class GetAzureRmAutoscaleSettingCommand : ManagementCmdletBase
        /// </summary>
        [Parameter(ParameterSetName = GetAzureRmAutoscaleSettingParamGroup, Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name")]
        [ValidateNotNullOrEmpty]
-        public string ResourceGroup { get; set; }
+        public string ResourceGroupName { get; set; }


@gucalder same comment about adding an alias to ResourceGroup

cormacpayne · 2017-10-29T21:46:35Z

src/ResourceManager/Insights/Commands.Insights/Autoscale/AddAzureRmAutoscaleSettingCommand.cs

@@ -41,6 +41,7 @@ public class AddAzureRmAutoscaleSettingCommand : ManagementCmdletBase
        /// </summary>
        [Parameter(ParameterSetName = AddAzureRmAutoscaleSettingUpdateParamGroup, Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The complete spec of an AutoscaleSetting")]
        [ValidateNotNullOrEmpty]
+        [Alias("InputObject")]


@gucalder I would make InputObject the actual name of the parameter and add an alias to SettingSpec

cormacpayne · 2017-10-29T21:47:06Z

...ResourceManager/Insights/Commands.Insights/Autoscale/RemoveAzureRmAutoscaleSettingCommand.cs

@@ -32,7 +32,7 @@ public class RemoveAzureRmAutoscaleSettingCommand : ManagementCmdletBase
        /// </summary>
        [Parameter(ParameterSetName = RemoveAzureRmAutoscaleSettingParamGroup, Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name")]
        [ValidateNotNullOrEmpty]
-        public string ResourceGroup { get; set; }


@gucalder same comment about adding an alias to ResourceGroup

cormacpayne · 2017-10-29T21:47:54Z

src/ResourceManager/Insights/Commands.Insights/LogProfiles/AddAzureRmLogProfileCommand.cs

@@ -56,48 +56,57 @@ public class AddAzureRmLogProfileCommand : ManagementCmdletBase
        /// </summary>
        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The retention in days")]
        [ValidateNotNullOrEmpty]
-        public int? RetentionInDays { get; set; }
+        [Alias("RetentionInDays")]
+        public int? RetentionInDay { get; set; }


@gucalder for parameters with units (e.g., InDays, InMinutes, etc.), you don't have to worry about making them singluar

…nto release50ShouldProcOutChange # Conflicts: # src/ResourceManager/Insights/Commands.Insights/help/Add-AzureRmAutoscaleSetting.md # src/ResourceManager/Insights/Commands.Insights/help/Add-AzureRmLogAlertRule.md # src/ResourceManager/Insights/Commands.Insights/help/Add-AzureRmLogProfile.md # src/ResourceManager/Insights/Commands.Insights/help/Add-AzureRmMetricAlertRule.md # src/ResourceManager/Insights/Commands.Insights/help/Add-AzureRmWebtestAlertRule.md # src/ResourceManager/Insights/Commands.Insights/help/Get-AzureRmAlertRule.md # src/ResourceManager/Insights/Commands.Insights/help/Get-AzureRmAutoscaleSetting.md # src/ResourceManager/Insights/Commands.Insights/help/Get-AzureRmLog.md # src/ResourceManager/Insights/Commands.Insights/help/Get-AzureRmMetric.md # src/ResourceManager/Insights/Commands.Insights/help/Get-AzureRmMetricDefinition.md # src/ResourceManager/Insights/Commands.Insights/help/Get-AzureRmUsage.md # src/ResourceManager/Insights/Commands.Insights/help/New-AzureRmAlertRuleEmail.md # src/ResourceManager/Insights/Commands.Insights/help/New-AzureRmAlertRuleWebhook.md # src/ResourceManager/Insights/Commands.Insights/help/New-AzureRmAutoscaleNotification.md # src/ResourceManager/Insights/Commands.Insights/help/New-AzureRmAutoscaleProfile.md # src/ResourceManager/Insights/Commands.Insights/help/New-AzureRmAutoscaleWebhook.md # src/ResourceManager/Insights/Commands.Insights/help/Remove-AzureRmAlertRule.md # src/ResourceManager/Insights/Commands.Insights/help/Remove-AzureRmAutoscaleSetting.md # tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv

cormacpayne · 2017-11-01T15:31:46Z

src/ResourceManager/Insights/Commands.Insights/MonitorCmdletBase.cs

+        /// <param name="withTimeStamp">true if the message should include a timestamp, false (default) it no timestamp should be included</param>
+        protected void WriteIdentifiedWarning(string topic, string message, bool withTimeStamp = false)
+        {
+            string formattedMessage = Utilities.FormatIdentifiedMessage(


@gucalder we are going to be creating our own mechanism for custom obsolete messages and attributes soon, similar to what you are doing here, so we recommend just making this simple for the time being since it will change in the near future ☺️

Rather than using the type of the class, I would just pass in the hard-coded name from the cmdlet itself as another parameter of this function (i.e., string cmdletName, and from the GetAzureRmMetricDefinitionCommand class, you can just call this.WriteIdenfitiedWarning("Get-AzureRmMetricDefinition", "Parameter deprecation", " The DetailedOutput parameter will be deprecated in a future breaking change release").

cormacpayne · 2017-11-01T15:35:56Z

src/ResourceManager/Insights/Commands.Insights/Metrics/GetAzureRmMetricDefinitionCommand.cs

@@ -72,6 +73,8 @@ protected string ProcessParameters()
        /// </summary>
        protected override void ProcessRecordInternal()
        {
+            this.WriteIdentifiedWarning("Parameter deprecation", "The DetailedOutput parameter will be deprecated in May 2018.");


@gucalder I would change "May 2018" to "future breaking change release" - even though the next breaking change release should be May 2018, we cannot be 100% positive that is when it is right now

cormacpayne · 2017-11-01T19:58:17Z

src/ResourceManager/Insights/Commands.Insights/LogsCmdletBase.cs

@@ -36,11 +36,11 @@ public abstract class LogsCmdletBase : MonitorClientCmdletBase
        private const int MaxNumberOfReturnedRecords = 1000;
        private int MaxRecords = 0;

-        internal const string SubscriptionLevelName = "GetBySubscription";


@gucalder please revert the change made to these parameter set names - we want parameter sets to be singular strings with pascal casing. I'm not sure if this was just changed because of a merge or not.

Sorry I did not see this before I pushed the changes for the other comments.
Do you need me to make the names of the constants back to what they were? (I will need to change at least one of them since ResourceGroupName is the name of a parameter now. That's why I change them all.
Or do I need to change the value of the constants?

@gucalder sorry, I made this comment a few hours after when I was looking at a couple of last minute things on this PR. We've received feedback that parameter set names should be a string with no spaces instead of the space-delimited string you are currently using. I recently merged a PR where we fix this in all cmdlets who were using them. If it wouldn't be too much trouble, would you mind revert the set names back to what they were originally, but updating them to reflect the changes you made? That would be great ☺️

Ok.
The content of each of those constants is now a single string, Pascal casing. I also updated the documentation to reflect that.

@gucalder great, let me know when you push that update out

I checked the files and I have already pushed all those changes for parameter groups names: they are all single strings, Pascal casing.

…re/azure-powershell into release50ShouldProcOutChange

cormacpayne · 2017-11-02T22:01:13Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/129/

gucalder added 3 commits October 27, 2017 13:52

[Insights] Deprecating cmdlets: Add-AzureRmLogAlertRule and Get-Azure…

7dee903

…RmUsage

[Insights] Add the ShouldProcess functionality to all the cmdlets tha…

e3b8dfe

…t require it

[Insights] Introducting announced breaking changes, reaming arguments…

5c444d9

… and adding aliases to keep the change non-breaking, implementing ShouldProcess, announcing deprecation of plural parameter names # Conflicts: # tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv

azuresdkci added the needs-review label Oct 27, 2017

azuresdkci assigned shahabhijeet Oct 27, 2017

[Insights] Removing empty lines at the end of BreakingChangeIssues.csv

626ed19

cormacpayne suggested changes Oct 29, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Oct 29, 2017

cormacpayne assigned gucalder and unassigned shahabhijeet Oct 29, 2017

gucalder added 3 commits October 31, 2017 09:47

[Insights] Including feedback from PR

aa9cd90

Merge branch 'preview' into release50ShouldProcOutChange

d47cecd

cormacpayne reviewed Nov 1, 2017

View reviewed changes

Merge branch 'preview' into release50ShouldProcOutChange

a4b609e

cormacpayne reviewed Nov 1, 2017

View reviewed changes

gucalder added 2 commits November 1, 2017 13:23

[Insights] Including more feedback from PR

51c9f89

Merge branch 'release50ShouldProcOutChange' of https://github.com/Azu…

e90b67d

…re/azure-powershell into release50ShouldProcOutChange

markcowl changed the base branch from preview to release-5.0.0 November 2, 2017 23:25

markcowl approved these changes Nov 2, 2017

View reviewed changes

cormacpayne approved these changes Nov 3, 2017

View reviewed changes

cormacpayne merged commit baab3b7 into release-5.0.0 Nov 3, 2017

markcowl deleted the release50ShouldProcOutChange branch November 14, 2017 16:26

[Insights] Introduce announced breaking changes, implement ShouldProcess, rename parameters to singluar names and add aliases to keep it non-breaking #4875

[Insights] Introduce announced breaking changes, implement ShouldProcess, rename parameters to singluar names and add aliases to keep it non-breaking #4875

Uh oh!

Conversation

gucalder commented Oct 27, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

gucalder commented Oct 27, 2017

Uh oh!

gucalder commented Oct 27, 2017

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!

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!

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

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!

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

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!

cormacpayne commented Nov 2, 2017

Uh oh!

Uh oh!

gucalder commented Oct 27, 2017 •

edited by azuresdkci

Loading

cormacpayne Nov 1, 2017 •

edited

Loading

gucalder Nov 1, 2017 •

edited

Loading