Fix, update, and add tests for ResourceManager SQL Failover Group cmdlets #3826

dlemMSFT · 2017-04-26T00:58:49Z

Description

This pull request contains a number of changes to the Resource Manager SQL Failover Group cmdlets. There are several breaking changes -- they are removals of parameters that were previously declared obsolete.

Here is a list of the changes. This is mostly copied from the ChangeLog update.

Remove 'Tag' parameters (previously obsoleted)
Remove 'PartnerResourceGroupName' and 'PartnerServerName' parameters from Remove-AzureRmSqlDatabaseFailoverGroup cmdlet (previously obsoleted)
Add 'GracePeriodWithDataLossHours' parameter to New- and Set- cmdlets, which shall eventually replace 'GracePeriodWithDataLossHour'
Documentation has been fleshed out and updated
Change formatting of returned objects and fix some bugs where fields were not always populated
Add 'DatabaseNames' and 'PartnerLocation' properties to Failover Group object
Fix bug causing Switch- cmdlet to return immediately rather than waiting for operation to complete
Fix integer overflow bug when high grace period values are used
Adjust grace period to a minimum of 1 hour and warn user if a lower one is provided
Add and record scenario tests for Failover Group cmdlets

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

(No new cmdlets) 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.
(No new cmdlets) Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru 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.

msftclas · 2017-04-26T00:58:54Z

@dlemMSFT,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-04-26T01:00:10Z

Can one of the admins verify this patch?

akromm-zz · 2017-04-26T16:25:33Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/AzureSqlFailoverGroupCmdletBase.cs

+            {
+                // If the policy is Automatic, the grace period must be non-null. If this is an update and the grace
+                // period was non-null, use the existing grace period. Otherwise, use a default of 1.
+                gracePeriod = originalGracePeriod ?? 1;


Is it safe to assume a default value of 1 hour? Would it be better to return an error requesting the user to set the grace period?

This is a subjective choice we made in favor of user convenience. What exactly do you mean by "safe?" Avoiding data loss unless the user very explicitly opts in? Also, this would arguably be a breaking change since previously the command would have worked without specifying the grace period.

akromm-zz · 2017-04-26T16:28:19Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

        [ValidateNotNullOrEmpty]
        public FailoverPolicy FailoverPolicy { get; set; }

        /// <summary>
        /// Gets or sets the grace period with data loss for the Sql Azure Failover Group.
        /// </summary>
        [Parameter(Mandatory = false,
-            HelpMessage = "The window of grace period that we tolerate with data loss during a failover operation for the failover group.")]
+            HelpMessage = "The grace period during outage before automatic failover with data loss of the Failover Group is triggered.")]


Please re-word this. It is a bit tricky to comprehend.

akromm-zz · 2017-04-26T16:29:19Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

-        [Obsolete("This parameter will be deprecated in the next release.")]
-        public int GracePeriodWithDataLossHour { get; set; }
+        [ValidateRange(0, int.MaxValue)]
+        public int GracePeriodWithDataLossHours { get; set; }


Since this refers to a time duration I can't help but think that a TimeSpan would be a much better type than int. Please consider updating the APIs to take a timespan rather than int in the future.

How do you create timespan in powershell? [TimeSpan]::FromHours (etc) is imo not beginner-friendly, so I think Hours is fine.

What I'm trying to say is that cmdlet params should generally be int, string, or similar types that can be created as a literal value (e.g. double, decimal, etc) any data type more complex than those makes the cmdlet much harder to use for powershell beginners if there is no alternative simpler parameter.

@dlemMSFT @akromm if we're renaming this cmdlet, what do you guys think about renaming it to GracePeriodWithDataLossInHours?

I say this because I know StaticAnalysis is going to throw an exception because parameter nouns should be singular (but nouns such as InHours, InMinutes, etc. are suppressed and won't trigger an exception). Also, I think InHours sounds better with the rest of the parameter noun than Hours. Let me know what you think.

@cormacpayne I don't disagree with your points -- but the competing argument is that we have already documented the signatures in other places, and shared them with customers. It would be be more work than just refreshing this PR to make that change.

@dlemMSFT sure, no worries. Just wanted to throw it out as a suggestion.

akromm-zz · 2017-04-26T16:31:42Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

-            HelpMessage = "The tag to associate with the Azure SQL Database Failover Group")]
-        public Hashtable Tag { get; set; }
+        [Parameter(Mandatory = false, DontShow = true,
+            HelpMessage = "Whether an outage on the secondary server should trigger automatic failover of the read-only endpoint. This feature is not yet supported.")]


