[REBASE] Added AFS support to Get Policy cmdlet #360

siddharth7 · 2018-08-09T09:38:13Z

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 PassThrough 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.

dragonfly91 · 2018-08-10T09:04:05Z

...r/RecoveryServices/Commands.RecoveryServices.Backup.Helpers/Conversions/ConversionHelpers.cs

@@ -191,6 +191,31 @@ public static PolicyBase GetPolicyModel(ServiceClientModel.ProtectionPolicyResou
                sqlPolicyModel.RetentionPolicy =
                    PolicyHelpers.GetPSSimpleRetentionPolicy(azureSqlRetentionPolicy, null);
            }
+            else if (serviceClientResponse.Properties.GetType() ==
+                typeof(ServiceClientModel.AzureFileShareProtectionPolicy))
+            {


In the interest of keeping the method size short, can we introduce smaller methods like GetAzureVmPolicyModel, GetAzureFileSharePolicyModel etc?

dragonfly91 · 2018-08-10T09:10:43Z

...r/RecoveryServices/Commands.RecoveryServices.Backup.Helpers/Conversions/ConversionHelpers.cs

+                    return null;
+                }
+
+                policyModel = new AzureFileSharePolicy();


Schedule policy is missing

dragonfly91 · 2018-08-10T09:13:05Z

...ervices/Commands.RecoveryServices.Backup.Models/AzureFileShareModels/AzureFileSharePolicy.cs

+    /// <summary>
+    /// Azure FileShare specific backup policy class.
+    /// </summary>
+    public class AzureFileSharePolicy : PolicyBase


Use interim base class, AzureWorkloadPolicy, for all Azure workloads

dragonfly91 · 2018-08-10T09:18:04Z

...coveryServices/Commands.RecoveryServices.Backup.Test/ScenarioTests/AzureFile/PolicyTests.ps1

+
+	try
+	{
+		$vault = Get-AzureRmRecoveryServicesVault -ResourceGroupName $resourceGroupName


Please use Create-RecoveryServicesVault here. You can move it to a Common.ps1 under the SenarioTests folder.

dragonfly91 · 2018-08-21T11:25:09Z

...r/RecoveryServices/Commands.RecoveryServices.Backup.Helpers/Conversions/ConversionHelpers.cs

+            AzureFileSharePolicy fileSharePolicyModel = policyModel as AzureFileSharePolicy;
+            fileSharePolicyModel.WorkloadType = WorkloadType.AzureFiles;
+            fileSharePolicyModel.BackupManagementType = BackupManagementType.AzureStorage;
+            fileSharePolicyModel.RetentionPolicy = PolicyHelpers.GetPSLongTermRetentionPolicy((ServiceClientModel.LongTermRetentionPolicy)


nit: Make sure all lines in the edits you make are < 100 chars long.

dragonfly91 · 2018-08-21T11:29:32Z

...ervices/Commands.RecoveryServices.Backup.Models/AzureFileShareModels/AzureFileSharePolicy.cs

@@ -17,17 +17,8 @@ namespace Microsoft.Azure.Commands.RecoveryServices.Backup.Cmdlets.Models
    /// <summary>
    /// Azure FileShare specific backup policy class.
    /// </summary>
-    public class AzureFileSharePolicy : PolicyBase
+    public class AzureFileSharePolicy : AzureWorkloadPolicy


I think this should be renamed to AzurePolicy instead of AzureWorkloadPolicy, to maintain consistency with other similar objects.

dragonfly91 · 2018-08-21T11:34:07Z

One good practice is to run "Ctrl + K + D" in all the touched files.

dragonfly91 · 2018-08-23T11:08:02Z

Found merge conflicts in PR. Author needs to fix it before review can be done.

changed base class for AzureFileSharePolicy rebase changes renaming azurefileshareextended info rebase changes

dragonfly91 · 2018-08-29T12:19:26Z

...r/RecoveryServices/Commands.RecoveryServices.Backup.Helpers/Conversions/ConversionHelpers.cs

+        /// <summary>
+        /// Helper function to convert ps backup policy model for AzureVM from service response.
+        /// </summary>
+        public static PolicyBase GetPolicyModelForAzureIaaSVM(ServiceClientModel.ProtectionPolicyResource serviceClientResponse,


Please limit to 100 chars

dragonfly91 changed the title ~~Added AFS support to Get Policy cmdlet~~ [DO NOT MERGE] Added AFS support to Get Policy cmdlet Aug 10, 2018

dragonfly91 requested changes Aug 10, 2018

View reviewed changes

siddharth7 force-pushed the users/sisi/afs branch from 73b65f9 to 9275e8a Compare August 21, 2018 10:42

dragonfly91 requested changes Aug 21, 2018

View reviewed changes

dragonfly91 changed the title ~~[DO NOT MERGE] Added AFS support to Get Policy cmdlet~~ [REBASE] Added AFS support to Get Policy cmdlet Aug 21, 2018

dragonfly91 assigned siddharth7 Aug 23, 2018

dragonfly91 added the needs revision The PR needs revision from the author label Aug 23, 2018

siddharth7 force-pushed the users/sisi/afs branch 2 times, most recently from 6da94a1 to 3bfe94e Compare August 27, 2018 13:02

Addressed review comments

7ca71b0

changed base class for AzureFileSharePolicy rebase changes renaming azurefileshareextended info rebase changes

siddharth7 force-pushed the users/sisi/afs branch from bae582e to 7ca71b0 Compare August 28, 2018 10:20

dragonfly91 approved these changes Aug 29, 2018

View reviewed changes

format text in conversionHelper

b612af2

siddharth7 merged this pull request into afs_dev Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[REBASE] Added AFS support to Get Policy cmdlet #360

[REBASE] Added AFS support to Get Policy cmdlet #360

Uh oh!

siddharth7 commented Aug 9, 2018

Uh oh!

dragonfly91 Aug 10, 2018

Uh oh!

dragonfly91 Aug 10, 2018

Uh oh!

dragonfly91 Aug 10, 2018

Uh oh!

dragonfly91 Aug 10, 2018

Uh oh!

dragonfly91 Aug 21, 2018

Uh oh!

dragonfly91 Aug 21, 2018

Uh oh!

dragonfly91 commented Aug 21, 2018

Uh oh!

dragonfly91 commented Aug 23, 2018

Uh oh!

dragonfly91 Aug 29, 2018

Uh oh!

Uh oh!

[REBASE] Added AFS support to Get Policy cmdlet #360

[REBASE] Added AFS support to Get Policy cmdlet #360

Uh oh!

Conversation

siddharth7 commented Aug 9, 2018

Description

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!

dragonfly91 commented Aug 21, 2018

Uh oh!

dragonfly91 commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!