Remove AzureAuditLog as a valid kind of datasource as it's been replaced by AzureActivityLog #3379

haitch · 2017-01-11T22:04:18Z

Description

AzureAuditLog is nolonger a kind for Get-AzureRmOperationalInsightsDataSource.
- Use AzureActivityLog instead.
- We've stopped adding new AzureActivityLog datasource since, Nov 2016 Transit AzureAudit to AzureActivity, plus a small fix on Get-**DataSource #2978
- All existing datasources has been migrated over to AzureActivityLog.

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.

…ityLog

azurecla · 2017-01-11T22:04:24Z

Hi @haitch, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (haitch). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

cormacpayne · 2017-01-11T22:11:21Z

...hts/Commands.OperationalInsights/DataSources/GetAzureOperationalInsightsDataSourceCommand.cs

@@ -57,7 +57,6 @@ public class GetAzureOperationalInsightsDataSourceCommand : OperationalInsightsB
        HelpMessage = "The data source name.")]
        [Parameter(ParameterSetName = ByWorkspaceObjectByKind)]
        [ValidateSet(
-            PSDataSourceKinds.AzureAuditLog,


@haitch this is a breaking change - existing scripts that use this value for the Kind parameter will no longer work

As I stated in PR description, this is well planned, we have disabled New-*AzureAuditLogDataSource since last Nov, aliasing to New-*AzureActivityLogDataSource, so AzureAuditLogDataSource is readOnly.

From backend we have convert all existing AuditLog datasource to ActivityLog datasource.

@haitch should there be a value in the validate set for AzureActivityLog if it is replacing AzureAuditLog?

It is right there just next to it.

Oops, didn't see that below

cormacpayne · 2017-01-11T22:16:28Z

...hts/Commands.OperationalInsights/DataSources/GetAzureOperationalInsightsDataSourceCommand.cs

@@ -20,7 +20,7 @@

 namespace Microsoft.Azure.Commands.OperationalInsights
 {
-    [Cmdlet(VerbsCommon.Get, Constants.DataSource, DefaultParameterSetName = ByWorkspaceName), OutputType(typeof(List<PSDataSource>), typeof(PSDataSource))]
+    [Cmdlet(VerbsCommon.Get, Constants.DataSource, DefaultParameterSetName = ByWorkspaceNameByKind), OutputType(typeof(List<PSDataSource>), typeof(PSDataSource))]


@haitch why was this change made? This will be flagged as a breaking change by StaticAnalysis.

this is actually a fix I spotted, we have 4 parameterSet for this cmdlet, ByWorkspaceName isn't a valid parameterSet for this cmdlet. (probably copied from other cmdlets )

ByWorkspaceObjectByName {-Workspace -Name}
ByWorkspaceObjectByKind {-Workspace -Kind}
ByWorkspaceNameByName {-ResourceGroup -WorkspaceName -Name}
ByWorkspaceNameByKind {-ResourceGroup -WorkspaceName -Kind}

@haitch looking into this, this is going to be a breaking change. Existing scripts could use Get-AzureRmOperationalInsightsDataSource with no parameters, even though it would do nothing (because the ByWorkspaceName parameter set isn't checked for in any if-statement in ExecuteCmdlet).

A problem arises now that you have changed the default parameter set to ByWorkspaceNameByKind because parameter Kind is mandatory in that parameter set, which means if you run Get-AzureRmOperationalInsightsDataSource with no parameters like before, you will now be prompted to enter a value for Kind, which will break that script that uses the cmdlet with no parameters.

My suggestion is to revert the change made to the default parameter set, write a warning to users who are using the ByWorkspaceName parameter set to let them know that this parameter set will be deprecated and changed to ByWorkspaceNameByKind in an upcoming release of Azure PowerShell, and then change the if-statements in ExecuteCmdlet to include ByWorkspaceName.

haitch · 2017-01-12T00:25:53Z

...hts/Commands.OperationalInsights/DataSources/GetAzureOperationalInsightsDataSourceCommand.cs

@@ -57,7 +57,6 @@ public class GetAzureOperationalInsightsDataSourceCommand : OperationalInsightsB
        HelpMessage = "The data source name.")]
        [Parameter(ParameterSetName = ByWorkspaceObjectByKind)]
        [ValidateSet(
-            PSDataSourceKinds.AzureAuditLog,
            PSDataSourceKinds.AzureActivityLog,


pilor

Looks good as long as Azure is OK with how the breaking changes have been handled.

azurecla · 2017-01-13T00:17:50Z

Hi @haitch, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (haitch). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

haitch added 2 commits January 10, 2017 17:04

remove AzureAuditLog datasource as it has been migrated to AzureActiv…

d3acb18

…ityLog

update doc

99873d1

azurecla added the cla-not-required label Jan 11, 2017

cormacpayne suggested changes Jan 11, 2017

View reviewed changes

haitch commented Jan 12, 2017

View reviewed changes

haitch added 2 commits January 11, 2017 18:16

address comments about breaking change

e8f4847

deprecate and warning instead of remove

dfecbc0

haitch changed the base branch from dev to release-3.4.0 January 12, 2017 21:57

pilor approved these changes Jan 12, 2017

View reviewed changes

cormacpayne closed this Jan 13, 2017

cormacpayne changed the base branch from release-3.4.0 to dev January 13, 2017 00:16

cormacpayne reopened this Jan 13, 2017

cormacpayne changed the base branch from dev to release-3.4.0 January 13, 2017 00:17

azurecla added the cla-not-required label Jan 13, 2017

update upcoming breaking changes

c6de42e

cormacpayne approved these changes Jan 13, 2017

View reviewed changes

cormacpayne merged commit ae9ff9f into Azure:release-3.4.0 Jan 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove AzureAuditLog as a valid kind of datasource as it's been replaced by AzureActivityLog #3379

Remove AzureAuditLog as a valid kind of datasource as it's been replaced by AzureActivityLog #3379

Uh oh!

haitch commented Jan 11, 2017 •

edited

Loading

Uh oh!

azurecla commented Jan 11, 2017

Uh oh!

cormacpayne Jan 11, 2017

Uh oh!

haitch Jan 11, 2017

Uh oh!

cormacpayne Jan 11, 2017

Uh oh!

haitch Jan 12, 2017

Uh oh!

cormacpayne Jan 12, 2017

Uh oh!

cormacpayne Jan 11, 2017

Uh oh!

haitch Jan 11, 2017

Uh oh!

cormacpayne Jan 11, 2017

Uh oh!

haitch Jan 12, 2017

Uh oh!

pilor left a comment

Uh oh!

azurecla commented Jan 13, 2017

Uh oh!

Uh oh!

Remove AzureAuditLog as a valid kind of datasource as it's been replaced by AzureActivityLog #3379

Remove AzureAuditLog as a valid kind of datasource as it's been replaced by AzureActivityLog #3379

Uh oh!

Conversation

haitch commented Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

azurecla commented Jan 11, 2017

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!

pilor left a comment

Choose a reason for hiding this comment

Uh oh!

azurecla commented Jan 13, 2017

Uh oh!

Uh oh!

haitch commented Jan 11, 2017 •

edited

Loading