If the feature is not yet supported why are you adding a parameter for it?

I would remove it if I could. The parameter is already added two releases ago and cannot be removed since it is not obsoleted. The github diff doesn't seem to be doing a good job of clearly showing the changes.

akromm-zz · 2017-04-26T16:40:25Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

@@ -111,9 +122,7 @@ protected override IEnumerable<AzureSqlFailoverGroupModel> GetEntity()
            }

            // The database already exists


*Failover Group

akromm-zz · 2017-04-26T16:42:11Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/SetAzureSqlFailoverGroup.cs

        [ValidateNotNullOrEmpty]
        public FailoverPolicy FailoverPolicy { get; set; }

        /// <summary>
        /// Gets or sets the grace period with data loss for the Sql Azure Failover Group.
        /// </summary>
        [Parameter(Mandatory = false,
-            HelpMessage = "The grace period for failover with data loss of the failover group. This property defines how big of the window we tolerate for data loss during failover operation")]
+            HelpMessage = "The grace period during outage before automatic failover with data loss of the Failover Group is triggered.")]


please reword to make it easier to understand.

akromm-zz · 2017-04-26T16:43:29Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/SetAzureSqlFailoverGroup.cs

        [ValidateNotNullOrEmpty]
+        [ValidateRange(0, int.MaxValue)]
        [Obsolete("This parameter will be deprecated in the next release.")]
        public int GracePeriodWithDataLossHour { get; set; }


You have removed this parameter from other cmdlets. Can it not be removed from here also?

This wasn't removed from the other cmdlet -- the github diff is obscuring the fact that it existed previously in both.

akromm-zz · 2017-04-26T16:45:06Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Services/AzureSqlFailoverGroupAdapter.cs

            {
-                readWriteEndpoint.FailoverWithDataLossGracePeriodMinutes = null;
+                readWriteEndpoint.FailoverWithDataLossGracePeriodMinutes = checked(model.FailoverWithDataLossGracePeriodHours * 60);


If the API uses minutes why does the cmdlet require hours to be passed in?

'Hours' is representative of what we actually support. We think making it 'Hours' in PowerShell regardless of the API is the best option to avoid misleading users. We intend to change the API.

cormacpayne · 2017-04-26T18:25:29Z

@azuresdkci add to whitelist

jaredmoo · 2017-04-26T18:15:38Z

...ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/AddAzureSqlDatabaseToFailoverGroup.cs

@@ -44,19 +54,11 @@ public class AddAzureSqlDatabaseToFailoverGroup : AzureSqlFailoverGroupCmdletBas
        /// </summary>
        [Parameter(Mandatory = true,
            ValueFromPipeline = true,
-            HelpMessage = "The Azure SQL Database to be added to the secondary server.")]
+            HelpMessage = "One or more Azure SQL Databases on the Failover Group's primary server to be added to the Failover Group.")]
        [ValidateNotNullOrEmpty]
        public List<AzureSqlDatabaseModel> Database { get; set; }


Why is this database model? Shouldn't it just be database name?

The reason is that it makes it possible to do things like "Get-AzureRmSqlDatababase <...> | Add-AzureRmSqlDatabaseToFailoverGroup <...>" without the need to write any other PowerShell code. It needs to be a single command -- adding the databases one by one would take too long.

I think an argument could be made that we should also have a 'DatabaseName' parameter, but in my opinion that is redundant since this one already exists.

Sure, but PowerShell beginners do not use piping. Piping is cool, I use it all the time, but I have watched people use PowerShell for the first time and it takes a while to figure this out. In fact most powershell beginner just type the command name Add-AzureRmSqlDatabaseToFailoverGroup and then follow the prompts for required parameters.

