Added Powershell Cmdlets to support Enabling/Disabling GeoBackupPolicy for SQL Azure DataWarehouses #2811
Conversation
|
Hi @alazad-msft, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution! TTYL, AZPRBOT; |
|
Can one of the admins verify this patch? |
|
@akromm @yoavrubin Can you guys take a look at this and sign off if this looks OK and doesn't confliuct with planned changes this release? |
|
looks good to me. What are the planned changes this might conflict with? |
| namespace Microsoft.Azure.Commands.Sql.Backup.Cmdlet | ||
| { | ||
| public abstract class AzureSqlDatabaseGeoBackupPolicyCmdletBase : | ||
| AzureSqlCmdletBase<IEnumerable<AzureSqlDatabaseGeoBackupPolicyModel>, AzureSqlDatabaseBackupAdapter> |
There was a problem hiding this comment.
Why aren't you inheriting from AzureSqlDatabaseCmdletBase, it already provides the server and database parameters
There was a problem hiding this comment.
I need a AzureSqlDatabaseBackupAdapter object to be returned with InitModelAdapter, so an override would be needed anyways.
protected override AzureSqlDatabaseBackupAdapter InitModelAdapter(AzureSubscription subscription) { return new AzureSqlDatabaseBackupAdapter(DefaultProfile.Context); }
There was a problem hiding this comment.
Discussed offline, @alazad-msft will update the inheritance
|
LGTM |
|
@azuresdkci test this please |
|
@azuresdkci add to whitelist |
| @@ -1,4 +1,4 @@ | |||
| ## 2016.08.09 version 2.0.1 | |||
There was a problem hiding this comment.
@alazad-msft Please revert changes to this file
There was a problem hiding this comment.
@markcowl Reverted ChangeLog.md
|
@alazad-msft Hey Alec, the build is failing because you are missing help for the two cmdlets that you have added. |
|
In this PR #2824 I've made a change that affects all of AzureSQL's cmdlets. Please take a look there and make the relevant changes to your new cmdlets (changes that are related to using ConfirmAction). |
|
@cormacpayne Thanks! I noticed it as well. I have made the needed updates and resolved the issues. Please continue with merge. |
| /// </summary> | ||
| [Cmdlet(VerbsCommon.Set, "AzureRmSqlDatabaseGeoBackupPolicy", | ||
| SupportsShouldProcess = true, | ||
| ConfirmImpact = ConfirmImpact.Low)] |
There was a problem hiding this comment.
Why confirmimpact.low? Normally, cmdlets that make changes would have the default confirmpmact (confirmimpact.medium)
There was a problem hiding this comment.
@markcowl Fixed - upgraded set cmdlet to ConfirmImpact.Medium.
|
@alazad-msft One nit + please pull (and rebase on using |
…t/set of geo backup policies on Azure SQL DBs: - Get-AzureRmSqlDatabaseGeoBackupPolicy - Set-AzureRmSqlDatabaseGeoBackupPolicy
…del an enum instead of string with Enabled/Disabled possible values. Reverted changelog.
…reSqlDatabaseCmdletBase instead of AzureSqlCmdletBase to inherit the ServerName and DatabaseName properties.
…act.Medium instead of Low since it makes changes to a SQL Azure resource.
|
@markcowl Fixed nit and rebased. Thanks for your kind review, Mark. |
No description provided.