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

Eliminate repetition when specifying SKU sizing #1500

Closed
Supermathie opened this issue Jul 5, 2018 · 8 comments
Closed

Eliminate repetition when specifying SKU sizing #1500

Supermathie opened this issue Jul 5, 2018 · 8 comments

Comments

@Supermathie
Copy link
Contributor

Supermathie commented Jul 5, 2018

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

ø

@Supermathie
Copy link
Contributor Author

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.

@lfshr
Copy link
Contributor

lfshr commented Jul 5, 2018

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.

@tombuildsstuff
Copy link
Contributor

@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 👍

@tombuildsstuff tombuildsstuff added this to the Future milestone Nov 5, 2018
@WodansSon WodansSon self-assigned this Jan 29, 2019
@WodansSon WodansSon modified the milestones: Future, v2.0.0 Mar 21, 2019
@WodansSon
Copy link
Collaborator

WodansSon commented Mar 21, 2019

I am proposing to flatten the below 17 resources:

NOTE: Checks mark the resources that are currently complete

  • resource_arm_api_management
  • resource_arm_app_service_plan
  • resource_arm_automation_account
  • resource_arm_cognitive_account
  • resource_arm_devspace_controller
  • resource_arm_iothub
  • resource_arm_key_vault
  • resource_arm_mariadb_server
  • resource_arm_mssql_elasticpool
  • resource_arm_mysql_server
  • resource_arm_notification_hub_namespace
  • resource_arm_postgresql_server
  • 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.

@katbyte
Copy link
Collaborator

katbyte commented Jan 13, 2020

@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.

@katbyte
Copy link
Collaborator

katbyte commented Jan 20, 2020

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.

@katbyte katbyte closed this as completed Jan 20, 2020
@ghost
Copy link

ghost commented Feb 24, 2020

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 ...

@ghost
Copy link

ghost commented Feb 25, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

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

No branches or pull requests

5 participants