Create azure App Service Plan resource #1

lstolyarov · 2017-06-11T13:06:08Z

No description provided.

lstolyarov · 2017-06-11T13:34:39Z

@tombuildsstuff I have migrated the code in pull requests hashicorp/terraform#12001 and hashicorp/terraform#13634 to this repository.

tombuildsstuff

Hi @lstolyarov

Thanks for this contribution / porting this over to the new repo - I've reviewed and left some comments in-line - but this is off to a good start :)

Thanks!

tombuildsstuff · 2017-06-24T09:33:03Z

azurerm/resource_arm_app_service_plan.go

+	client := meta.(*ArmClient)
+	AppServicePlanClient := client.appServicePlansClient
+
+	log.Printf("[INFO] preparing arguments for Azure App Service Plan creation.")


can we update this to AzureRM for consistency?

tombuildsstuff · 2017-06-24T09:33:19Z

azurerm/resource_arm_app_service_plan.go

+				Type:     schema.TypeString,
+				Required: true,
+			},
+			"location": {


can we switch this to use locationSchema()?

tombuildsstuff · 2017-06-24T09:34:55Z

azurerm/resource_arm_app_service_plan.go

+
+func resourceArmAppServicePlanCreateUpdate(d *schema.ResourceData, meta interface{}) error {
+	client := meta.(*ArmClient)
+	AppServicePlanClient := client.appServicePlansClient


minor: we'd generally combine these into one variable client

tombuildsstuff · 2017-06-24T09:38:22Z

azurerm/resource_arm_app_service_plan.go

+		Create: resourceArmAppServicePlanCreateUpdate,
+		Read:   resourceArmAppServicePlanRead,
+		Update: resourceArmAppServicePlanCreateUpdate,
+		Delete: resourceArmAppServicePlanDelete,


given there's a test for it, can we add import support here:

Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, },

tombuildsstuff · 2017-06-24T09:38:36Z

azurerm/resource_arm_app_service_plan.go

+			},
+			"maximum_number_of_workers": {
+				Type:     schema.TypeString,
+				Optional: true,


should this also be Computed?

tombuildsstuff · 2017-06-24T09:41:36Z

azurerm/resource_arm_app_service_plan.go

+	}
+
+	d.Set("name", name)
+	d.Set("resource_group_name", resGroup)


can we set the other field values here?

tombuildsstuff · 2017-06-24T09:43:57Z

azurerm/resource_arm_app_service_plan_test.go