This means in order to make your cmdlet approachable there needs to be a param set like Add-AzureRmSqlDatabaseToFailoverGroup -ServerName myserver -FailoverGroupName mygroup -DatabaseName mydatabase. Yes it's redundant once you've learned the more advanced piping technique, but we often make redundancies in order to improve UX.

In fact, the piping scenario would work if you had DatabaseName parameter with ValueFromPipelineByPropertyName=true. So it's actually DatabaseName that makes Database redundant, not the other way around :)

Sorry for being (very) unclear in my examples -- the important part is not that we are piping, it is that we are adding multiple databases at the same time. Using a model parameter, you can list all databases on a server, or list all databases in an elastic pool, and add them to a failover group at the same time (without code to select DatabaseName). Adding takes a long time, so adding many at a time is much better than adding one-by-one.

Your point about 'DatabaseName' being better for beginners or people doing tab-completion is well taken, though. We can do that in the future, but I don't want to put us at risk for the next release by making more changes in this pull request.

That's fine, please just consider it for next release :)

Note that, for future plans, we will be suggesting using models like this for piping within modules, but ValueFromPipelineByPropertyName for piping between modules (from generic resource cmdlets)

As Jared indicates, it is best to have a piping parameter set like this, and a parameter set more friendly to interactive input.

jaredmoo · 2017-04-26T18:29:00Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

-        [Obsolete("This parameter will be deprecated in the next release.")]
-        public int GracePeriodWithDataLossHour { get; set; }
+        [ValidateRange(0, int.MaxValue)]
+        public int GracePeriodWithDataLossHours { get; set; }


How do you create timespan in powershell? [TimeSpan]::FromHours (etc) is imo not beginner-friendly, so I think Hours is fine.

jaredmoo · 2017-04-26T18:32:06Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

-        [Obsolete("This parameter will be deprecated in the next release.")]
-        public int GracePeriodWithDataLossHour { get; set; }
+        [ValidateRange(0, int.MaxValue)]
+        public int GracePeriodWithDataLossHours { get; set; }


What I'm trying to say is that cmdlet params should generally be int, string, or similar types that can be created as a literal value (e.g. double, decimal, etc) any data type more complex than those makes the cmdlet much harder to use for powershell beginners if there is no alternative simpler parameter.

jaredmoo · 2017-04-26T18:39:13Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Services/AzureSqlFailoverGroupAdapter.cs

+        /// SQL server URI template used for parsing.
+        /// </summary>
+        private readonly UriTemplate _serverUriTemplate =
+            new UriTemplate("subscriptions/{subscriptionId}/resourcegroups/{resourceGroup}/providers/Microsoft.Sql/servers/{serverName}");


Shouldn't this have leading / ? i.e. /subscriptions/.... ?

jaredmoo · 2017-04-26T18:43:30Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Services/AzureSqlFailoverGroupAdapter.cs

+            model.Location = failoverGroup.Location;
+            model.DatabaseNames = RetrieveDatabaseNames(failoverGroup);
+
+            UriTemplateMatch match = _failoverGroupUriTemplate.Match(_dummyUri, new Uri(_dummyUri, failoverGroup.Id));


can you comment why dummy uri is needed?

It's either this or string.Split. Actually, after looking at how other services do it, I've converted this to use string.Split. The code is much cleaner.

cormacpayne · 2017-04-26T18:28:26Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/FailoverGroupTests.cs

        public void TestFailoverGroup()
        {
            RunPowerShellTest("Test-FailoverGroup");
        }

+        [Fact]


@dlemMSFT you will need to add the CheckIn trait to each test for them to be run during the build. For example:

[Fact] [Trait(Category.AcceptanceType, Category.CheckIn)] public void TestCreateFailoverGroup_Named() { RunPowerShellTest("Test-CreateFailoverGroup-Named"); }

cormacpayne · 2017-04-26T18:37:08Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/AzureSqlFailoverGroupCmdletBase.cs

