-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/azurerm: Create azure App Service Plan resource #13634
provider/azurerm: Create azure App Service Plan resource #13634
Conversation
Hi @tombuildsstuff, here is a pull request just for the App Service Plan resource. I have kept just the App Service resource in the previous pull request #12001. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lstolyarov
Thanks for splitting this PR out - apologies for the delay reviewing this.
I've taken a look and left some comments in-line - however it possible to add some Acceptance Tests covering the App Service Plan? Here's an example for the Redis Cache resource
As I mentioned in #12001 - unfortunately this PR is blocked on #14004 - once that's merged we can proceed with this PR.
Thanks! :)
tier := d.Get("tier").(string) | ||
|
||
sku := web.SkuDescription{ | ||
Name: &tier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to also specify the Size and the Tier here too, as we do in the other resources? We tend to make sku
a hash like so:
sku {
name = "Standard_S0"
tier = "Standard"
size = "S0"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombuildsstuff do you mean that I have to pass in the Size and Tier as inputs to the provider? At the moment I am only passing in the service tier, e.g. "S0" which determines the tier.
client := meta.(*ArmClient) | ||
AppServicePlanClient := client.appServicePlansClient | ||
|
||
log.Printf("[INFO] preparing arguments for Azure ARM Server Farm creation.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we rename this from Server Farm
-> App Service Plan
to match the resource?
} | ||
|
||
d.Set("name", name) | ||
d.Set("resource_group_name", resGroup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to set the other properties here, i.e. tier
/ maximum_number_of_workers
etc
return err | ||
} | ||
if read.ID == nil { | ||
return fmt.Errorf("Cannot read Server farm %s (resource group %s) ID", name, resGroup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename Server Farm
-> App Service Plan
here too?
d.SetId("") | ||
return nil | ||
} | ||
return fmt.Errorf("Error making Read request on Azure Server Farm %s: %s", name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename Server Farm
-> App Service Plan
here too?
Migrated to hashicorp/terraform-provider-azurerm#1 |
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
No description provided.