+	}
+}
+
+var testAccAzureRMAppServicePlan_standard = `


for newer resources we're tending to make this a function which returns a formatted string:

func testAccAzureRMAppServicePlan_standard(rInt int) string { return fmt.Sprintf(`foo`, rInt) }

tombuildsstuff · 2017-06-24T09:44:53Z

azurerm/resource_arm_app_service_plan_test.go

+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestAccAzureRMAppServicePlan_standard(t *testing.T) {


Can we add another test case with all fields set?

tombuildsstuff · 2017-06-24T09:51:49Z

azurerm/resource_arm_app_service_plan.go

+			"tier": {
+				Type:     schema.TypeString,
+				Required: true,
+			},


Looking at the SkuDescription object:

// SkuDescription is description of a SKU for a scalable resource. type SkuDescription struct { Name *string `json:"name,omitempty"` Tier *string `json:"tier,omitempty"` Size *string `json:"size,omitempty"` Family *string `json:"family,omitempty"` Capacity *int32 `json:"capacity,omitempty"` SkuCapacity *SkuCapacity `json:"skuCapacity,omitempty"` Locations *[]string `json:"locations,omitempty"` Capabilities *[]Capability `json:"capabilities,omitempty"` }

I think it makes sense to separate this out into it's own block i.e.:

sku { name = "Standard_S0" tier = "Standard" size = "S0" ... }

Here's an example of the Schema usage - expanding/parsing it out and flattening/writing it back - which I'm hoping makes sense?

tombuildsstuff · 2017-06-24T09:58:40Z

azurerm/resource_arm_app_service_plan.go

+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func resourceArmAppServicePlan() *schema.Resource {


could we add some documentation for this resource too?

lstolyarov · 2017-06-24T17:37:39Z

Hey @tombuildsstuff, I am facing a problem when running the acceptance test for the App Service Plan. I get the following error: Cannot find Web space acctestRG-2152939972729330496-WestEuropewebspace for subscription xxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx. Have you ever seen this problem before?

tombuildsstuff · 2017-06-25T06:46:11Z

Hey @lstolyarov

Taking a look at the request made in Debug mode, I can confirm I'm seeing the same response - which appears to be an Azure API / Service error:

PUT /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-212/providers/Microsoft.Web/serverfarms/acctestAppServicePlan-test?api-version=2016-09-01 HTTP/1.1
Host: management.azure.com
User-Agent: HashiCorp-Terraform-v0.9.8
Content-Length: 91
Content-Type: application/json
Accept-Encoding: gzip

{
	"location": "westeurope",
	"properties": {},
	"sku": {
		"name": "S0",
		"tier": "Standard",
		"size": "S0"
	}
}

Which returns the response:

HTTP/1.1 404 Not Found
Content-Length: 667
...

{
	"Code": "NotFound",
	"Message": "Cannot find Web space acctestRG-212-WestEuropewebspace for subscription 00000000-0000-0000-0000-000000000000.",
	"Target": null,
	"Details": [{
		"Message": "Cannot find Web space acctestRG-212-WestEuropewebspace for subscription 00000000-0000-0000-0000-000000000000."
	}, {
		"Code": "NotFound"
	}, {
		"ErrorEntity": {
			"ExtendedCode": "03005",
			"MessageTemplate": "Cannot find Web space {0} for subscription {1}.",
			"Parameters": ["acctestRG-212-WestEuropewebspace", "00000000-0000-0000-0000-000000000000"],
			"Code": "NotFound",
			"Message": "Cannot find Web space acctestRG-212-WestEuropewebspace for subscription 00000000-0000-0000-0000-000000000000."
		}
	}],
	"Innererror": null
}

I've also tested in an HTTP client submitting via a POST rather than a PUT to check if this allowed is to be created - but unfortunately this doesn't appear to be an option. As such, I believe this is a service error, and the next step would be to open an issue over on the Azure Rest API Specs Repository which would allow the team to investigate further.

Hope that helps? :)

echuvyrov · 2017-07-03T22:22:11Z

Hey @lstolyarov thanks for all your work on this. I'd love to help you resolve the issue so we can move your change along. I did a bit of looking and testing and it looks like you have "S0" as the plan for your tests. Using your code, I changed it to "S1" and the tests almost passed :) (got a different, hopefully familiar error). Here are a couple of links to take a look at:

App Service Plans: https://azure.microsoft.com/en-us/pricing/details/app-service/
Gist creating a resource group + web app plan using Go SDK (portions, including logging borrowed from TF): https://gist.github.com/echuvyrov/cd8f9c2fcbd90a01e7bace2c523bf915

Let me know if this helps or if there's something else going on here.

tombuildsstuff · 2017-07-17T05:40:53Z

👋 hey @lstolyarov - have you had a chance to look into @echuvyrov's comments? :)

lstolyarov · 2017-07-17T12:41:47Z

Hi @tombuildsstuff,

Apologies, I haven't had a chance yet.

@echuvyrov thanks for the suggestion. I am aiming to try it out this week.

Thanks

echuvyrov · 2017-07-18T16:43:26Z

@lstolyarov let me know if you could use a helping hand - I think this is one of the key resources for Azure :)

lstolyarov · 2017-07-18T18:21:54Z

Hey guys,

I just had a look at this and the app service plan does get created with the sku size set to "S1". However, the tests now fail when resources are being destroyed. The destroy command cannot execute because the resource still has a create operation running on it:

{
  "Code": "Conflict",
  "Message": "Cannot modify this web hosting plan because another operation is in progress. Details: Id: 096c1fa1-b248-4e67-98c5-f0e018b1b261, OperationName: Create, CreatedTime: 7/18/2017 5:42:06 PM, RequestId: 09e22889-8fb2-40f0-8446-2739e820f135, EntityType: 9",
  "Target": null,
  "Details": [
    {
      "Message": "Cannot modify this web hosting plan because another operation is in progress. Details: Id: 096c1fa1-b248-4e67-98c5-f0e018b1b261, OperationName: Create, CreatedTime: 7/18/2017 5:42:06 PM, RequestId: 09e22889-8fb2-40f0-8446-2739e820f135, EntityType: 9"
    },
    {
      "Code": "Conflict"
    },
    {
      "ErrorEntity": {
        "ExtendedCode": "59205",
        "MessageTemplate": "Cannot modify this web hosting plan because another operation is in progress. Details: {0}",
        "Parameters": [
          "Id: 096c1fa1-b248-4e67-98c5-f0e018b1b261, OperationName: Create, CreatedTime: 7/18/2017 5:42:06 PM, RequestId: 09e22889-8fb2-40f0-8446-2739e820f135, EntityType: 9"
        ],
        "Code": "Conflict",
        "Message": "Cannot modify this web hosting plan because another operation is in progress. Details: Id: 096c1fa1-b248-4e67-98c5-f0e018b1b261, OperationName: Create, CreatedTime: 7/18/2017 5:42:06 PM, RequestId: 09e22889-8fb2-40f0-8446-2739e820f135, EntityType: 9"
      }
    }
  ],
  "Innererror": null
}

@tombuildsstuff have you come across this before?

tombuildsstuff · 2017-07-19T14:14:53Z

Hey @lstolyarov

From what you've said above, I'd guess the SDK is returning from the CreateOrUpdate method when the resource creation has been submitted/requested, rather than polling for it's completion and returning once it's finished creating. In general I'd put that down to a Swagger bug, where the status code isn't right - let's ignore that for now, but we should probably open a bug about it on the azure-rest-api-specs repository at some point

To work around that - in some resources we have functions which poll to check the resource has been created - which is what I'd suggest adding to the Create method.

Hopefully that should solve the issue you're seeing here :)

Thanks!

lstolyarov · 2017-07-19T20:05:05Z

Hi @tombuildsstuff,

Thanks for the suggestion. I added the function but it doesn't help. The ProvisioningState is updated to "Succeeded" when the app service is created. I also tested this manually and I am only able to delete the app service plan about 5 minutes after it has been provisioned so there is definitely something else happening. I have got an issue open here updated with the latest problems.

echuvyrov · 2017-07-21T18:43:31Z

hey @lstolyarov @tombuildsstuff I am running into the same issue you are. Looking into this into internally and hope to have an answer very soon.

* added .travis.yml and deploy.sh * added deploy script, updated travis.yml to build topic- branches * generate random string for hostname * plan now produces output plan, apply now consumes outputted plan * cleanup; sane defaults * explicit build dirs

tombuildsstuff · 2017-08-07T09:22:30Z

Hey @echuvyrov

Did you manage to get anywhere with this internally? :)

Thanks!

georgeedwards · 2017-08-09T09:24:17Z

There's been an update on the SDK issue. Are we sure that S0 is a valid tier?

marstr · 2017-08-09T17:07:36Z

azurerm/resource_arm_app_service_plan.go

+		Sku: &sku,
+	}
+
+	_, error := AppServicePlanClient.CreateOrUpdate(resGroup, name, appServicePlan, make(chan struct{}))


You may as well update this to be:

_, error := AppServicePlanClient.CreateOrUpdate(resGroup, name, appServicePlan, nil)

Instantiating a channel like this on the fly doesn't allow you a way to cancel and just adds a little garbage to be cleaned up later. :) Notice in this playground example nil is passed to wait(...) but never triggers the cancel select case.

georgeedwards · 2017-08-15T08:48:24Z

Hey @lstolyarov, Did you get a chance to look at the comments? Let me know if you need a hand - I would be really keen to see this as a supported resource.

echuvyrov · 2017-08-16T00:02:57Z

hey @georgeedwards @lstolyarov @tombuildsstuff we should have this unblocked this week, will keep you posted. I will test at the end of the week or at the beginning of the next week to confirm.

echuvyrov · 2017-08-17T20:28:08Z

hey @lstolyarov @tombuildsstuff @georgeedwards - our team reporting the acceptance tests passing now, can you please confirm that you are unblocked when you get a chance?

lstolyarov · 2017-08-19T14:06:55Z

Hi @echuvyrov, thanks a lot for having a look at this. The tests are now passing:

$ TF_ACC=1 envchain azurecloudops go test ./azurerm -v -timeout 300m -parallel 5 -run TestAccAzureRMAppServicePlan
=== RUN   TestAccAzureRMAppServicePlan_importBasic
--- PASS: TestAccAzureRMAppServicePlan_importBasic (63.80s)
=== RUN   TestAccAzureRMAppServicePlan_importStandard
--- PASS: TestAccAzureRMAppServicePlan_importStandard (77.62s)
=== RUN   TestAccAzureRMAppServicePlan_importPremium
--- PASS: TestAccAzureRMAppServicePlan_importPremium (74.71s)
=== RUN   TestAccAzureRMAppServicePlan_basic
--- PASS: TestAccAzureRMAppServicePlan_basic (57.62s)
=== RUN   TestAccAzureRMAppServicePlan_standard
--- PASS: TestAccAzureRMAppServicePlan_standard (63.03s)
=== RUN   TestAccAzureRMAppServicePlan_premium
--- PASS: TestAccAzureRMAppServicePlan_premium (66.64s)
PASS

@tombuildsstuff I have added a couple more tests and updated the documentation. Is there anything else that needs to be done before this can be merged?

Thanks,
Leo

tombuildsstuff

Hey @lstolyarov

Thanks for the continued effort on this PR :)

I've taken a look through and left some comments, but they're mostly minor - if we can fix those up this otherwise looks good to merge :)

Thanks!

tombuildsstuff · 2017-08-19T17:02:59Z

azurerm/resource_arm_app_service_plan.go

+				Set: resourceAzureRMAppServicePlanSkuHash,
+			},
+			"maximum_number_of_workers": {
+				Type:     schema.TypeString,


I think this should be an int rather than a String?

tombuildsstuff · 2017-08-19T17:03:25Z

azurerm/appserviceplan.go

+
+		return res, string(res.AppServicePlanProperties.ProvisioningState), nil
+	}
+}


minor could we move this into the resource_arm_app_service_plan.go file?

tombuildsstuff · 2017-08-19T17:06:51Z

azurerm/resource_arm_app_service_plan.go

+		Timeout: 10 * time.Minute,
+	}
+	if _, err := stateConf.WaitForState(); err != nil {
+		return fmt.Errorf("Error waiting for App Service Plan (%s) to become available: %s", name, err)


minor can we make this second formatting argument %+v rather than %s to show the full error?

tombuildsstuff · 2017-08-19T17:07:22Z

azurerm/appserviceplan.go

+	return func() (interface{}, string, error) {
+		res, err := client.appServicePlansClient.Get(resourceGroupName, appserviceplan)
+		if err != nil {
+			return nil, "", fmt.Errorf("Error issuing read request in appServicePlanStateRefreshFunc to Azure ARM for App Service Plan '%s' (RG: '%s'): %s", appserviceplan, resourceGroupName, err)


can we make the final formatting argument %+v here to return the full error?

tombuildsstuff · 2017-08-19T17:07:54Z

azurerm/appserviceplan.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func appServicePlanStateRefreshFunc(client *ArmClient, resourceGroupName string, appserviceplan string) resource.StateRefreshFunc {


really minor it'd be good to call appserviceplan -> name for consistency across these methods, if possible

tombuildsstuff · 2017-08-19T17:19:23Z

azurerm/resource_arm_app_service_plan_test.go

+			size = "B1"
+		}
+}
+`, rInt, rInt)


