Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function app consumption plan bug #1515

Merged
merged 6 commits into from
Jul 16, 2018
Merged

Function app consumption plan bug #1515

merged 6 commits into from
Jul 16, 2018

Conversation

ranieuwe
Copy link
Contributor

@ranieuwe ranieuwe commented Jul 7, 2018

This PR fixes #1453 by disabling the WEBSITE_CONTENT app setting when a dynamic App service plan is used. All TestAccAzureRMFunctionApp* tests are successful.

It is very hard to write a test for this scenario because the default app settings variables get deleted on resource read and are not transported to the terraform schema. This means it is not possible not to do a TestCheckResourceAttr or similar. Have manually inspected the resources and the settings are applied correctly with these fixes.

As an aside, the function app logic will need refactoring as it is fairly inefficient and not DRY. Create calls update, likely to update the app_settings, but update basically reruns the same ARM call that Create just made, slowing down the progress by approx 20 to 30 seconds.

Because of this double logic you'll see the app service plan needing to be retrieved twice and introduced throughout. There is no cleaner way without refactoring extensively.

{Name: &contentSharePropName, Value: &contentShare},
{Name: &contentFileConnStringPropName, Value: &storageConnection},
}

// If the application plan is dynamic (consumption), we do not want to include WEBSITE_CONTENT components
if strings.EqualFold(appServiceTier, "dynamic") {
Copy link

@APErebus APErebus Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is incorrect. We want the extra app settings if the app service tier is dynamic. We do not want these settings for standard/non-consumption app services.

Just missing an invert on this boolean check 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolute right. D'oh. Thanks for spotting this. Will fix

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @ranieuwe

Thanks for this PR :)

I've taken a look through and aside from the logic bug this mostly LGTM; if we can fix up the potential crash then we should be good to run the tests/get this merged

Thanks!

return "", fmt.Errorf("[ERROR] Could not retrieve App Service Plan ID '%s': %+v", appServicePlanId, err)
}

return *appServicePlan.Sku.Tier, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a crash here if appServicePlan.Sku or appServicePlan.Sku.Tier is nil (which happens for older resources); can we make this:

if sku := appServicePlan.Sku; sku != nil {
 if tier := sku.Tier; tier != nil {
    return *tier, nil
  }
}
return fmt.Errorf("No `sku` block was returned for App Service Plan %q (Resource Group %q)", resourceGroup, name)

appServicePlansClient := meta.(*ArmClient).appServicePlansClient
appServicePlan, err := appServicePlansClient.Get(ctx, id.ResourceGroup, id.Path["serverfarms"])
if err != nil {
return "", fmt.Errorf("[ERROR] Could not retrieve App Service Plan ID '%s': %+v", appServicePlanId, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor these two '%s' can become %q

@tombuildsstuff
Copy link
Contributor

As an aside, the function app logic will need refactoring as it is fairly inefficient and not DRY. Create calls update, likely to update the app_settings, but update basically reruns the same ARM call that Create just made, slowing down the progress by approx 20 to 30 seconds.

Some context here: for both App Service and Function Apps the App Settings weren't persisted on the initial creation (at least when we first worked on this, there's a new API version so this may have changed) - so this was the only way to ensure the settings were actually set (and that users wouldn't see a perpetual diff)

@ranieuwe
Copy link
Contributor Author

Changes done, nil check added and logic inversed. Tests all pass.

```
$ acctests azurerm TestAccAzureRMFunctionApp_basic
=== RUN   TestAccAzureRMFunctionApp_basic
--- PASS: TestAccAzureRMFunctionApp_basic (111.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	111.938s
```

```
$ acctests azurerm TestAccAzureRMFunctionApp_consumptionPlan
=== RUN   TestAccAzureRMFunctionApp_consumptionPlan
--- PASS: TestAccAzureRMFunctionApp_consumptionPlan (143.36s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	143.406s
```
@tombuildsstuff
Copy link
Contributor

hey @ranieuwe

I hope you don't mind - but I've taken a look into this and have pushed a commit which verifies the settings aren't set at the server end :)

Thanks!

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @ranieuwe

Thanks for pushing those changes - I've pushed a commit to add a test for this but this now LGTM 👍

Thanks!

@tombuildsstuff
Copy link
Contributor

Tests pass:

screenshot 2018-07-16 at 14 28 53

@tombuildsstuff tombuildsstuff merged commit 6ec6d71 into hashicorp:master Jul 16, 2018
tombuildsstuff added a commit that referenced this pull request Jul 16, 2018
@ghost
Copy link

ghost commented Mar 30, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App Service Plan based Function App has erroneous application settings
4 participants