V2A AAD Changes #4843

viverm · 2017-10-24T13:47:22Z

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

@viverm some comments that need to be addressed

In addition, please make sure of the following:

You have updated the RecoveryServices.SiteRecovery change log with information about the changes being made in this PR
You have enabled all three piping scenarios (where applicable) to the New/Remove/Set/Start/Stop/Update, etc cmdlets (I left a comment for this one somewhere in the PR)
Please cleanup the commits in this PR so that it is easier to review for both sides. I would split them up so one commit has test changes, one has the test json files, one has the new cmdlets, and another has miscellaneous updates made. You can read about how to update your commits here

cormacpayne · 2017-10-31T01:09:55Z

src/ResourceManager/RecoveryServices.SiteRecovery/AzureRM.RecoveryServices.SiteRecovery.psd1

@@ -12,7 +12,7 @@
 # RootModule = ''

 # Version number of this module.
-ModuleVersion = '0.1.6'
+ModuleVersion = '0.1.5'


@viverm looks like a bad merge caused some unnecessary changes in this file

Changed to 0.1.6

cormacpayne · 2017-10-31T01:13:04Z

...vices.SiteRecovery/Commands.RecoveryServices.SiteRecovery.Test/ScenarioTests/AsrTestsBase.cs

-                        FileAccess.Read,
-                        FileShare.Read))
+                {  
+                    if (File.ReadAllText(this.vaultSettingsFilePath).ToLower().Contains("<asrvaultcreds"))


@viverm to read from/write to files, use DataStore.ReadFromFileStream() from in the FileUtilities class in Commands.Common

https://github.com/Azure/azure-powershell/blob/preview/src/Common/Commands.Common/Utilities/FileUtilities.cs#L41

Used FileUtilities.DataStore.ReadFileAsText method

cormacpayne · 2017-10-31T01:15:06Z

...ands.RecoveryServices.SiteRecovery/AlertSetting/SetAzureRmRecoveryServicesAsrAlertSetting.cs

+    /// <summary>
+    ///     Sets Azure Site Revcovery Notification / Alert.
+    /// </summary>
+    [Cmdlet(


@viverm it looks like you're missing the SupportsShouldProcess property in this cmdlt attribute

That was the miss . Added that again.

cormacpayne · 2017-10-31T01:17:06Z

...ands.RecoveryServices.SiteRecovery/AlertSetting/SetAzureRmRecoveryServicesAsrAlertSetting.cs

+        ///     Gets or sets EmailSubscriptionOwner .Mail to subscription owner.
+        /// </summary>
+        [Parameter(ParameterSetName = ASRParameterSets.EmailToSubscriptionOwner, Mandatory = true)]
+        public SwitchParameter EmailSubscriptionOwners { get; set; }


@viverm this should be renamed to EnableEmailSubscriptionOwners

cormacpayne · 2017-10-31T01:17:46Z

...ands.RecoveryServices.SiteRecovery/AlertSetting/SetAzureRmRecoveryServicesAsrAlertSetting.cs

+                VerbsCommon.Set))
+            {
+                base.ExecuteSiteRecoveryCmdlet();
+                if (this.ShouldProcess(


@viverm why is there two calls to this.ShouldProcess and base.ExecuteSiteRecoveryCmdlet?

cormacpayne · 2017-10-31T01:21:03Z

...eRecovery/Commands.RecoveryServices.SiteRecovery/Event/GetAzureRmRecoveryServicesAsrEvent.cs

+        [Parameter(ParameterSetName = ASRParameterSets.ByParam)]
+        [Parameter(ParameterSetName = ASRParameterSets.ByFabricId)]
+        [ValidateNotNullOrEmpty]
+        public ASRFabric Fabric { get; set; }


@viverm does Fabric contain a FabricId?

cormacpayne · 2017-10-31T01:21:17Z

...eRecovery/Commands.RecoveryServices.SiteRecovery/Event/GetAzureRmRecoveryServicesAsrEvent.cs

+        ///     Gets or sets Resource Id.
+        /// </summary>
+        [Parameter(ParameterSetName = ASRParameterSets.ByFabricId, Mandatory = true)]
+        public string FabricId { get; set; }


@viverm what exactly is FabricId?

Id of ASRFabric.

cormacpayne · 2017-10-31T01:21:58Z

...eRecovery/Commands.RecoveryServices.SiteRecovery/Event/GetAzureRmRecoveryServicesAsrEvent.cs

+        [Parameter(ParameterSetName = ASRParameterSets.ByParam)]
+        [Parameter(ParameterSetName = ASRParameterSets.ByFabricId)]
+        [ValidateNotNullOrEmpty]
+        public DateTime StartTime { get; set; }


@viverm should there be a corresponding EndTime parameter?

Already there . But reordered below StartTime to give better visibility.

cormacpayne · 2017-10-31T01:27:29Z

...overy/ReplicationProtectedItem/StartAzureRmRecoveryServicesAsrResynchronizeReplicationJob.cs

+        ///     Gets or sets Resource Id.
+        /// </summary>
+        [Parameter(ParameterSetName = ASRParameterSets.ByResourceId, Mandatory = true)]
+        public string ResourceId { get; set; }


@viverm anywhere that you are using a ResourceId parameter, it should have the property ValueFromPipelineByPropertyName = true so users are able to pipe the result of Get/Find-AzureRmResource to a cmdlet with a ResourceId parameter

cormacpayne · 2017-10-31T01:32:05Z

...overy/Commands.RecoveryServices.SiteRecovery/VCenter/NewAzureRmRecoveryServicesAsrVCenter.cs

+        /// </summary>
+        [Parameter(Mandatory = true, ValueFromPipeline = true)]
+        [ValidateNotNullOrEmpty]
+        public ASRFabric Fabric { get; set; }


@viverm parameter set for ResourceId of Fabric being provided? Please make sure that all cmldets that should support piping have the following three scenarios enabled:

By object - involves using an InputObject parameter that has ValueFromPipeline = true

By resource id - involves using a ResourceId parameter that has ValueFromPipelineByPropertyName = true

By command line - involves using a Name and ResourceGroupName parameter with no ability to accept its value from the pipeline; this is your default parameter set

First two scenario fits for ASR.
Getting the PSObject by Name and ResourceGroupName parameter is not possible in ASR Case we need additional parameter like vault , protection container etc on case to case basis.
Name is mainly server generated Id and hard to remember.

markcowl · 2017-10-31T21:49:46Z

@viverm It looks like you haven't committed and pushed the changes you mention in your responses to review comments.

viverm · 2017-11-01T14:50:02Z

Error during build

https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/322/console

Saw similar error with other PR builds/

d:\workspace\powershell\src\ResourceManager\Common\Commands.ScenarioTests.ResourceManager.Common\EnvironmentSetupHelper.cs(427): error : Microsoft.Azure.Commands.Sql.Test.ScenarioTests.VirtualNetworkRuleTest.TestVirtualNetworkRuleRemove [FAIL] [d:\workspace\powershell\build.proj]
04:47:22 d:\workspace\powershell\src\ResourceManager\Common\Commands.ScenarioTests.ResourceManager.Common\EnvironmentSetupHelper.cs(427): error : Microsoft.Azure.Commands.Sql.Test.ScenarioTests.VirtualNetworkRuleTest.TestVirtualNetworkRuleCreateAndUpdate [FAIL] [d:\workspace\powershell\build.proj]
04:47:22 d:\workspace\powershell\src\ResourceManager\Common\Commands.ScenarioTests.ResourceManager.Common\EnvironmentSetupHelper.cs(427): error : Microsoft.Azure.Commands.Sql.Test.ScenarioTests.VirtualNetworkRuleTest.TestVirtualNetworkRuleGet [FAIL] [d:\workspace\powershell\build.proj]

cormacpayne · 2017-11-02T05:33:10Z

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

cormacpayne

@viverm a few additional comments

I kicked off an on-demand job, and I suspect that StaticAnalysis will fail from the breaking change tool and/or signature verifier. If so, please check the Build Artifacts of the job, and look for the severity 0 or 1 exceptions in the BreakingChangeIssues.csv and/or SignatureIssues.csv files

cormacpayne · 2017-11-02T05:33:57Z

src/ResourceManager/RecoveryServices.SiteRecovery/AzureRM.RecoveryServices.SiteRecovery.psd1

@@ -170,7 +231,7 @@ PrivateData = @{
        Tags = 'Azure','ResourceManager','ARM','SiteRecovery'

        # A URL to the license for this module.
-        LicenseUri = 'https://aka.ms/azps-license'
+        LicenseUri = 'https://raw.githubusercontent.com/Azure/azure-powershell/preview/LICENSE.txt'


@viverm please revert this change

cormacpayne · 2017-11-02T05:34:22Z

.../RecoveryServices.SiteRecovery/Commands.RecoveryServices.SiteRecovery/Utilities/Utilities.cs

@@ -153,7 +155,7 @@ public static string GetDefaultPath()
                throw new Exception("Invalid ARM ID");
            }

-            for (var i = 1; i < armFields.Length; i = i + 2)
+            for (var i = 0; i < armFields.Length; i = i + 2)


@viverm shouldn't this always fail with an IndexOutOfRangeException?

For example, if armFields has five elements, this loop will get the following elements:

i == 0: 1st (idx 0) and 2nd (idx 1)
i == 2: 3rd (idx 2) and 4th (idx 3)
i == 4: 5th (idx 4) and 6th (idx 5 DOESN'T EXIST!)

here we were expecting things in pair like 1st will be the 2nd the value .
We are using the other utility method to get the value .this call is not used anymore .
Marked this as part of cleanup activity,

cormacpayne · 2017-11-02T05:35:03Z

...y/Commands.RecoveryServices.SiteRecovery/Vault/SetAzureRmRecoveryServicesAsrVaultSettings.cs

@@ -75,25 +76,28 @@ public override void ExecuteSiteRecoveryCmdlet()
            {
                using (var powerShell = System.Management.Automation.PowerShell.Create())


@viverm if the powerShell object is not being used anymore, we should remove the using

cormacpayne · 2017-11-02T05:37:47Z

...s.RecoveryServices.SiteRecovery/RecoveryPlan/UpdateAzureRmRecoveryServicesAsrRecoveryPlan.cs

@@ -148,6 +148,20 @@ public override void ExecuteSiteRecoveryCmdlet()
                                            VmId = ((HyperVReplicaBlueReplicationDetails)item
                                                .Properties.ProviderSpecificDetails).VmId;
                                        }
+                                        else if (item.Properties.ProviderSpecificDetails
+                                                .GetType() ==


@viverm nit: for the lines where .GetType() is by itself, can you tab it to be consistent with the other instances of it above

It looks like this is also the case in NewAzureRmRecoveryServicesAsrRecoveryPlan.cs

…wershell into Final_V2A_AAD

cormacpayne

@viverm a few minor questions, otherwise LGTM

cormacpayne · 2017-11-02T18:44:56Z

...eRecovery/Commands.RecoveryServices.SiteRecovery/Event/GetAzureRmRecoveryServicesAsrEvent.cs

+        [Parameter(ParameterSetName = ASRParameterSets.ByParam)]
+        [Parameter(ParameterSetName = ASRParameterSets.ByFabricId)]
+        [ValidateNotNullOrEmpty]
+        public ASRFabric Fabric { get; set; }


@viverm raising this again - if a Fabric has a FabricId, why does the user need to provide one? It seems like you're just using it to get the Name of the fabric, but you already receive the name from the mandatory Fabric parameter that a user would pass.

We can make is mutually exclusive and break in two paramset. If the user has fabricId or fabricObject he can directly call it.

cormacpayne · 2017-11-02T18:51:20Z

...ry/Commands.RecoveryServices.SiteRecovery/VCenter/RemoveAzureRmRecoveryServicesAsrVCenter.cs

+        /// <summary>
+        ///     ProcessRecord of the command.
+        /// </summary>
+        public override void ExecuteSiteRecoveryCmdlet()


@viverm can we add a parameter set that allows users to provide a Fabric and Name? This will be the default parameter set that will take care of the command line scenario and is similar to what is done with the Get-AsrVCenter cmdlet.

cormacpayne · 2017-11-02T19:03:09Z

...ry/Commands.RecoveryServices.SiteRecovery/VCenter/RemoveAzureRmRecoveryServicesAsrVCenter.cs

+            if (this.ShouldProcess(vCenterName, VerbsCommon.Remove))
+            {
+                var response = this.RecoveryServicesClient.RemoveAzureRmSiteRecoveryvCenter(
+                    fabricName,


@viverm just a general question for cmdlets that have a Fabric parameter (and this might have been discussed already): why don't we make the user provide a FabricName parameter instead of the entire object? That way, they don't have to get the Fabric object each time they want to run these cmdlets and can just provide a string instead. Is the FabricName hard to discover for the user, or difficult to remember?

Fabric Name is ASR generated id which is unique . It is Difficult for user to remember and pass it. user will store and pass it so he can fabric object like he is doing in other cmdlets.

cormacpayne · 2017-11-06T06:10:55Z

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

viverm · 2017-11-06T07:28:21Z

@cormacpayne Build Completed
https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/140/console

viverm requested a review from cormacpayne October 24, 2017 13:47

viverm mentioned this pull request Oct 24, 2017

ASR V2A Replication Provider + AAD Support #4688

Closed

13 tasks

azuresdkci added the needs-review label Oct 24, 2017

azuresdkci assigned shahabhijeet Oct 24, 2017

cormacpayne suggested changes Oct 31, 2017

View reviewed changes

cormacpayne added Cmdlet Review Complete needs-revision and removed needs-review labels Oct 31, 2017

cormacpayne assigned viverm and unassigned shahabhijeet Oct 31, 2017

viverm added 4 commits November 1, 2017 11:23

AAD Changes

b121766

ASR V2A powershell new cmdlet

884fab1

ASR V2A powershell update cmdlet

4107edd

Test Changes

7f97411

viverm force-pushed the Final_V2A_AAD branch from ebbf4a6 to 7f97411 Compare November 1, 2017 06:00

Merge branch 'preview' into Final_V2A_AAD

7eb1705

cormacpayne added needs-review and removed needs-revision labels Nov 1, 2017

cormacpayne assigned cormacpayne and unassigned viverm Nov 1, 2017

cormacpayne suggested changes Nov 2, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Nov 2, 2017

cormacpayne assigned viverm and unassigned cormacpayne Nov 2, 2017

viverm added 2 commits November 2, 2017 19:40

misc fix

5650367

Merge branch 'Final_V2A_AAD' of https://github.com/AsrOneSdk/azure-po…

b1bbbb4

…wershell into Final_V2A_AAD

viverm assigned cormacpayne and unassigned viverm Nov 2, 2017

cormacpayne suggested changes Nov 2, 2017

View reviewed changes

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

markcowl added the Merge Conflicts label Nov 2, 2017

resolve merge conflict

c52c604

viverm added 3 - Review and removed Merge Conflicts 3 - Review needs-revision labels Nov 3, 2017

cormacpayne added Waiting for On Demand and removed needs-review labels Nov 6, 2017

cormacpayne removed their assignment Nov 6, 2017

cormacpayne approved these changes Nov 6, 2017

View reviewed changes

cormacpayne merged commit e82c8a2 into Azure:release-5.0.0 Nov 6, 2017

viverm deleted the Final_V2A_AAD branch November 19, 2017 12:58

		@@ -75,25 +76,28 @@ public override void ExecuteSiteRecoveryCmdlet()
		{
		using (var powerShell = System.Management.Automation.PowerShell.Create())

V2A AAD Changes #4843

V2A AAD Changes #4843

Uh oh!

Conversation

viverm commented Oct 24, 2017 • edited by azuresdkci 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!

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!

markcowl commented Oct 31, 2017

Uh oh!

viverm commented Nov 1, 2017

Uh oh!

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

Uh oh!

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!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viverm commented Oct 24, 2017 •

edited by azuresdkci

Loading

cormacpayne commented Nov 2, 2017 •

edited

Loading

viverm Nov 3, 2017 •

edited

Loading