Skip to content

Get Container and Unregister Container for AzureSql #324

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

Merged
merged 2 commits into from
May 6, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,14 @@ public static AzureRmRecoveryServicesBackupContainerBase GetContainerModel(Prote
{
containerModel = new AzureRmRecoveryServicesBackupIaasVmContainer(protectionContainer);
}
if (protectionContainer.Properties.GetType() == typeof(MabProtectionContainer))
else if (protectionContainer.Properties.GetType() == typeof(MabProtectionContainer))
{
containerModel = new AzureRmRecoveryServicesMabContainer(protectionContainer);
}
else if (protectionContainer.Properties.GetType() == typeof(AzureSqlProtectionContainer))
{
containerModel = new AzureRmRecoveryServicesAzureSqlContainer(protectionContainer);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put some trace if no type is is matched.

}

return containerModel;
Expand Down
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@ namespace Microsoft.Azure.Commands.RecoveryServices.Backup.Cmdlets.Models
{
public class AzureRmRecoveryServicesAzureSqlContainer : AzureRmRecoveryServicesBackupContainerBase
{
public ContainerRegistrationStatus Status { get; set; }
public AzureRmRecoveryServicesAzureSqlContainer(ProtectionContainerResource protectionContainer)
: base(protectionContainer)
{
AzureSqlProtectionContainer sqlProtectionContainer = (AzureSqlProtectionContainer)protectionContainer.Properties;
Status = EnumUtils.GetEnum<ContainerRegistrationStatus>(sqlProtectionContainer.RegistrationStatus);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@ public static ContainerType GetPsContainerType(string containerType)
{
return ContainerType.Windows;
}
else if (containerType == ContainerType.AzureSqlContainer.ToString())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this comparison be done against the corresponding ContainerType of the Hydra DLL?

{
return ContainerType.AzureSqlContainer;
}
else
{
throw new Exception("Unsupported ContainerType: " + containerType);
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ Please contact Microsoft for further assistant.</value>
<value>Please provide AzureRmBackupManagementServer of BackupEngineType as DpmEngine or DpmVenusEngine and provide BackupManagementType as SCDPM. Provided AzureRmBackupManagementServer has BackupEngineType {0} and backupManagementType {1} which is not valid.</value>
</data>
<data name="UnsupportedContainerException" xml:space="preserve">
<value>Please provide Container of containerType as Windows and backupManagementType as MARS. Provided Container has containerType {0} and backupManagementType {1} which is invalid.</value>
<value>Please provide Container of containerType as Windows and backupManagementType as MARS or Container of containerType as AzureSQL and backupManagementType as AzureSQL. Provided Container has containerType {0} and backupManagementType {1} which is invalid.</value>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider something like:
'Please provide either 1. a Container object whose container type is "Windows" and backup management type is "MARS" or 2. a Container object whose container type is "AzureSQL" and backup management type is "AzureSQL". Provided Container object's container type is "{0}" and backup management type is "{1}" which is invalid.'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not taking this one. Scrubbing will finally happen by PM

</data>
<data name="GetRPErrorInputDatesShouldBeInUTC" xml:space="preserve">
<value>Please specify startdate and enddate in UTC format</value>
Expand Down Expand Up @@ -447,6 +447,6 @@ Please contact Microsoft for further assistant.</value>
<value>BackupManagementType provider for ContainerType {0} is incorrect.</value>
</data>
<data name="AllowedSqlRetentionRange" xml:space="preserve">
<value>RetentionCount in Weeks should be 1-520, in Months should be 1-120 and Years should be 1-10</value>
<value>Retention count in Weeks should be in between 1-520, in Months should be in between 1-120 and Years should be in between 1-10</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,25 @@ public ProtectionPolicyResponse ModifyPolicy()

public List<Models.AzureRmRecoveryServicesBackupContainerBase> ListProtectionContainers()
{
throw new NotImplementedException();
string name = (string)this.ProviderData.ProviderParameters[ContainerParams.Name];

ProtectionContainerListQueryParams queryParams = new ProtectionContainerListQueryParams();

queryParams.BackupManagementType = Microsoft.Azure.Management.RecoveryServices.Backup.Models.BackupManagementType.AzureSql.ToString();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use alias instead of referencing the complete namespace here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't alias be used if it is used in multiple places?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what my comment was about :) Please use alias instead of the complete namespace


var listResponse = HydraAdapter.ListContainers(queryParams);

List<AzureRmRecoveryServicesBackupContainerBase> containerModels = ConversionHelpers.GetContainerModelList(listResponse);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break this line. We shouldn't have lines that are too long.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how can this be broken further ???
List containerModels = ConversionHelpers.GetContainerModelList(listResponse);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like:
List<AzureRmRecoveryServicesBackupContainerBase> containerModels = ConversionHelpers.GetContainerModelList(listResponse);


if (!string.IsNullOrEmpty(name))
{
if (containerModels != null)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the containerModels object ever be null?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In null case it will just return null which means 0 number of objects back

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

As per the implementation of the GetContainerModelList, the output list can never be null. So, there's no need for this null check here.

{
containerModels = containerModels.Where(x => x.Name == name).ToList();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use better names. Consider using "containerModel" instead of "x".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lambda expression are best used with such variables. because it is understood here that x here means containerModels. do let me know if you disagree

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

}
}

return containerModels;
}

public List<AzureRmRecoveryServicesBackupEngineBase> ListBackupManagementServers()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public IPsBackupProvider GetProviderInstance(ContainerType containerType, Backup
throw new ArgumentException(String.Format(Resources.BackupManagementTypeIncorrectForContainerType, containerType));
break;
case ContainerType.AzureSqlContainer:
if (backupManagementType == BackupManagementType.AzureSql)
if (backupManagementType == BackupManagementType.AzureSql || backupManagementType == null)
providerType = PsBackupProviderTypes.AzureSql;
else
throw new ArgumentException(String.Format(Resources.BackupManagementTypeRequiredForContainerType, containerType));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class GetAzureRmRecoveryServicesBackupContainer : RecoveryServicesBackupC

[Parameter(Mandatory = false, Position = 2, HelpMessage = ParamHelpMsg.Container.BackupManagementType)]
[ValidateNotNullOrEmpty]
[ValidateSet("AzureVM", "MARS")]
[ValidateSet("AzureVM", "MARS", "AzureSql")]
public string BackupManagementType { get; set; }

[Parameter(Mandatory = false, Position = 3, HelpMessage = ParamHelpMsg.Container.Name)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,14 @@ public override void ExecuteCmdlet()
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic appears to be quite specific to the workload. Shouldn't this be in the corresponding provider for a cleaner implementation?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this need to move to provider model in next iteration

base.ExecuteCmdlet();

if (Container.ContainerType != ContainerType.Windows || Container.BackupManagementType != BackupManagementType.MARS)
if (!((Container.ContainerType == ContainerType.Windows && Container.BackupManagementType == BackupManagementType.MARS) ||
(Container.ContainerType == ContainerType.AzureSqlContainer && Container.BackupManagementType == BackupManagementType.AzureSql)))
{
throw new ArgumentException(String.Format(Resources.UnsupportedContainerException, Container.ContainerType, Container.BackupManagementType));
}
AzureRmRecoveryServicesMabContainer mabContainer = Container as AzureRmRecoveryServicesMabContainer;
string containerName = mabContainer.Name;
string containerName = Container.Name;
if (Container.ContainerType == ContainerType.AzureSqlContainer)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use braces

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do this change

containerName = ContainerConstansts.SqlContainerNamePrefix + containerName;
HydraAdapter.UnregisterContainers(containerName);
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,9 @@ public class PolicyConstants
public const int MinPolicyNameLength = 3;
public const int MaxPolicyNameLength = 150;
}

public class ContainerConstansts
{
public const string SqlContainerNamePrefix = "AzureSqlContainer;";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prefix seems to be ContainerType + ";". Isn't it better to use the ContainerType itself rather than create a new constant for better code manageability?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will happen in next iteration.

}
}