KeyVault improvements #5654

maddieclayton · 2018-03-02T01:48:35Z

Description

#5496

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

… into KeyVault1

maddieclayton · 2018-03-02T22:36:47Z

on demand run here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/544/

dragav

Many thanks for the thorough refreshing of the KV cmdlets. :-)

dragav · 2018-03-09T19:13:30Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/AddAzureKeyVaultCertificateContact.cs

                   HelpMessage = "Specifies the email address of the contact.")]
        [ValidateNotNullOrEmpty]
-        public string EmailAddress { get; set; }
+        public string[] EmailAddress { get; set; }


I assume this is marked as a breaking change.

@dragav For PowerShell, this is not a breaking change - this parameter will bind to a string or a list of strings.

dragav · 2018-03-09T19:16:09Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/AddAzureKeyVaultCertificateContact.cs

+                VaultName = parsedResourceId.ResourceName;
+            }
+
+            foreach (String email in EmailAddress)


This seems inefficient; we shouldn't have to make an outbound call to the service to get the current contacts for each email address. Furthermore, the cmdlet should validate the input list of emails does not contain duplicates before making an (expensive) service call. I would suggest adding the emails to a hashset, then merging the hashset into the existing contact list, skipping pre-existing, duplicate contacts.

Good catch - does this fix look better?

dragav · 2018-03-09T19:48:23Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVault.cs

        /// Vault name
        /// </summary>
-        [Parameter(Mandatory = true,
+        [Parameter(Mandatory = false,


I'm trying to understand the behavior of the cmdlet after this change. We should be able to:

retrieve a vault by name (alone) -> name is required

list the vaults in a resource group -> name is optional, rg name is optional but indicates a scoped search

list the vaults in a subscription -> no parameters required

Does your change guarantee this behavior?

Yes. By making VaultName option in the GetVault parameter set, I have enable the user to pass in just the resourcegroupname using this parameter. Thus, the parameter set with just resourcegroupname set as mandatory is no longer needed. Check out the help file to see the change a little more clearly if you still have questions.

dragav · 2018-03-09T19:50:27Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultCertificate.cs

+
+        private const string InputObjectByVaultNameParameterSet = "ByNameInputObject";
+        private const string InputObjectByCertificateNameandVersionParameterSet = "ByCertificateNameAndVersionInputObject";
+        private const string InputObjectByCertificateVersionsParameterSet = "ByCertificateAllVersionsInputObject";


What is the scenario that gets a certificate given an input of a) list of certificate versions or b) a certificate itself?

There was never a scenario where the cmdlet accepted a list of versions, only a single version. Is this a scenario we want to enable? Additionally, why would we want to pass in a certificate itself to a get cmdlet? If we have the certificate, it seems unlikely we'd want to "get" it again. All I have done here is remodel the parameter sets so that we could eliminate one unnecessary extra parameter set (see the help file for the new syntax), and add an "inputobject" parameter set for each existing parameter set that would accept a vault object instead of a vault name.

I wasn't suggesting any new scenario; it seemed odd to me to have an "cert all versions input object" parameter set - it implied that "cert all versions" can be the input. Thanks for clarifying.

dragav · 2018-03-09T19:51:46Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultCertificate.cs

+                   ParameterSetName = InputObjectByCertificateNameandVersionParameterSet,
+                   Position = 0,
+                   ValueFromPipeline = true,
+                   HelpMessage = "KeyVault object.")]


I think this is confusing. The only supported input object should be a vault identifier or a vault. Accepting any other object merely to extract the vault name is misleading as to what the cmdlet returns. Did I misunderstand the effect of this change?

This inputobject accepts a vault, and extracts the needed information from that vault object. Does that answer your question?

Yes, thank you for the clarification.

dragav · 2018-03-09T20:32:05Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RemoveAzureKeyVaultCertificate.cs

        /// </summary>