can we add location as the second argument here?

tombuildsstuff · 2017-08-19T17:21:10Z

website/azurerm.erb

@@ -42,6 +42,17 @@
              </ul>
            </li>

+            <li<%= sidebar_current("docs-azurerm-resource-app-service") %>>
+              <a href="#">App Service Resources</a>


🤔 do you think it's worth making this "App Service (Web Apps) Resources", like we've done for CosmosDB/DocumentDB?

Good point. Yes, I think it is worth doing that.

tombuildsstuff · 2017-08-19T17:21:18Z

website/azurerm.erb

+              <ul class="nav nav-visible">
+
+                <li<%= sidebar_current("docs-azurerm-resource-app-service-plan") %>>
+                  <a href="/docs/providers/azurerm/r/app_service_pla.html">azurerm_app_service_plan</a>


app_service_pla.html -> app_service_plan.html

tombuildsstuff · 2017-08-19T17:24:33Z

website/docs/r/app_service_plan.html.markdown

+* `tier` - (Required) Specified the plan's pricing tier.
+* `size` - (Required) Specified the plan's instance size. 
+
+* `maximum_number_of_workers` - (Optional) .


could we add a description for this field, here's the description from the Azure docs: "Maximum number of instances that can be assigned to this App Service plan."

tombuildsstuff · 2017-08-19T17:30:53Z