@@ -25,16 +25,6 @@ namespace Microsoft.Azure.Commands.Sql.FailoverGroup.Cmdlet
    public abstract class AzureSqlFailoverGroupCmdletBase : AzureSqlCmdletBase<IEnumerable<AzureSqlFailoverGroupModel>, AzureSqlFailoverGroupAdapter>
    {
        /// <summary>
-        /// Gets or sets the name of the server to use.
-        /// </summary>
-        [Parameter(Mandatory = true,


@dlemMSFT any reason for removing this parameter from the base cmdlet if you are just going to add it back to each cmdlet that inherits from it?

I wanted to improve the documentation to refer to "primary" and "secondary" server rather than just server, as the distinction is important for these cmdlets.

cormacpayne · 2017-04-26T18:38:51Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

-        [Obsolete("This parameter will be deprecated in the next release.")]
-        public int GracePeriodWithDataLossHour { get; set; }
+        [ValidateRange(0, int.MaxValue)]
+        public int GracePeriodWithDataLossHours { get; set; }


@dlemMSFT @akromm if we're renaming this cmdlet, what do you guys think about renaming it to GracePeriodWithDataLossInHours?

I say this because I know StaticAnalysis is going to throw an exception because parameter nouns should be singular (but nouns such as InHours, InMinutes, etc. are suppressed and won't trigger an exception). Also, I think InHours sounds better with the rest of the parameter noun than Hours. Let me know what you think.

cormacpayne · 2017-04-26T18:44:20Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/NewAzureSqlFailoverGroup.cs

        /// </summary>
-        [Parameter(Mandatory = false,
-            HelpMessage = "The failover policy for read only endpoint of the failover group.")]
+        [Parameter(Mandatory = false, DontShow = true,


@dlemMSFT why are you hiding this parameter? You renamed it above, so there's no need to re-add it and obsolete it.

We thought this would be less disruptive to customers. Unlike the other removed parameters, we expect that customers will actually be using this one. We obsoleted it during the previous release, but we provided no alternative. We'd like to remove it after customers have had a chance to move to the alternative (in the next breaking release).

Would you suggest that we remove immediately?

cormacpayne · 2017-04-26T18:50:23Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/SetAzureSqlFailoverGroup.cs

+            HelpMessage = "The grace period during outage before automatic failover with data loss of the Failover Group is triggered.")]
+        [ValidateNotNullOrEmpty]
+        [ValidateRange(0, int.MaxValue)]
+        public int GracePeriodWithDataLossHours { get; set; }


@dlemMSFT @akromm same comment about InHours

cormacpayne · 2017-04-26T18:50:40Z

src/ResourceManager/Sql/Commands.Sql/Failover Group/Cmdlet/SetAzureSqlFailoverGroup.cs

+        /// <summary>
+        /// Gets or sets the grace period with data loss for the Sql Azure Failover Group.
+        /// </summary>
+        [Parameter(Mandatory = false, DontShow = true,


@dlemMSFT same comment about DontShow

Adam is on vacation and won't be able to approve.

…over Group cmdlets.

Talked with Cormac offline this PR looks OK

markcowl · 2017-05-02T06:26:10Z

tools/StaticAnalysis/Exceptions/SignatureIssues.csv

 "Microsoft.Azure.Commands.Sql.dll","Microsoft.Azure.Commands.Sql.FailoverGroup.Cmdlet.SetAzureSqlFailoverGroup","Set-AzureRmSqlDatabaseFailoverGroup","1","8100","Set-AzureRmSqlDatabaseFailoverGroup Does not support ShouldProcess but the cmdlet verb Set indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue"
+"Microsoft.Azure.Commands.Sql.dll","Microsoft.Azure.Commands.Sql.FailoverGroup.Cmdlet.SetAzureSqlFailoverGroup","Set-AzureRmSqlDatabaseFailoverGroup","2","8210","Set-AzureRmSqlDatabaseFailoverGroup does not have a Force parameter but the cmdlet verb 'Set' indicates that it may perform destructive actions under certain circumstances. Consider whether the cmdlet should have a Force parameter anduse ShouldContinue under some circumstances. ","Consider wether the cmdlet should have a Force parameter and use ShouldContinue under some circumstances. "


Note that you do not need to add suppressions for severity 2 issues

markcowl · 2017-05-02T06:28:07Z

On demand run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-sign/1005/

dlemMSFT requested review from akromm-zz, jaredmoo and ziwa-msft April 26, 2017 00:58

msftclas added the cla-required label Apr 26, 2017

azuresdkci assigned cormacpayne Apr 26, 2017

akromm-zz previously requested changes Apr 26, 2017

View reviewed changes

cormacpayne added cla-not-required and removed cla-required labels Apr 26, 2017

jaredmoo suggested changes Apr 26, 2017

View reviewed changes

cormacpayne previously requested changes Apr 26, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Apr 26, 2017

cormacpayne assigned dlemMSFT and unassigned cormacpayne Apr 26, 2017

jaredmoo mentioned this pull request Apr 27, 2017

Improved tracing for sql tests. #3841

Merged

13 tasks

dlemMSFT added 11 commits April 30, 2017 19:41

Update SQL LegacySdk

6bc50e0

Remove obsolete params, update and fix Failover Group cmdlets

ff06583

Warn and truncate when grace period is outside allowed range

d75ddc3

Add and record tests for SQL Failover Group commands

c6573c5

Change formatting of Failover Group to hide obsolete properties

6922882

Update SQL Failover Group documentation

d146ae2

Update ChangeLog and current-breaking-changes

46ea477

Address pull request comments

4c4ba50

Update Sql Failover Group documentation

6271d6c

Copy Sql Failover Group test recordings to output directory

722d5a5

Suppress some Failover Group signature issues

e0fd4e4

dlemMSFT force-pushed the preview branch from 544a5ed to e0fd4e4 Compare May 1, 2017 05:34

jaredmoo previously approved these changes May 1, 2017

View reviewed changes

Merge branch 'preview' into preview

a9ee682

Remove DontShow attributes as they are not supported by PowerShell 3.0.

cdbba9d

dlemMSFT dismissed jaredmoo’s stale review via cdbba9d May 1, 2017 22:16

Remove obsolete parameter 'GracePeriodWithDataLossHour' from Sql Fail…

8ed3cc5

…over Group cmdlets.

markcowl reviewed May 2, 2017

View reviewed changes

markcowl approved these changes May 2, 2017

View reviewed changes

markcowl added Waiting for On Demand and removed needs-revision labels May 2, 2017

markcowl merged commit 3b22284 into Azure:preview May 2, 2017

azuresdkci added the needs-review label May 2, 2017

		@@ -111,9 +122,7 @@ protected override IEnumerable<AzureSqlFailoverGroupModel> GetEntity()
		}

		// The database already exists

		"Microsoft.Azure.Commands.Sql.dll","Microsoft.Azure.Commands.Sql.FailoverGroup.Cmdlet.SetAzureSqlFailoverGroup","Set-AzureRmSqlDatabaseFailoverGroup","1","8100","Set-AzureRmSqlDatabaseFailoverGroup Does not support ShouldProcess but the cmdlet verb Set indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue"
		"Microsoft.Azure.Commands.Sql.dll","Microsoft.Azure.Commands.Sql.FailoverGroup.Cmdlet.SetAzureSqlFailoverGroup","Set-AzureRmSqlDatabaseFailoverGroup","2","8210","Set-AzureRmSqlDatabaseFailoverGroup does not have a Force parameter but the cmdlet verb 'Set' indicates that it may perform destructive actions under certain circumstances. Consider whether the cmdlet should have a Force parameter anduse ShouldContinue under some circumstances. ","Consider wether the cmdlet should have a Force parameter and use ShouldContinue under some circumstances. "

Fix, update, and add tests for ResourceManager SQL Failover Group cmdlets #3826

Fix, update, and add tests for ResourceManager SQL Failover Group cmdlets #3826

Uh oh!

Conversation

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

Uh oh!

Description

Uh oh!

msftclas commented Apr 26, 2017

Uh oh!

azuresdkci commented Apr 26, 2017

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!

dlemMSFT Apr 26, 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!

cormacpayne commented Apr 26, 2017

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!

dlemMSFT commented Apr 26, 2017 •

edited

Loading

dlemMSFT Apr 26, 2017 •

edited

Loading

markcowl commented May 2, 2017 •

edited

Loading