-        [Parameter( Mandatory = false,
-                    HelpMessage = "Permanently remove the previously deleted certificate." )]
+        [Parameter(Mandatory = false,


(nitpick) I've seen that elsewhere in this changeset the InRemovedState parameter was marked as mandatory for the 'deleted' parameter set. Here it seems to be missing. Would you consider adding the 'deleted' parameter set to this cmdlet as well, and maintain consistency with other cmdlets?

So here, -InRemovedState is optional because the user can use the parameter set to either delete an existing certificate (if -inremovedstate is not specified) or to purge a deleted certificate (if -inremovedstate is specified). I could split this into two parameter sets, but there is not reason to as both deleting and purging certificates take the same parameters. Does that make sense? Remove-Key and Remove-Secret are also formatted like this, and were in the past.

dragav · 2018-03-09T20:33:46Z

...ResourceManager/KeyVault/Commands.KeyVault/Commands/RemoveAzureKeyVaultCertificateContact.cs

+        public override void ExecuteCmdlet()
        {
-            if (ShouldProcess(EmailAddress, Properties.Resources.RemoveCertificateContact))
+            foreach (string email in EmailAddress)


Same observation as in the matching AddCertContact cmdlet - consider storing the input email addresses in a hashset, making a single call to the service to retrieve the contacts, and then updating the contacts in a single pass.

Good call, fixed.

dragav · 2018-03-09T20:37:44Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RestoreAzureKeyVaultKey.cs

                   HelpMessage = "Input file. The input file containing the backed-up blob")]
        [ValidateNotNullOrEmpty]
-        public string InputFile { get; set; }
+        public string[] InputFile { get; set; }


Why an array? Is there a scenario that requires the batching of multiple Restore cmdlets in a single one? I get the convenience factor, but I'm concerned that error scenarios will lead to confusion: what was restored, which one failed and why, and do we continue on error or stop at the first one?

Okay, if you believe this scenario will cause more confusion then convenience then I will remove - I simply was adding arrays where I thought it might be useful to customers, but you have a better idea of where it is actually useful.

dragav · 2018-03-09T20:40:54Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultAccessPolicy.cs

+            ParameterSetName = InputObjectByEmailAddress,
+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Specifies certificate operation permissions to grant to a user or service principal.")]
        [ValidateSet("get", "list", "delete", "create", "import", "update", "managecontacts", "getissuers", "listissuers", "setissuers", "deleteissuers", "manageissuers", "recover", "purge", "all")]


As an aside, we wanted to deprecate 'all' as a permission for some time. Since this is a deprecating change, we should just remove it with this commit. If you'd rather not, that's perfectly fine - we'll follow up. Thanks.

This is not a breaking change, but we should deprecate anything we would like to remove at the next breaking change release

I will add this to the list of breaking change and take care of it with the breaking change release, and the deprecation with the next PR where I will add deprecation to other upcoming breaking changes. Do you want "all" removed from each validateset, or only from "PermissionsToCertificates"?

All occurrences of "All" as a permission should be removed. Thanks.

Added to list of breaking changes - will be included in my next PR.

dragav · 2018-03-09T20:44:46Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultCertificatePolicy.cs

+        [Parameter(Mandatory = true,
+            ValueFromPipelineByPropertyName = true,
+            ParameterSetName = ExpandedRenewNumberParameterSet,
+            HelpMessage = "Specifies the number of days after which the automatic process for the certificate renewal begins.")]


"the number of days before expiration when automatic renewal should start".

maddieclayton · 2018-03-12T17:51:40Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/AddAzureKeyVaultCertificateContact.cs

+                VaultName = parsedResourceId.ResourceName;
+            }
+
+            foreach (String email in EmailAddress)


Good catch - does this fix look better?

maddieclayton · 2018-03-12T17:54:55Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVault.cs

        /// Vault name
        /// </summary>