azurerm/resource_arm_app_service_plan_test.go

+
+resource "azurerm_app_service_plan" "test" {
+    name                = "acctestASP-%d"
+    location            = "West Europe"


can we make this ${azurerm_resource_group.test.location} to only insert the location formatting argument a single time?

georgeedwards · 2017-09-12T19:45:02Z

@lstolyarov Are ther any plans to support adding env variables to the configuration?

Merging from upstream

Merge base

catch up with main fork

Fixes for `azurerm_policy_definition`

pull

update

Syncing from TF

merge from origin

Add test and documentation

add _id to fields for clarity

Sync master from Terraform head

pull upstream

Merge latest version

Update fork

q

Updated to include TRIAL as a SKU

Add network interface and security group association

ghost · 2020-04-01T15:21:49Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

lstolyarov mentioned this pull request Jun 21, 2017

provider/azurerm: Create azure App Service Plan resource hashicorp/terraform#13634

Closed

tombuildsstuff added enhancement new-resource labels Jun 24, 2017

tombuildsstuff previously requested changes Jun 24, 2017

View reviewed changes

tombuildsstuff mentioned this pull request Jun 26, 2017

Feature Request: Azure Functions #131

Closed

lstolyarov mentioned this pull request Jun 26, 2017

Cannot find Web space when creating app service plan Azure/azure-sdk-for-go#667

Closed

tombuildsstuff mentioned this pull request Jun 27, 2017

Azure web app deployment #35

Closed

tombuildsstuff added the upstream label Jul 14, 2017

tombuildsstuff added the waiting-response label Jul 17, 2017

marstr reviewed Aug 9, 2017

View reviewed changes

tombuildsstuff reviewed Aug 19, 2017

View reviewed changes

tombuildsstuff removed the upstream label Aug 19, 2017

maniSbindra mentioned this pull request Aug 28, 2017

sync from upstream to resolve merge conflict in PR1 lstolyarov/terraform-provider-azurerm#3

Closed

tombuildsstuff pushed a commit that referenced this pull request Sep 14, 2017

Merge pull request #1 from terraform-providers/master

2377474

Merging from upstream

tombuildsstuff pushed a commit that referenced this pull request Oct 12, 2017

Merge pull request #1 from terraform-providers/master

1d5a475

Merge base

tombuildsstuff pushed a commit that referenced this pull request Oct 12, 2017

Merge pull request #1 from terraform-providers/master

0c3eb31

catch up with main fork

metacpp mentioned this pull request Nov 29, 2017

Updated the way user agent string gets assigned. #587

Merged

tombuildsstuff pushed a commit that referenced this pull request Mar 27, 2018

Merge pull request #1 from terraform-providers/policy-definition

06a5842

Fixes for `azurerm_policy_definition`

metacpp mentioned this pull request Mar 29, 2018

How to specify a custom VHD to azurerm_virtual_machine resource? #1042

Closed

siddthota mentioned this pull request May 31, 2018

Issue with spinning-up SQL Server 2012 running on Windows Server 2012 R2 Datacenter VM? #1325

Closed

katbyte pushed a commit that referenced this pull request Jun 14, 2018

Merge pull request #1 from terraform-providers/master

8f6b05f

pull

katbyte pushed a commit that referenced this pull request Jun 28, 2018

Merge pull request #1 from terraform-providers/master

444236a

update

katbyte pushed a commit that referenced this pull request Aug 14, 2018

Merge pull request #1 from terraform-providers/master

e5e75f2

Syncing from TF

katbyte pushed a commit that referenced this pull request Sep 19, 2018

Merge pull request #1 from terraform-providers/master

5dab9e0

merge from origin

katbyte pushed a commit that referenced this pull request Oct 26, 2018

Merge pull request #1 from julienstroheker/upgrade_policy_rolling

e113177

Add test and documentation

tombuildsstuff pushed a commit that referenced this pull request Jan 3, 2019

Merge pull request #1 from DenheenJ/update-fields

f488e6c

add _id to fields for clarity

tombuildsstuff pushed a commit that referenced this pull request Feb 14, 2019

Merge pull request #1 from terraform-providers/master

7c164f7

Sync master from Terraform head

tombuildsstuff pushed a commit that referenced this pull request May 7, 2019

Merge pull request #1 from terraform-providers/master

9fc54d9

pull upstream

tombuildsstuff pushed a commit that referenced this pull request Sep 3, 2019

Merge pull request #1 from terraform-providers/master

18b2213

Merge latest version

neil-yechenwei mentioned this pull request Sep 16, 2019

Fix TF issue 4151: azurerm_app_service blocks not working as expected #4328

Closed

tombuildsstuff pushed a commit that referenced this pull request Sep 23, 2019

Merge pull request #1 from terraform-providers/master

28e93e4

Update fork

jnunham mentioned this pull request Sep 27, 2019

Error on Terraform Apply: CosmosDB account name already exists #3813

Closed

tombuildsstuff pushed a commit that referenced this pull request Oct 25, 2019

Merge pull request #1 from terraform-providers/master

a4ba7fe

q

ahelal mentioned this pull request Dec 4, 2019

terraform execution in westeurope is slow #5067

Closed

venkey1000 mentioned this pull request Dec 31, 2019

Updating terraform files from version 1.22 to 1.39 with out changing manually in every file #5253

Closed

tombuildsstuff pushed a commit that referenced this pull request Feb 18, 2020

Merge pull request #1 from johnwildes/johnwildes-databricks-sku-update

c3f2e8d

Updated to include TRIAL as a SKU

roshanp85 mentioned this pull request Feb 21, 2020

Using Azure Vnet Peerings across subscriptions - Terraform #5843

Closed

tombuildsstuff pushed a commit that referenced this pull request Feb 25, 2020

Merge pull request #1 from sdowsland/sdowsland-patch-1

a33624b

Add network interface and security group association

wyarde mentioned this pull request Feb 27, 2020

Add tags to cluster node pools #5931

Merged

ghost locked and limited conversation to collaborators Apr 1, 2020

Create azure App Service Plan resource #1

Create azure App Service Plan resource #1

Conversation

lstolyarov commented Jun 11, 2017

lstolyarov commented Jun 11, 2017

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lstolyarov commented Jun 24, 2017

tombuildsstuff commented Jun 25, 2017

echuvyrov commented Jul 3, 2017

tombuildsstuff commented Jul 17, 2017

lstolyarov commented Jul 17, 2017

echuvyrov commented Jul 18, 2017

lstolyarov commented Jul 18, 2017

tombuildsstuff commented Jul 19, 2017

lstolyarov commented Jul 19, 2017

echuvyrov commented Jul 21, 2017

tombuildsstuff commented Aug 7, 2017

georgeedwards commented Aug 9, 2017

Choose a reason for hiding this comment

georgeedwards commented Aug 15, 2017

echuvyrov commented Aug 16, 2017

echuvyrov commented Aug 17, 2017

lstolyarov commented Aug 19, 2017

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeedwards commented Sep 12, 2017

ghost commented Apr 1, 2020