Terraform-provider-azurerm: Eliminate repetition when specifying SKU sizing

Created on 5 Jul 2018  Β·  8Comments  Β·  Source: terraform-providers/terraform-provider-azurerm

Community Note

  • Please vote on this issue by adding a πŸ‘ reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

When specifying the sku for postgres and redis (and probably some other resources) you need to repeat yourself when specifying SKUs, example:

resource "azurerm_postgresql_server" "test" {
  name                = "postgresql-server-1"

  sku {
    name = "B_Gen4_2"
    capacity = 2
    tier = "Basic"
    family = "Gen4"
  }
…
}

resource "azurerm_redis_cache" "test" {
  name                = "tf-redis-basic"
  family              = "C"
  sku_name            = "Basic"
…
}

resource "azurerm_application_gateway" "network" {
  name                = "my-application-gateway-12345"
  sku {
    name           = "Standard_Small"
    tier           = "Standard"
    capacity       = 2
  }
…
}

There is unnecessary repetition here - e.g. we can eliminate name from the pg resource and family from redis since they can be 100% inferred from the other values

New or Affected Resource(s)

  • azurerm_postgresql_server
  • azurerm_redis_cache
  • azurerm_application_gateway

Potential Terraform Configuration

resource "azurerm_postgresql_server" "test" {
  name                = "postgresql-server-1"

  sku {
    capacity = 2
    tier = "Basic"
    family = "Gen4"
  }
…
}

resource "azurerm_postgresql_server" "test" {
  name                = "postgresql-server-1"

  sku {
    name = "B_Gen4_2"
  }
…
}

resource "azurerm_redis_cache" "test" {
  name                = "tf-redis-basic"
  sku_name            = "Basic"
…
}

resource "azurerm_application_gateway" "network" {
  name                = "my-application-gateway-12345"
  sku {
    name           = "Standard_Small"
    capacity       = 2
  }
…
}

References

ΓΈ

enhancement user-experience

Most helpful comment

@lfshr we deviate from the Go SDK repository where it makes sense to provide a better UX - I'd agree this'd be a good example of where we should deviate. Thinking about this, I think the best path here would be to make the name field optional and compute it from the other fields for the following reasons:

  1. this makes the diff clearer e.g. capacity: 3 => 2
  2. This allows us to apply validation as needed (for instance, if it's not possible to change the SKU but it is possible to change the capacity / the capacity can only be in given intervals)
  3. We shouldn't rely on being able to tell that B (the first character of the name field in the example above) will mean Basic (for example, in the past Basic has been renamed Classic, and ManagedBasic was introduced to certain resources); whereas the inverse is true.

This means we should be able to make this block:

 sku {
  capacity = 2
  tier = "Basic"
  family = "Gen4"
}

I can't give a timeframe for getting to this at the moment, but this is something I think we should do πŸ‘

All 8 comments

Regarding the choice of:

  sku {
    capacity = 2
    tier = "Basic"
    family = "Gen4"
  }

vs.

  sku {
    name = "B_Gen4_2"
  }

It would of course be more flexible to allow both forms, but if I were to choose one I would prefer the first as it lets logic decisions be easily made on each component.

The resources essentially wrap the Azure/azure-rest-api-specs repo. I don't know if deviating from the swagger specs is the best idea. Defaults might be a happy medium here, but not ideal.

@lfshr we deviate from the Go SDK repository where it makes sense to provide a better UX - I'd agree this'd be a good example of where we should deviate. Thinking about this, I think the best path here would be to make the name field optional and compute it from the other fields for the following reasons:

  1. this makes the diff clearer e.g. capacity: 3 => 2
  2. This allows us to apply validation as needed (for instance, if it's not possible to change the SKU but it is possible to change the capacity / the capacity can only be in given intervals)
  3. We shouldn't rely on being able to tell that B (the first character of the name field in the example above) will mean Basic (for example, in the past Basic has been renamed Classic, and ManagedBasic was introduced to certain resources); whereas the inverse is true.

This means we should be able to make this block:

 sku {
  capacity = 2
  tier = "Basic"
  family = "Gen4"
}

I can't give a timeframe for getting to this at the moment, but this is something I think we should do πŸ‘

I am proposing to flatten the below 17 resources:

NOTE: Checks mark the resources that are currently complete

  • [x] resource_arm_api_management
  • [ ] resource_arm_app_service_plan
  • [X] resource_arm_automation_account
  • [x] resource_arm_cognitive_account
  • [x] resource_arm_devspace_controller
  • [x] resource_arm_iothub
  • [X] resource_arm_key_vault
  • [X] resource_arm_mariadb_server
  • [ ] resource_arm_mssql_elasticpool
  • [X] resource_arm_mysql_server
  • [X] resource_arm_notification_hub_namespace
  • [X] resource_arm_postgresql_server
  • [X] resource_arm_relay_namespace
  • [ ] ~resource_arm_signalr_service~ - no gains
  • [ ] ~resource_arm_application_gateway~ - no gains
  • [ ] ~resource_arm_express_route_circuit~ - no gains
  • [ ] ~resource_arm_virtual_machine_scale_set~ - fix in new 2,0 resources

The SKU attributes in all of these resources would no longer be defined in block groups but rather as a single attribute, for example:

resource "azurerm_postgresql_server" "test" {
  name      = "postgresql-server-1"
  sku_name  = "B_Gen4_2"
}

resource "resource_arm_express_route_circuit" "test" {
  name      = "express-route-circuit-1"
  sku_name  = "Standard_Metered"
}

resource "resource_arm_cognitive_account" "test" {
  name      = "cognitive-account-1"
  sku_name  = "P2_Premium"
}

resource "azurerm_app_service_plan" "test" {
  name      = "app-service-plan-1"
  sku_name  = "Dynamic_Y1"
}

NOTE: For backwards compatibility I will leave the existing sku structure as is and mark it as deprecated. The existing sku attribute will be removed in the 2.0 timeframe.

@WodansSon,

I have opened some more PRs to address the outstanding resources. of the remaining ones i think azurerm_application_gateway, azurerm_express_route_circuit, azurerm_signalr_service are fine as is.

I'm going to close this as only app service and mssql elastic pool remain and they are a bit more complicated and require more thought.

This has been released in version 2.0.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.0.0"
}
# ... other configuration ...

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!

Was this page helpful?
0 / 5 - 0 ratings