-        [Parameter(Mandatory = true,
+        [Parameter(Mandatory = false,


Yes. By making VaultName option in the GetVault parameter set, I have enable the user to pass in just the resourcegroupname using this parameter. Thus, the parameter set with just resourcegroupname set as mandatory is no longer needed. Check out the help file to see the change a little more clearly if you still have questions.

maddieclayton · 2018-03-12T17:56:05Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultCertificate.cs

+                   ParameterSetName = InputObjectByCertificateNameandVersionParameterSet,
+                   Position = 0,
+                   ValueFromPipeline = true,
+                   HelpMessage = "KeyVault object.")]


This inputobject accepts a vault, and extracts the needed information from that vault object. Does that answer your question?

maddieclayton · 2018-03-12T18:02:33Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultCertificate.cs

+
+        private const string InputObjectByVaultNameParameterSet = "ByNameInputObject";
+        private const string InputObjectByCertificateNameandVersionParameterSet = "ByCertificateNameAndVersionInputObject";
+        private const string InputObjectByCertificateVersionsParameterSet = "ByCertificateAllVersionsInputObject";


There was never a scenario where the cmdlet accepted a list of versions, only a single version. Is this a scenario we want to enable? Additionally, why would we want to pass in a certificate itself to a get cmdlet? If we have the certificate, it seems unlikely we'd want to "get" it again. All I have done here is remodel the parameter sets so that we could eliminate one unnecessary extra parameter set (see the help file for the new syntax), and add an "inputobject" parameter set for each existing parameter set that would accept a vault object instead of a vault name.

maddieclayton · 2018-03-12T18:04:46Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultCertificate.cs

-                    break;
-
-                case ByVaultNameParameterSet:
+            if (!string.IsNullOrEmpty(Version))


If you go through each of these if statements, each one covers one case that you list above. I had to change to this structure because of the additional "inputobject" parameter sets.

maddieclayton · 2018-03-12T18:21:14Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RemoveAzureKeyVaultCertificate.cs

        /// </summary>
-        [Parameter( Mandatory = false,
-                    HelpMessage = "Permanently remove the previously deleted certificate." )]
+        [Parameter(Mandatory = false,


So here, -InRemovedState is optional because the user can use the parameter set to either delete an existing certificate (if -inremovedstate is not specified) or to purge a deleted certificate (if -inremovedstate is specified). I could split this into two parameter sets, but there is not reason to as both deleting and purging certificates take the same parameters. Does that make sense? Remove-Key and Remove-Secret are also formatted like this, and were in the past.

maddieclayton · 2018-03-12T18:31:33Z

...ResourceManager/KeyVault/Commands.KeyVault/Commands/RemoveAzureKeyVaultCertificateContact.cs

+        public override void ExecuteCmdlet()
        {
-            if (ShouldProcess(EmailAddress, Properties.Resources.RemoveCertificateContact))
+            foreach (string email in EmailAddress)


Good call, fixed.

maddieclayton · 2018-03-12T18:35:05Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RestoreAzureKeyVaultKey.cs

                   HelpMessage = "Input file. The input file containing the backed-up blob")]
        [ValidateNotNullOrEmpty]
-        public string InputFile { get; set; }
+        public string[] InputFile { get; set; }


Okay, if you believe this scenario will cause more confusion then convenience then I will remove - I simply was adding arrays where I thought it might be useful to customers, but you have a better idea of where it is actually useful.

maddieclayton · 2018-03-12T18:37:54Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RestoreAzureKeyVaultSecret.cs

                   HelpMessage = "Input file. The input file containing the backed-up blob" )]
        [ValidateNotNullOrEmpty]
-        public string InputFile { get; set; }
+        public string[] InputFile { get; set; }


Removed the array here as well - let me know if you think it should be added back here.

maddieclayton · 2018-03-12T18:39:33Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultAccessPolicy.cs

+            ParameterSetName = InputObjectByEmailAddress,
+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Specifies certificate operation permissions to grant to a user or service principal.")]
        [ValidateSet("get", "list", "delete", "create", "import", "update", "managecontacts", "getissuers", "listissuers", "setissuers", "deleteissuers", "manageissuers", "recover", "purge", "all")]


I will add this to the list of breaking change and take care of it with the breaking change release, and the deprecation with the next PR where I will add deprecation to other upcoming breaking changes. Do you want "all" removed from each validateset, or only from "PermissionsToCertificates"?

cormacpayne · 2018-03-20T23:50:40Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultKey.cs

        /// Key version.
        /// </summary>
-        [Parameter(Mandatory = false,
+        [Parameter(Mandatory = true,


@maddieclayton do you know if this parameter set worked without providing the Version parameter before?

Same q. Are we breaking any existing scenarios?

If you look at the help file, I added an optional "Name" parameter to the first parameter set, which will take care of the case where -Version is not provided to this parameter set. This leaves all scenarios working, and I believed that the parameter sets made more sense.

cormacpayne · 2018-03-20T23:51:24Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultSecret.cs

        /// Secret version
        /// </summary>
-        [Parameter(Mandatory = false,
+        [Parameter(Mandatory = true,


@maddieclayton do you know if this parameter set worked before without providing the Version parameter?

Same comment here. I added an optional "Name" parameter to the first parameter set, which will take care of the case where -Version is not provided to this parameter set. This leaves all scenarios working, and I believed that the parameter sets made more sense.

cormacpayne · 2018-03-20T23:53:41Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultCertificatePolicy.cs

+        /// </summary>
+        [Parameter(Mandatory = true,
+            Position = 2,
+            ValueFromPipelineByPropertyName = true,


@maddieclayton should this be ValueFromPipeline?

Good call, thanks for the catch.

cormacpayne · 2018-03-20T23:54:36Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/RemoveAzureKeyVault.cs

        /// If present, operate on the deleted vault entity.
        /// </summary>
-        [Parameter(Mandatory = false,
+        [Parameter(Mandatory = true,


@maddieclayton do we know if this parameter set worked previously if the user did not provide InRemovedState?

If they provided all the parameters except -InRemovedState, it would default to the first parameter set, because the first is the default and contains all the parameters in the second.

cormacpayne · 2018-03-21T00:16:25Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/ImportAzureKeyVaultCertificate.cs

+                   ValueFromPipeline = true,
                   ParameterSetName = ImportWithPrivateKeyFromCollectionParameterSet,
                   HelpMessage = "Specifies the certificate collection to add to key vault.")]
+        [Alias("InputObject")]


@maddieclayton any reason we don't want to call this parameter InputObject with an alias to CertificateCollection like we do for all other cmdlets?

Deleting alias as it doesn't really make sense in this context.

cormacpayne · 2018-03-21T00:19:59Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/SetAzureKeyVaultCertificateIssuer.cs

                   HelpMessage = "Specifies the certificate issuer to set.")]
        [ValidateNotNullOrEmpty]
-        public KeyVaultCertificateIssuer Issuer { get; set; }
+        public PSKeyVaultCertificateIssuerIdentityItem Issuer { get; set; }


@maddieclayton do we want to name this InputObject with an alias to Issuer?

cormacpayne · 2018-03-21T00:28:14Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureKeyVaultCertificateAttribute.md

@@ -1,4 +1,4 @@
---


@maddieclayton it looks like the underlying call for this cmdlet is a PATCH, so we should look at renaming this cmdlet to Update-AzureKeyVaultCertificateAttribute with an alias to Set-*

Please don't do this. There is no separate command for adding or importing a secret - Set fits the intended profile here.

I'm going to skip doing these aliases for now and we will discuss again for the breaking change PR - I think some of these aliases might make sense while others do not.

cormacpayne · 2018-03-21T00:30:22Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureKeyVaultCertificatePolicy.md

@@ -1,4 +1,4 @@
---


@maddieclayton same comment about this call being a PATCH, so we should change the verb to Update

Same comment as above - there is no Add-Policy, and we don't want one. A Set creates the policy if missing, and overwrites it if existing.

cormacpayne · 2018-03-21T00:31:54Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureKeyVaultKeyAttribute.md

@@ -1,4 +1,4 @@
---


@maddieclayton same comment about this call being a PATCH, so we should change the verb to Update

cormacpayne · 2018-03-21T00:34:47Z

src/ResourceManager/KeyVault/Commands.KeyVault/help/Set-AzureKeyVaultSecretAttribute.md

@@ -1,4 +1,4 @@
---


@maddieclayton same comment about this call being a PATCH, so we should change the verb to Update

Going to rename to Update-AzureKeyVaultSecret as I will combine this with Set-AzureKeyVaultSecret at the breaking change release and I don't want to aliases to get out of control.

markcowl

a couple of questions and potential changes

markcowl · 2018-03-21T09:32:32Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultCertificateIssuer.cs

+        /// </summary>
+        [Parameter(Mandatory = false,
            Position = 1,
            ValueFromPipelineByPropertyName = true,


We should be removing these valuefrompipelinebypropertyname for the interactive parameter sets, right? I assume this is happening in the breaking change update.

Yes, this will be happening for the breaking change update.

markcowl · 2018-03-21T09:33:55Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/GetAzureKeyVaultKey.cs

        /// Key version.
        /// </summary>
-        [Parameter(Mandatory = false,
+        [Parameter(Mandatory = true,


Same q. Are we breaking any existing scenarios?

markcowl · 2018-03-21T09:37:15Z

src/ResourceManager/KeyVault/Commands.KeyVault/Commands/NewAzureKeyVaultCertificatePolicy.cs

+                   ParameterSetName = DNSNamesParameterSet,
+                   ValueFromPipelineByPropertyName = true,
+                   HelpMessage = "Specifies the DNS names in the certificate.")]
+        public List<string> DnsNames { get; set; }


should rename as DnsName, with alias to old name DnsNames (and deprecate)

Why? This parameter does take a list..

@dragav it comes from the Strongly Encouraged Developer Guidelines for PowerShell cmdlets:

Avoid using plural names for parameters whose value is a single element. This includes parameters that take arrays or lists because the user might supply an array or list with only one element.

I disagree. We don't have a parameter called "PermissionToSecrets" - plural is appropriate here as it conveys the fact that the certificate accepts an enumeration of DNS names. Having a suggestively named parameter outweighs a convention based solely on the parameter's type.

@dragav I agree that this convention is silly, but it is one that we enforce for new changes across all modules and one that has been brought up by users numerous times (e.g., this issue).

Going back to the guidelines mentioned above, as long as the user can provide a single object to the list/array parameter, than the parameter noun should be singular. If, however, you can do extra validation to ensure that the user always provides more than one object, then having a plural noun is allowed:

Plural parameter names should be used only in those cases where the value of the parameter is always a multiple-element value. In these cases, the cmdlet should verify that multiple elements are supplied, and the cmdlet should display a warning to the user if multiple elements are not supplied.

maddieclayton · 2018-03-21T19:49:07Z

on demand here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/649/

Addressed.

azdevxps and others added 11 commits February 21, 2018 11:43

initial changes

d416e40

more changes

a5ad7f8

more changes

150e445

more changes

c25f805

more changes

7afcb04

add help files

6c3c37d

fix output types

8b662fb

fix new policy

785b964

update changelog

7f0735d

Merge branch 'preview' into KeyVault1

6f3efa6

Update New-AzureRmKeyVault.md

86bdc9e

azuresdkci added the needs-review label Mar 2, 2018

maddieclayton added 3 commits March 2, 2018 11:00

update breakingchanges.csv

d6b7cd7

Merge branch 'KeyVault1' of github.com:maddieclayton/azure-powershell…

32fbef3

… into KeyVault1

Update BreakingChangeIssues.csv

5e71f44

maddieclayton assigned markcowl Mar 2, 2018

maddieclayton requested a review from rohmano March 5, 2018 18:33

maddieclayton assigned RandalliLama Mar 5, 2018

dragav suggested changes Mar 9, 2018

View reviewed changes

cormacpayne assigned maddieclayton and unassigned markcowl and RandalliLama Mar 12, 2018

cormacpayne added needs-revision and removed needs-review labels Mar 12, 2018

maddieclayton commented Mar 12, 2018

View reviewed changes

maddieclayton assigned dragav and unassigned maddieclayton Mar 12, 2018

maddieclayton added needs-review and removed needs-revision labels Mar 12, 2018

maddieclayton and others added 4 commits March 19, 2018 17:04

fix piping

2319a02

Merge branch 'preview' into KeyVault1

08f4333

Update GetAzureKeyVaultCertificateIssuer.cs

a1a3b3b

Merge branch 'preview' into KeyVault1

3eef5be

cormacpayne suggested changes Mar 21, 2018

View reviewed changes

markcowl previously requested changes Mar 21, 2018

View reviewed changes

markcowl added needs-revision and removed needs-review labels Mar 21, 2018

markcowl assigned maddieclayton and unassigned markcowl Mar 21, 2018

cormacpayne changed the base branch from preview to release-2018-03-23 March 21, 2018 15:45

address comments

d508cdf

maddieclayton assigned cormacpayne and unassigned maddieclayton Mar 21, 2018

maddieclayton added needs-review and removed needs-revision labels Mar 21, 2018

cormacpayne previously approved these changes Mar 21, 2018

View reviewed changes

cormacpayne assigned markcowl and unassigned cormacpayne Mar 21, 2018

update valuefrompipelinebyproperty

aa6fa32

maddieclayton dismissed cormacpayne’s stale review via aa6fa32 March 21, 2018 19:32

maddieclayton assigned cormacpayne and unassigned markcowl Mar 21, 2018

maddieclayton changed the base branch from release-2018-03-23 to preview March 22, 2018 00:00

maddieclayton assigned markcowl Mar 22, 2018

cormacpayne approved these changes Mar 22, 2018

View reviewed changes

maddieclayton merged commit 0b76130 into Azure:preview Mar 23, 2018

maddieclayton deleted the KeyVault1 branch March 23, 2018 23:50

KeyVault improvements #5654

KeyVault improvements #5654

Uh oh!

Conversation

maddieclayton commented Mar 2, 2018

Description

Checklist

Uh oh!

maddieclayton commented Mar 2, 2018

Uh oh!

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

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!

Choose a reason for hiding this comment