-
Notifications
You must be signed in to change notification settings - Fork 2
Added Azure File Share support to disable protection #371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3f4671a
to
43464e0
Compare
@@ -62,70 +62,313 @@ public class AzureFilesPsBackupProvider : IPsBackupProvider | |||
/// </summary> | |||
/// <returns>The job response returned from the service</returns> | |||
public RestAzureNS.AzureOperationResponse EnableProtection() | |||
{ | |||
return EnableOrDisableProtection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
9450171
to
a976e23
Compare
@siddharth7 I'm still seeing the EnableOrDisableProtection method. I thought this was supposed to be EnableOrModifyProtection and DisableProtection. Did anything change from our initial agreement? |
string sourceResourceId = null; | ||
AzureFileshareProtectedItem properties = new AzureFileshareProtectedItem(); | ||
|
||
if (policy != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't depend on policy being null for this check. Expose a boolean parameter bool disableWithRetainData and use this for this check. Also, please add some comments so that the flow will be a bit more readable.
itemWorkloadType.ToString())); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this method, you are throwing an incorrect exception message with VirtualMachineIdIsEmptyOrNull. Please fix this:
private void ValidateAzureFilesModifyProtectionRequest(ItemBase itemBase,
PolicyBase policy)
{
if (itemBase == null || itemBase.GetType() != typeof(AzureFileShareItem))
{
throw new ArgumentException(string.Format(Resources.InvalidProtectionPolicyException,
typeof(AzureFileShareItem).ToString()));
}
if (string.IsNullOrEmpty(((AzureFileShareItem)itemBase).ParentContainerFabricId))
{
throw new ArgumentException(Resources.VirtualMachineIdIsEmptyOrNull);
}
}
} | ||
if (itemBase == null || itemBase.GetType() != typeof(AzureFileShareItem)) | ||
{ | ||
throw new ArgumentException(string.Format(Resources.InvalidProtectionPolicyException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that all of the error messages have been signed off from PM.
7e40303
to
e26fad9
Compare
6ace8d0
to
7744151
Compare
8632464
to
899f228
Compare
2501703
to
22ed694
Compare
} | ||
else | ||
{ | ||
return EnableOrModifyProtection(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnableOrModifyProtection(disableWithRetentionData: true);
|
||
PolicyBase policy = ProviderData.ContainsKey(ItemParams.Policy) ? | ||
(PolicyBase)ProviderData[ItemParams.Policy] : null; | ||
bool deleteBackupData = ProviderData.ContainsKey(ItemParams.DeleteBackupData) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this
string sourceResourceId = null; | ||
AzureFileshareProtectedItem properties = new AzureFileshareProtectedItem(); | ||
|
||
if (!disableWithRetentionData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider swap of conditions?
899f228
to
3a78386
Compare
3a78386
to
d91baa0
Compare
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.
General Guidelines
Testing Guidelines
Cmdlet Signature Guidelines
ShouldProcess
and haveSupportShouldProcess=true
specified in the cmdlet attribute. You can find more information onShouldProcess
here.OutputType
attribute if any output is produced - if the cmdlet produces no output, it should implement aPassThrough
parameter.Cmdlet Parameter Guidelines