Add Data Sync PowerShell Cmdlets to Azure.SQL #4037

fhljys · 2017-05-26T12:47:00Z

Description

Azure SQL Data Sync is a new feature with a new set of REST API on the existing Azure SQL RP (Microsoft.Sql namespace). Data Sync feature is mainly used in two scenarios: one is sync schema between two Azure SQL databases, and the other is sync schema between an Azure SQL Database and an on-premises SQL server database. Currently, the REST API is in public review. And this PR is to add PowerShell Cmdlets for data sync feature.

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.

[Y] I have read the contribution guidelines.
[Y] If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
[Y] If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

[Y] Title of the pull request is clear and informative.
[Y] 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.
[Y] The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

[Y] Pull request includes test coverage for the included changes.
[Y] 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

[Y] 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.
[Y] Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

[Y] Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
[Y] 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.
[Y] Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

msftclas · 2017-05-26T12:47:04Z

@fhljys,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-05-26T12:50:10Z

Can one of the admins verify this patch?

fhljys · 2017-05-30T05:15:38Z

@akromm Hi Adam, could you please help review this one? Thanks a lot!

jaredmoo

Minor comments, mainly looks good

jaredmoo · 2017-05-30T15:37:13Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/Common.ps1

+function Get-SqlSyncGroupTestEnvironmentParameters ($testSuffix)
+{
+	return @{ 
+			  intervalInSeconds = 300;


nit: bad indentation here due to tabs

I will fix this. Thanks.

jaredmoo · 2017-05-30T15:37:23Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/Common.ps1

+#>
+function Get-SqlSyncMemberTestEnvironmentParameters ($testSuffix)
+{
+	return @{ 


nit: bad indentation here due to tabs

I will fix this.

jaredmoo · 2017-05-30T15:55:34Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/NewAzureSqlSyncAgent.cs

+        /// <summary>
+        /// Gets or sets the name of the server on which syncDB is hosted
+        /// </summary>
+        [Parameter(Mandatory = true,


Could you make SyncDatabaseServerName and SyncDatabaseResourceGroupName non-mandatory, use the value of ServerName and ResourceGroupName by default? That way you don't have to specify it if sync agent server and sync db server are the same.

jaredmoo · 2017-05-30T16:01:55Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/NewAzureSqlSyncAgent.cs

+                && MyInvocation.BoundParameters.ContainsKey("SyncDatabaseServerName")
+                && MyInvocation.BoundParameters.ContainsKey("SyncDatabaseName"))
+            {
+                this.syncDatabaseId = string.Format("resourceGroups/{0}/providers/Microsoft.Sql/servers/{1}/databases/{2}", 


Where does "/subscriptions/{id}/" prefix get added? can you comment?

It was added in AzureSqlDataSyncCommunicator. I will add a comment in the code.

jaredmoo · 2017-05-30T16:03:11Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/NewAzureSqlSyncGroup.cs

+        /// Gets or sets the frequency (in seconds) of doing data synchronization.
+        /// </summary>
+        [Parameter(Mandatory = false, HelpMessage = "The frequency (in seconds) of doing data synchronization. Default is -1, which means the auto synchronization is not enabled.")]
+        public int? IntervalInSeconds { get; set; }


Nice, perhaps consider adding IntervalInMinutes, IntervalInHours as different parameter sets so that user doesn't need to do math

Thank you for your suggestions. This will be considered in future improvement.

Any reason not to use a TimeSpan interval?

Its unit is seconds. It is more like a frequency than a time span.

akromm-zz · 2017-05-26T17:33:59Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/AzureSqlSyncGroupCmdletBase.cs

+            }
+            catch (Newtonsoft.Json.JsonReaderException)
+            {
+                throw new PSArgumentException("The schema file is empty or invalid!", "SchemaFile");


Can you provide a link to a page that describes the expected schema?

The official doc is not published yet. Could we add this in next sprint?

akromm-zz · 2017-05-26T17:37:56Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/NewAzureSqlSyncGroup.cs

+        /// Gets or sets the frequency (in seconds) of doing data synchronization.
+        /// </summary>
+        [Parameter(Mandatory = false, HelpMessage = "The frequency (in seconds) of doing data synchronization. Default is -1, which means the auto synchronization is not enabled.")]
+        public int? IntervalInSeconds { get; set; }


I don't think this needs to be nullable.

I will change it to int.

akromm-zz · 2017-05-26T17:39:43Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/NewAzureSqlSyncGroup.cs

+        [Parameter(Mandatory = true,
+           HelpMessage = "The database used to store sync related metadata.")]
+        [ValidateNotNullOrEmpty]
+        public string SyncDatabaseName { get; set; }


Can you sort all of these so that the required parameters appear first in the list.

OK. Thanks.

akromm-zz · 2017-05-26T17:43:12Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/NewAzureSqlSyncMember.cs

+        [Parameter(Mandatory = true,
+            ParameterSetName = AzureSqlSet,
+            HelpMessage = "The Azure SQL Server Name of the member database.")]
+        public string MemberServerName { get; set; }


please sort such that required parameters appear before optional

akromm-zz · 2017-05-26T17:53:56Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/RemoveAzureSqlSyncGroup.cs

+        /// </summary>
+        public override void ExecuteCmdlet()
+        {
+            if (!Force.IsPresent && !ShouldProcess(


I think this is already done here:
https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBase.cs#L137

akromm-zz · 2017-05-30T18:06:48Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/RemoveAzureSqlSyncMember.cs

+            if (!Force.IsPresent && !ShouldProcess(
+               string.Format(CultureInfo.InvariantCulture, Microsoft.Azure.Commands.Sql.Properties.Resources.RemoveAzureSqlSyncMemberDescription, this.SyncMemberName, this.SyncGroupName),
+               string.Format(CultureInfo.InvariantCulture, Microsoft.Azure.Commands.Sql.Properties.Resources.RemoveAzureSqlSyncMemberWarning, this.SyncMemberName, this.SyncGroupName),
+               Microsoft.Azure.Commands.Sql.Properties.Resources.ShouldProcessCaption))


This is already done/covered by:
https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Sql/Commands.Sql/Common/AzureSqlCmdletBase.cs#L137

akromm-zz · 2017-05-30T18:43:38Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/SetAzureSqlSyncGroup.cs

+        /// Gets or sets the frequency (in seconds) of doing data synchronization
+        /// </summary>
+        [Parameter(Mandatory = false, HelpMessage = "The frequency (in seconds) of doing data synchronization. Default is -1, which means the auto synchronization is not enabled.")]
+        public int? IntervalInSeconds { get; set; }


This doesn't need to be nullable.

akromm-zz · 2017-05-30T18:50:58Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/SetAzureSqlSyncMember.cs

+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true,
+            HelpMessage = "The database type.")]
+        [ValidateSet("AzureSqlDatabase", IgnoreCase = true)]
+        public string DatabaseType { get; set; }


This parameter seems unnecessary. It is marked as mandatory and it can only have 1 value. Furthermore it is not used anywhere.

Ok. I will delete it.

akromm-zz · 2017-05-30T18:59:58Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Model/AzureSqlSyncGroupModel.cs

+        /// <summary>
+        /// Gets or sets the frequency (in seconds) time of doing data synchronization
+        /// </summary>
+        public int? IntervalInSeconds { get; set; }


Should this be nullable? What does it mean if this is null?

Additionally consider changing this to a TimeSpan object.

The service layer will use the default value when this is null. It is the business logic requirement of service layer. I have changed Power Shell parameters to 'int' instead of 'int?'. Will keep it as it here.

If there is a server default this is appropriate. In PowerShell, you can make this parameter optional and check BoundParameters to see if the user provided it. Limiting the number of values that the user has to provide is a good thing

akromm-zz · 2017-05-30T19:00:32Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Model/AzureSqlSyncGroupSchemaModel.cs

+                Tables = syncGroupSchemaTable
+            };
+        }
+


Nit: remove blank link.

OK. Thanks.

fhljys · 2017-05-31T10:41:58Z

@akromm Hi Adam, I have updated this code to resolve your comments. Please help me review it again.

fhljys · 2017-06-02T01:52:09Z

@akromm Hi Adam, I pushed a commit to improve the help doc xml, which dismissed your signoff accidently. Could you please help me review this again. Very sorry for the trouble. Thanks a lot.

fhljys · 2017-06-18T14:39:50Z

@markcowl Please help me review the changes. Thanks.

markcowl · 2017-06-19T04:52:01Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/RemoveAzureSqlSyncAgent.cs

+        /// </summary>
+        public override void ExecuteCmdlet()
+        {
+            if (!Force.IsPresent && !ShouldProcess(


This is not the correct way to implement ShouldProcess. This should be implemented as

{ if (Force || ShouldContinue) { <execute action>

You can get this behavior using ConfirmAction(bool, string, string, Action) as well

markcowl · 2017-06-19T04:52:46Z

src/ResourceManager/Sql/Commands.Sql/Data Sync/Cmdlet/RemoveAzureSqlSyncMember.cs

        {
-            return string.Format("{0}.{1}", this.ServerName, this.DatabaseName);
+            if (!Force.IsPresent && !ShouldProcess(


same comment

markcowl · 2017-06-19T04:53:36Z

src/ResourceManager/Sql/Commands.Sql/Microsoft.Azure.Commands.Sql.dll-Help.xml

@@ -15507,6 +15507,14 @@ ResourceGroup01   Server01   DBAs        40b79501-b343-44ed-9ce7-da4c8cc7353b</d
 <maml:uri /></dev:type>
 <dev:defaultValue>False</dev:defaultValue>
 </command:parameter>
+<command:parameter required="false" variableLength="true" globbing="false" pipelineInput="False" position="named" aliases="none"><maml:name>Force</maml:name>


this file should not be included in the repo

fhljys · 2017-06-19T23:44:38Z

@markcowl Hi Mark, please help review this one. Thank you very much!

markcowl · 2017-06-20T22:04:46Z

on demand run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1686/

fhljys · 2017-06-21T01:40:11Z

@markcowl @cormacpayne @azuresdkci Hi Mark and Team, the on demand run finished successfully. Is there anything need I do to finish this PR? Thanks a lot.

fhljys added 4 commits May 26, 2017 19:10

Add Data Sync Legacy SDK

fb5e65d

Add Data Sync PowerShell Cmdlets

89e2b92

Add test cases and session records for Data Sync PowerShell Cmdlets

c1fb0de

Update session records, Update test cases, Update help.xml

59a1baf

msftclas added the cla-not-required label May 26, 2017

azuresdkci added the needs-review label May 26, 2017

azuresdkci assigned markcowl May 26, 2017

markcowl assigned akromm-zz and unassigned markcowl May 26, 2017

Update Sync Agent Creation Code and Session Records

cf01bba

jaredmoo reviewed May 30, 2017

View reviewed changes

resolve comments

e467f9c

akromm-zz suggested changes May 30, 2017

View reviewed changes

Resolve comments

aac3a5c

fangl added 2 commits May 31, 2017 18:44

Merge remote-tracking branch 'refs/remotes/upstream/preview'

55aab78

Update help doc

3dc64f8

akromm-zz previously approved these changes May 31, 2017

View reviewed changes

Update help doc xml

22d6568

fhljys dismissed akromm-zz’s stale review via 22d6568 June 1, 2017 05:22

akromm-zz previously approved these changes Jun 2, 2017

View reviewed changes

Add alias, make some parameters positional.

7ce9e74

cormacpayne assigned fhljys and jaredmoo Jun 12, 2017

cormacpayne added needs-revision and removed needs-review labels Jun 12, 2017

fangl added 3 commits June 18, 2017 11:21

Resolve comments --- except remove cmdlets

2c87d78

Fix remove cmdlets

2da71b0

Update session records

1310355

fhljys dismissed jaredmoo’s stale review via 1310355 June 18, 2017 12:00

fhljys assigned markcowl and unassigned fhljys Jun 18, 2017

fhljys added needs-review and removed needs-revision labels Jun 18, 2017

Merge remote-tracking branch 'refs/remotes/upstream/preview'

b4c2966

Update changelog

9549223

markcowl requested changes Jun 19, 2017

View reviewed changes

markcowl assigned fhljys and unassigned markcowl Jun 19, 2017

markcowl added needs-revision and removed needs-review labels Jun 19, 2017

Update Remove cmdlets and changelog

cf2c19d

fhljys assigned markcowl and unassigned fhljys Jun 19, 2017

fhljys added needs-review and removed needs-revision labels Jun 19, 2017

markcowl approved these changes Jun 20, 2017

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels Jun 20, 2017

jaredmoo approved these changes Jun 21, 2017

View reviewed changes

jaredmoo removed the Waiting for On Demand label Jun 22, 2017

markcowl merged commit b87bc4e into Azure:preview Jun 22, 2017

Add Data Sync PowerShell Cmdlets to Azure.SQL #4037

Add Data Sync PowerShell Cmdlets to Azure.SQL #4037

Uh oh!

Conversation

fhljys commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

msftclas commented May 26, 2017

Uh oh!

azuresdkci commented May 26, 2017

Uh oh!

fhljys commented May 30, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhljys May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

fhljys May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

fhljys commented May 26, 2017 •

edited

Loading

fhljys May 30, 2017 •

edited

Loading

fhljys May 31, 2017 •

edited

Loading