[REBASE] afs enable protection #361

siddharth7 · 2018-08-14T13:47:04Z

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-16T07:19:54Z

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

+        }
+
+        /// <summary>
+        /// Helper function to convert ps backup container model from service response.


Summary text should be Helper function to convert PS Protectable Container model from service response.

dragonfly91 · 2018-08-16T09:46:31Z

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

+        /// <summary>
+        /// Helper function to convert ps backup container model from service response.
+        /// </summary>
+        public static ContainerBase GetProtectableContainerModel(ServiceClientModel.ProtectableContainerResource protectableContainer)


Please avoid converting this object as it doesn't need to be exposed to the customer.

dragonfly91 · 2018-08-16T09:47:00Z

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

@@ -93,7 +111,7 @@ public static ContainerBase GetContainerModel(ServiceClientModel.ProtectionConta
        public static List<ContainerBase> GetContainerModelList(IEnumerable<ServiceClientModel.ProtectionContainerResource> protectionContainers)
        {
            List<ContainerBase> containerModels = new List<ContainerBase>();
-
+              


nit: Extra white space

dragonfly91 · 2018-08-16T09:47:14Z

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

+        /// <summary>
+        /// Helper function to convert ps backup container model list from service response.
+        /// </summary>
+        public static List<ContainerBase> GetProtectableContainerModelList(IEnumerable<ServiceClientModel.ProtectableContainerResource> protectableContainers)


Please avoid converting this object as it doesn't need to be exposed to the customer.

dragonfly91 · 2018-08-16T09:48:19Z

...ices/Commands.RecoveryServices.Backup.Models/AzureFileShareModels/AzureFileShareContainer.cs

+        /// and converts it in to the PS container model
+        /// </summary>
+        /// <param name="protectableContainer">Service client object representing the container</param>
+        public AzureFileShareContainer(ProtectableContainerResource protectableContainer)


Please avoid converting this object as it doesn't need to be exposed to the customer.

dragonfly91 · 2018-08-16T11:22:29Z

...overyServices/Commands.RecoveryServices.Backup.ServiceClientAdapter/BMSAPIs/ContainerAPIs.cs

+        /// <returns>List of protection containers</returns>
+        public IEnumerable<ProtectableContainerResource> ListUnregisteredContainers(
+            ODataQuery<BMSContainerQueryObject> queryFilter,
+            string skipToken = default(string),


skipToken is not used anywhere in the method

dragonfly91 · 2018-08-16T11:23:44Z

...nager/RecoveryServices/Commands.RecoveryServices.Backup.Providers/PsBackupProviderManager.cs

@@ -138,6 +138,9 @@ public IPsBackupProvider GetProviderInstance(WorkloadType workloadType)
                case WorkloadType.AzureVM:
                    providerType = PsBackupProviderTypes.IaasVm;
                    break;
+                case WorkloadType.AzureFiles:


Why do we have AzureFiles and AzureFile. Shouldn't they be same?

dragonfly91 · 2018-08-16T11:24:06Z

...overyServices/Commands.RecoveryServices.Backup.ServiceClientAdapter/BMSAPIs/ContainerAPIs.cs

@@ -23,6 +23,22 @@ namespace Microsoft.Azure.Commands.RecoveryServices.Backup.Cmdlets.ServiceClient
 {
    public partial class ServiceClientAdapter
    {
+
+        public RestAzureNS.AzureOperationResponse InquireContainers(


Please add summary

dragonfly91 · 2018-08-16T11:24:51Z

...overyServices/Commands.RecoveryServices.Backup.ServiceClientAdapter/BMSAPIs/ContainerAPIs.cs

+
+        public RestAzureNS.AzureOperationResponse InquireContainers(
+            string containerName,
+            ODataQuery<BMSContainersInquiryQueryObject> queryFilter,


Where is BMSContainersInquiryQueryObject defined?

Here : https://github.com/MabOneSdk/azure-powershell/blob/26b44278a10809fd9e19447060d7e44e22d27a5c/src/ResourceManager/RecoveryServices/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupProvider.cs#L330

dragonfly91 · 2018-08-16T11:26:19Z

...overyServices/Commands.RecoveryServices.Backup.ServiceClientAdapter/BMSAPIs/ContainerAPIs.cs

@@ -92,6 +136,25 @@ public partial class ServiceClientAdapter
            return response;
        }

+        /// <summary>
+        /// Triggers register of container catalog in service


Triggers register of container in service

dragonfly91

Review is incomplete. But you can work on these comments and publish an update and then I'll review the remaining changes as well.

dragonfly91 · 2018-08-20T09:11:52Z

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

@@ -92,8 +96,7 @@ public static ContainerBase GetContainerModel(ServiceClientModel.ProtectionConta
        /// </summary>
        public static List<ContainerBase> GetContainerModelList(IEnumerable<ServiceClientModel.ProtectionContainerResource> protectionContainers)
        {
-            List<ContainerBase> containerModels = new List<ContainerBase>();
-
+            List<ContainerBase> containerModels = new List<ContainerBase>();              


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

dragonfly91 · 2018-08-20T09:12:55Z

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

+        /// <summary>
+        /// Helper function to convert ps backup container model list from service response.
+        /// </summary>
+        public static List<ServiceClientModel.ProtectableContainerResource> GetProtectableContainerModelList(IEnumerable<ServiceClientModel.ProtectableContainerResource> protectableContainers)


Looks like this method is not needed

It is being used in AzureFilesPsBackupProvider

Please use System.Linq.ToList extension method instead. https://stackoverflow.com/questions/7617771/converting-from-ienumerable-to-list

dragonfly91 · 2018-08-20T09:18:20Z

...ResourceManager/RecoveryServices/Commands.RecoveryServices.Backup.Helpers/TrackingHelpers.cs

+        /// <param name="response">Response of the operation returned by the service.</param>
+        /// <param name="getOpStatus">Delegate method to fetch the operation status of the operation.</param>
+        /// <returns>Result of the operation once it completes.</returns>
+        public static RestAzureNS.AzureOperationResponse<T> GetOperationResult<T>(


This method looks very similar to this method. Can you confirm why this is needed?

Its different, the second parameter is different ( RestAzureNS.AzureOperationResponse vs RestAzureNS.AzureOperationResponse)

dragonfly91 · 2018-08-20T10:24:02Z

...yServices/Commands.RecoveryServices.Backup.Models/AzureFileShareModels/AzureFileShareItem.cs

+            AzureFileshareProtectedItem protectedItem = (AzureFileshareProtectedItem)protectedItemResource.Properties;
+            LastBackupStatus = protectedItem.LastBackupStatus;
+            LastBackupTime = protectedItem.LastBackupTime;
+            ProtectionPolicyName = policyName;


This can be moved to AzureItem ctor

dragonfly91 · 2018-08-20T10:25:23Z

...ices/Commands.RecoveryServices.Backup.Models/AzureFileShareModels/AzureFileShareContainer.cs

+        {
+            AzureStorageContainer azureStorageContainer =
+                (AzureStorageContainer)protectionContainer.Properties;
+            ResourceGroupName = IdUtils.GetResourceGroupName(protectionContainer.Id);


This property assignment can be moved to AzureContainer ctor

dragonfly91 · 2018-08-21T12:52:53Z

...ces/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupBackupProvider.cs

+            string storageContainer = null;
+            string classicStorageContainer = null;
+            List<ContainerBase> registeredStorageAccounts = GetRegisteredStorageAccount(vaultName, resourceGroupName);
+            foreach (var storageAccount in registeredStorageAccounts)


Instead of using foreach loop to search, use the C# Find method.

dragonfly91 · 2018-08-21T12:54:15Z

...ces/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupBackupProvider.cs

+
+            //get unregistered storage account
+            if (!isRegistered)
+            {


Similar comments apply here as well.

dragonfly91 · 2018-08-21T12:54:41Z

...ces/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupBackupProvider.cs

+        }
+
+        private void TriggerInquiry(string vaultName, string resourceGroupName,
+            string storageContainer)


should be storageContainerName

dragonfly91 · 2018-08-21T12:55:18Z

...ces/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupBackupProvider.cs

+        private void TriggerInquiry(string vaultName, string resourceGroupName,
+            string storageContainer)
+        {
+            ODataQuery<BMSContainersInquiryQueryObject> queryParam = new ODataQuery<BMSContainersInquiryQueryObject>(


Should be queryParams

dragonfly91 · 2018-08-21T12:57:07Z

...ces/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupBackupProvider.cs

+            if (protectableObjectResource == null)
+            {
+                // Container is not discovered. Throw exception
+                string errMsg = string.Format(


readable variable names

dragonfly91 · 2018-08-23T11:04:44Z

@siddharth7 Please fix the conflicts. Will review afterward the fix.

dragonfly91

Please address the comments. Otherwise LGTM.

dragonfly91 · 2018-08-27T10:02:45Z

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

+        /// <summary>
+        /// Helper function to convert ps backup container model list from service response.
+        /// </summary>
+        public static List<ServiceClientModel.ProtectableContainerResource> GetProtectableContainerModelList(IEnumerable<ServiceClientModel.ProtectableContainerResource> protectableContainers)


Please use System.Linq.ToList extension method instead. https://stackoverflow.com/questions/7617771/converting-from-ienumerable-to-list

dragonfly91 · 2018-08-27T10:14:10Z

...yServices/Commands.RecoveryServices.Backup.Models/AzureFileShareModels/AzureFileShareItem.cs

@@ -62,12 +37,11 @@ public class AzureFileShareItem : ItemBase
        /// <param name="policyName">Name of the protection policy associated with this protected item</param>
        public AzureFileShareItem(ProtectedItemResource protectedItemResource,
            string containerName, ContainerType containerType, string policyName)
-            : base(protectedItemResource, containerName, containerType)
+            : base(protectedItemResource, containerName, containerType, policyName)
        {
            AzureFileshareProtectedItem protectedItem = (AzureFileshareProtectedItem)protectedItemResource.Properties;
            LastBackupStatus = protectedItem.LastBackupStatus;


Please move the initialization for LastBackupStatus, LastBackupTime, ProtectionState and ProtectionStatus to the base class - both in VM and Files.

workload specific properties, can't move it to base class

dragonfly91 · 2018-08-27T10:15:47Z

...rceManager/RecoveryServices/Commands.RecoveryServices.Backup.Models/AzureModels/AzureItem.cs

+
+namespace Microsoft.Azure.Commands.RecoveryServices.Backup.Cmdlets.Models
+{
+    public class AzureItem : ItemBase


Please provide summary text for this class.

dragonfly91 · 2018-08-27T10:19:57Z

...r/RecoveryServices/Commands.RecoveryServices.Backup.Providers/AzureWorkloadProviderHelper.cs

@@ -12,16 +12,17 @@
 // limitations under the License.
 // ----------------------------------------------------------------------------------

-using System;


nit: Run refactoring once again

dragonfly91 · 2018-08-27T10:27:57Z

...yServices/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupProvider.cs

+            string storageContainerName = null;
+            List<ContainerBase> registeredStorageAccounts = GetRegisteredStorageAccounts(vaultName, vaultResourceGroupName);
+            ContainerBase registeredStorageAccount = registeredStorageAccounts.Find(storageAccount =>
+            string.Compare(storageAccount.Name.Split(';').Last(), storageAccountName) == 0);


nit: Please fix indent.

dragonfly91 · 2018-08-27T10:44:42Z

...yServices/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupProvider.cs

+
+            //get protectable item
+            WorkloadProtectableItemResource protectableObjectResource = null;
+            protectableObjectResource = TriggerDiscovery(vaultName, vaultResourceGroupName, azureFileShareName, storageAccountName);


Name should be GetProtectableItem

dragonfly91 · 2018-08-27T10:50:57Z

...yServices/Commands.RecoveryServices.Backup.Providers/Providers/AzureFilesPsBackupProvider.cs

+                AzureFileShareProtectableItem azureFileShareProtectableItem =
+                    (AzureFileShareProtectableItem)protectableItem.Properties;
+                if (azureFileShareProtectableItem != null &&
+                    string.Compare(azureFileShareProtectableItem.FriendlyName, azureFileShareName, true) == 0 &&


Check with PM about how to deal with case

dragonfly91 · 2018-08-27T10:56:46Z

...mmands.RecoveryServices.Backup/Cmdlets/Item/EnableAzureRmRecoveryServicesBackupProtection.cs

+        /// </summary>
+        [Parameter(Position = 2, Mandatory = true, ValueFromPipelineByPropertyName = true,
+            ParameterSetName = AzureFileShareParameterSet, HelpMessage = ParamHelpMsgs.Item.AzureFileShareName)]
+        public string FileShareName { get; set; }


This should be Name. Check PM for details.

dragonfly91 · 2018-08-27T10:57:01Z

...mmands.RecoveryServices.Backup/Cmdlets/Item/EnableAzureRmRecoveryServicesBackupProtection.cs

+            ParameterSetName = AzureFileShareParameterSet,
+            HelpMessage = ParamHelpMsgs.Item.AzureFileStorageAccountResourceGroupName)]
+        [ResourceGroupCompleter]
+        public string StorageAccountResourceGroupName { get; set; }


Need to remove this parameter

dragonfly91 · 2018-08-27T10:57:59Z

src/ResourceManager/RecoveryServices/Commands.RecoveryServices.Backup/ParamHelpMsgs.cs

@@ -36,7 +36,7 @@ internal static class Common
        {
            public const string Vault = "The Azure Backup vault object which is the parent resource.";
            public const string VaultId = "Resource ID of the Recovery Services Vault.";
-            public const string WorkloadType = "Workload type of the resource (for example: AzureVM, WindowsServer, AzureFile).";
+            public const string WorkloadType = "Workload type of the resource (for example: AzureVM, WindowsServer, Azure File).";


Check for the right values here

enable protection 14/8 updated trigger discovery file->files added tracker for enquiry added tracking for register container minor changes fix for classic storage accounts rebase changes rebase changes addressed comments used storageaccountRg instead of Rg blank lines added to file share provider removed storageaccountresourcegroupname dependancy resolved issues during rebase resolved issues during rebase rebase changes Review comments

dragonfly91 requested changes Aug 16, 2018

View reviewed changes

dragonfly91 changed the title ~~[DO NOT MERGE] afs enable protection~~ [REBASE] afs enable protection Aug 21, 2018

siddharth7 force-pushed the users/sisi/afs-enable-protection branch 2 times, most recently from 9a995e1 to d451bf9 Compare August 21, 2018 12:14

dragonfly91 requested changes Aug 21, 2018

View reviewed changes

siddharth7 force-pushed the users/sisi/afs-enable-protection branch from 1a9323d to 26b4427 Compare August 22, 2018 09:06

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-enable-protection branch from 095c6f2 to c68a8f9 Compare August 27, 2018 09:42

dragonfly91 approved these changes Aug 27, 2018

View reviewed changes

siddharth7 force-pushed the users/sisi/afs-enable-protection branch from 6313148 to b2b88f3 Compare August 27, 2018 12:28

siddharth7 added 2 commits August 27, 2018 17:59

rebase changes

863f285

siddharth7 force-pushed the users/sisi/afs-enable-protection branch from b2b88f3 to 863f285 Compare August 27, 2018 12:35

siddharth7 merged commit b06f08b into afs_dev Aug 27, 2018

[REBASE] afs enable protection #361

[REBASE] afs enable protection #361

Uh oh!

Conversation

siddharth7 commented Aug 14, 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!

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 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!

dragonfly91 commented Aug 23, 2018

Uh oh!

dragonfly91 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