[Bug:] `azurerm_managed_lustre_file_system` - correct validation for `storage_capacity_in_tb` minimum per `sku_name` and remove hardcoded maximum `storage_capacity_in_tb` #23428

WodansSon · 2023-09-30T08:27:40Z

WodansSon · 2023-09-30T10:42:15Z

garvct · 2023-10-02T22:45:06Z

Is this bug fix scheduled to be released in v3.76.0 (~Oct 5, 2023)?

WodansSon · 2023-10-03T02:37:53Z

@garvct, I don't believe there is going to be a provider release this week, but it should be in next weeks which will be v3.76.0(~Oct 12, 2023).

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

manicminer

Thanks @WodansSon, I've taken a look and this mostly looks good. I've made a few requests inline below, if you can look at these then this should be good to merge.

manicminer · 2023-10-10T12:43:53Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

@@ -50,12 +50,45 @@ type MaintenanceWindow struct {
 	TimeOfDayInUTC string                                  `tfschema:"time_of_day_in_utc"`
 }

+type SkuProperties struct {
+	Name           string
+	MinumumStorage int


Suggested change

MinumumStorage int

MinimumStorage int

manicminer · 2023-10-10T12:45:35Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

+					if configCapacity.(int) < skuProperties.MinumumStorage {
+						return fmt.Errorf("'storage_capacity_in_tb' field cannot be less than '%d' for the %q sku, got '%d'", skuProperties.MinumumStorage, configSku, configCapacity)
+					}
+
+					if configCapacity.(int)%skuProperties.MinumumStorage != 0 {
+						return fmt.Errorf("'storage_capacity_in_tb' field must be defined in increments of '%d' for the %q sku, got '%d'", skuProperties.MinumumStorage, configSku, configCapacity)


Suggested change

if configCapacity.(int) < skuProperties.MinumumStorage {

return fmt.Errorf("'storage_capacity_in_tb' field cannot be less than '%d' for the %q sku, got '%d'", skuProperties.MinumumStorage, configSku, configCapacity)

}

if configCapacity.(int)%skuProperties.MinumumStorage != 0 {

return fmt.Errorf("'storage_capacity_in_tb' field must be defined in increments of '%d' for the %q sku, got '%d'", skuProperties.MinumumStorage, configSku, configCapacity)

if configCapacity.(int) < skuProperties.MinimumStorage {

return fmt.Errorf("'storage_capacity_in_tb' field cannot be less than '%d' for the %q sku, got '%d'", skuProperties.MinimumStorage, configSku, configCapacity)

}

if configCapacity.(int)%skuProperties.MinimumStorage != 0 {

return fmt.Errorf("'storage_capacity_in_tb' field must be defined in increments of '%d' for the %q sku, got '%d'", skuProperties.MinimumStorage, configSku, configCapacity)

manicminer · 2023-10-10T12:47:53Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

+}
+
+func PossibleValuesForSkuName() []string {
+	var skus []string


We should avoid (potentially) nil slices

Suggested change

var skus []string

skus := make([]string, 0)

manicminer · 2023-10-10T12:55:34Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

+		{"AMLFS-Durable-Premium-40", 48},
+		{"AMLFS-Durable-Premium-125", 16},
+		{"AMLFS-Durable-Premium-250", 8},
+		{"AMLFS-Durable-Premium-500", 4},


We also try to avoid unkeyed structs

Suggested change

{"AMLFS-Durable-Premium-40", 48},

{"AMLFS-Durable-Premium-125", 16},

{"AMLFS-Durable-Premium-250", 8},

{"AMLFS-Durable-Premium-500", 4},

{

Name: "AMLFS-Durable-Premium-40",

MinimumStorage: 48,

},

{

Name: "AMLFS-Durable-Premium-125",

MinimumStorage: 16,

},

{

Name: "AMLFS-Durable-Premium-250",

MinimumStorage: 8,

},

{

Name: "AMLFS-Durable-Premium-500",

MinimumStorage: 4,

},

manicminer · 2023-10-10T13:09:17Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

 var _ sdk.ResourceWithCustomizeDiff = ManagedLustreFileSystemResource{}

+func GetSkuPropertiesByName(skuName string) interface{} {


Can we please use a proper type here? We should be returning a *SkuProperties here

manicminer · 2023-10-10T13:12:49Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

+			_, configSku := metadata.ResourceDiff.GetChange("sku_name")
+			_, configCapacity := metadata.ResourceDiff.GetChange("storage_capacity_in_tb")


We can just use metadata.ResourceDiff.Get() here as we're not interested in the old value

manicminer · 2023-10-10T13:14:18Z

website/docs/r/managed_lustre_file_system.html.markdown

@@ -62,7 +62,9 @@ The following arguments are supported:

 * `sku_name` - (Required) The SKU name for the Azure Managed Lustre File System. Possible values are `AMLFS-Durable-Premium-40`, `AMLFS-Durable-Premium-125`, `AMLFS-Durable-Premium-250` and `AMLFS-Durable-Premium-500`. Changing this forces a new resource to be created.

-* `storage_capacity_in_tb` - (Required) The size of the Azure Managed Lustre File System in TiB. Possible values are between 8 and 128 and must be divisible by 8. Changing this forces a new resource to be created.
+* `storage_capacity_in_tb` - (Required) The size of the Azure Managed Lustre File System in TiB. The valid values for this field are dependant on which `sku_name` has been defined in the configuration file. Changing this forces a new resource to be created. For more information on the valid values for this field please see the [product documentation](https://learn.microsoft.com/azure/azure-managed-lustre/create-file-system-resource-manager#file-system-type-and-size-options).


Statement ordering

Suggested change

* `storage_capacity_in_tb` - (Required) The size of the Azure Managed Lustre File System in TiB. The valid values for this field are dependant on which `sku_name` has been defined in the configuration file. Changing this forces a new resource to be created. For more information on the valid values for this field please see the [product documentation](https://learn.microsoft.com/azure/azure-managed-lustre/create-file-system-resource-manager#file-system-type-and-size-options).

* `storage_capacity_in_tb` - (Required) The size of the Azure Managed Lustre File System in TiB. The valid values for this field are dependant on which `sku_name` has been defined in the configuration file. For more information on the valid values for this field please see the [product documentation](https://learn.microsoft.com/azure/azure-managed-lustre/create-file-system-resource-manager#file-system-type-and-size-options). Changing this forces a new resource to be created.

manicminer · 2023-10-10T13:15:50Z

website/docs/r/managed_lustre_file_system.html.markdown

-* `storage_capacity_in_tb` - (Required) The size of the Azure Managed Lustre File System in TiB. Possible values are between 8 and 128 and must be divisible by 8. Changing this forces a new resource to be created.
+* `storage_capacity_in_tb` - (Required) The size of the Azure Managed Lustre File System in TiB. The valid values for this field are dependant on which `sku_name` has been defined in the configuration file. Changing this forces a new resource to be created. For more information on the valid values for this field please see the [product documentation](https://learn.microsoft.com/azure/azure-managed-lustre/create-file-system-resource-manager#file-system-type-and-size-options).
+
+-> **NOTE:** If you are interested in `storage_capacity_in_tb` values larger than what is listed as the maximum `storage_capacity_in_tb` in the product documentation for your organization, please open a Microsoft support ticket.


I don't think we should be including this in our documentation as this isn't related to the provider at all, users should be able to find this on official Azure documentation.

Suggested change

-> **NOTE:** If you are interested in `storage_capacity_in_tb` values larger than what is listed as the maximum `storage_capacity_in_tb` in the product documentation for your organization, please open a Microsoft support ticket.

manicminer · 2023-10-11T06:02:08Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

-					if configCapacity.(int) < skuProperties.MinumumStorage {
-						return fmt.Errorf("'storage_capacity_in_tb' field cannot be less than '%d' for the %q sku, got '%d'", skuProperties.MinumumStorage, configSku, configCapacity)
-					}
+			if configCapacity.(int) < skuProperties.MinimumStorage {


We'll need a nil check here to avoid a potential crash

manicminer · 2023-10-11T06:02:16Z

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go

-						return fmt.Errorf("'storage_capacity_in_tb' field must be defined in increments of '%d' for the %q sku, got '%d'", skuProperties.MinumumStorage, configSku, configCapacity)
-					}
-				}
+			if configCapacity.(int)%skuProperties.MinimumStorage != 0 {


WodansSon · 2023-10-12T03:29:16Z

manicminer

Thanks @WodansSon, this LGTM 👍

<Actions> <action id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>"hashicorp/azurerm" updated from "3.74.0" to "3.75.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.75.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.75.0
FEATURES:

* New Resource: `azurerm_application_load_balancer` ([#22517](hashicorp/terraform-provider-azurerm#22517 New Resource: `azurerm_resource_management_private_link` ([#23098](https://github.com/hashicorp/terraform-provider-azurerm/issues/23098))

ENHANCEMENTS:

* dependencies: `firewall` migrated to `hashicorp/go-azure-sdk` ([#22863](hashicorp/terraform-provider-azurerm#22863 `azurerm_bot_service_azure_bot` - add support for the `icon_url` property ([#23114](hashicorp/terraform-provider-azurerm#23114 `azurerm_cognitive_deployment` - `capacity` property is now updateable ([#23251](hashicorp/terraform-provider-azurerm#23251 `azurerm_container_group` - added support for `key_vault_user_identity_id` ([#23332](hashicorp/terraform-provider-azurerm#23332 `azurerm_data_factory` - added support for the `publish_enabled` property ([#2334](hashicorp/terraform-provider-azurerm#2334 `azurerm_firewall_policy_rule_collection_group` - add support for the `description` property ([#23354](hashicorp/terraform-provider-azurerm#23354 `azurerm_kubernetes_cluster` - `network_profile.network_policy` can be migrated to `cilium` ([#23342](hashicorp/terraform-provider-azurerm#23342 `azurerm_log_analytics_workspace` - add support for the `data_collection_rule_id` property ([#23347](hashicorp/terraform-provider-azurerm#23347 `azurerm_mysql_flexible_server` - add support for the `io_scaling_enabled` property ([#23329](https://github.com/hashicorp/terraform-provider-azurerm/issues/23329))

BUG FIXES:

* `azurerm_api_management_api` - fix importing `openapi` format content file issue ([#23348](hashicorp/terraform-provider-azurerm#23348 `azurerm_cdn_frontdoor_rule` - allow a `cache_duration` of `00:00:00` ([#23384](hashicorp/terraform-provider-azurerm#23384 `azurerm_cosmosdb_cassandra_datacenter` - `sku_name` is now updatable ([#23419](hashicorp/terraform-provider-azurerm#23419 `azurerm_key_vault_certificate` - fix a bug that prevented soft-deleted certificates from being recovered ([#23204](hashicorp/terraform-provider-azurerm#23204 `azurerm_log_analytics_solution` - fix create and update lifecycle of resource by splitting methods ([#23333](hashicorp/terraform-provider-azurerm#23333 `azurerm_management_group_subscription_association` - mark resource as gone correctly if not found when retrieving ([#23335](hashicorp/terraform-provider-azurerm#23335 `azurerm_management_lock` - add polling after create and delete to check for RP propagation ([#23345](hashicorp/terraform-provider-azurerm#23345 `azurerm_monitor_diagnostic_setting` - added validation to ensure at least one of `category` or `category_group` is supplied ([#23308](hashicorp/terraform-provider-azurerm#23308 `azurerm_palo_alto_local_rulestack_prefix_list` - fix rulestack not being committed on delete ([#23362](hashicorp/terraform-provider-azurerm#23362 `azurerm_palo_alto_local_rulestack_fqdn_list` - fix rulestack not being committed on delete ([#23362](hashicorp/terraform-provider-azurerm#23362 `security_center_subscription_pricing_resource` - disabled extensions logic now works as expected ([#22997](https://github.com/hashicorp/terraform-provider-azurerm/issues/22997))



</pre> </details> <details> <summary>3.76.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.76.0
FEATURES:

* New Resource: `azurerm_security_center_storage_defender` ([#23242](hashicorp/terraform-provider-azurerm#23242 New Resource: `azurerm_spring_cloud_application_insights_application_performance_monitoring` ([#23107](https://github.com/hashicorp/terraform-provider-azurerm/issues/23107))

ENHANCEMENTS:

* provider: updating to build using Go `1.21.3` ([#23514](hashicorp/terraform-provider-azurerm#23514 provider: the `roll_instances_when_required` provider feature in the `virtual_machine_scale_set` block is now optional ([#22976](hashicorp/terraform-provider-azurerm#22976 dependencies: updating to `v0.20231012.1141427` of `github.com/hashicorp/go-azure-sdk` ([#23534](hashicorp/terraform-provider-azurerm#23534 Data Source: `azurerm_application_gateway` - support for `backend_http_settings`, `global`, `gateway_ip_configuration` and additional attributes ([#23318](hashicorp/terraform-provider-azurerm#23318 Data Source: `azurerm_network_service_tags` - export the `name` attribute ([#23382](hashicorp/terraform-provider-azurerm#23382 `azurerm_cosmosdb_postgresql_cluster` - add support for `sql_version` of `16` and `citus_version` of `12.1` ([#23476](hashicorp/terraform-provider-azurerm#23476 `azurerm_palo_alto_local_rulestack` - correctly normalize the `location` property ([#23483](hashicorp/terraform-provider-azurerm#23483 `azurerm_static_site` - add support for `app_settings` ([#23421](https://github.com/hashicorp/terraform-provider-azurerm/issues/23421))

BUG FIXES:

* `azurerm_automation_schedule` - fix a bug when updating `start_time` ([#23494](hashicorp/terraform-provider-azurerm#23494 `azurerm_eventhub` - remove ForceNew and check `partition_count` is not decreased ([#23499](hashicorp/terraform-provider-azurerm#23499 `azurerm_managed_lustre_file_system` - update validation for `storage_capacity_in_tb` according to `sku_name` in use ([#23428](hashicorp/terraform-provider-azurerm#23428 `azurerm_virtual_machine` - fix a crash when the API response for the `os_profile` block contains nil properties ([#23535](https://github.com/hashicorp/terraform-provider-azurerm/issues/23535))


</pre> </details> </details> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> --------- Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com> Co-authored-by: Damien Duportal <damien.duportal@gmail.com>

github-actions · 2024-05-12T02:11:45Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Initial Check-in...

41b3b61

WodansSon added the service/azuremanagedlustrefilesystem label Sep 30, 2023

github-actions bot added the size/M label Sep 30, 2023

WodansSon added the bug label Sep 30, 2023

Update documentation...

76e5e37

github-actions bot added the documentation label Sep 30, 2023

WodansSon added this to the v3.76.0 milestone Sep 30, 2023

katbyte reviewed Oct 3, 2023

View reviewed changes

internal/services/azuremanagedlustrefilesystem/managed_lustre_file_system_resource.go Show resolved Hide resolved

github-actions bot added the waiting-response label Oct 3, 2023

WodansSon removed the waiting-response label Oct 4, 2023

manicminer suggested changes Oct 10, 2023

View reviewed changes

github-actions bot added the waiting-response label Oct 10, 2023

Address PR comments...

41449d4

github-actions bot removed the waiting-response label Oct 10, 2023

manicminer suggested changes Oct 11, 2023

View reviewed changes

github-actions bot added the waiting-response label Oct 11, 2023

manicminer self-assigned this Oct 11, 2023

WodansSon removed the waiting-response label Oct 11, 2023

Add nil check for skuProperties...

7867f0a

github-actions bot added the waiting-response label Oct 11, 2023

WodansSon removed the waiting-response label Oct 11, 2023

github-actions bot added the waiting-response label Oct 11, 2023

github-actions bot added waiting-response and removed waiting-response labels Oct 12, 2023

manicminer approved these changes Oct 12, 2023

View reviewed changes

manicminer merged commit eef5bef into main Oct 12, 2023
23 of 24 checks passed

manicminer deleted the b_amlfs_capacity branch October 12, 2023 09:20

manicminer added a commit that referenced this pull request Oct 12, 2023

Changelog for #23428

b41e300

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug:] `azurerm_managed_lustre_file_system` - correct validation for `storage_capacity_in_tb` minimum per `sku_name` and remove hardcoded maximum `storage_capacity_in_tb` #23428

[Bug:] `azurerm_managed_lustre_file_system` - correct validation for `storage_capacity_in_tb` minimum per `sku_name` and remove hardcoded maximum `storage_capacity_in_tb` #23428

WodansSon commented Sep 30, 2023

WodansSon commented Sep 30, 2023

garvct commented Oct 2, 2023

WodansSon commented Oct 3, 2023

manicminer left a comment

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 10, 2023

WodansSon Oct 10, 2023

manicminer Oct 11, 2023

manicminer Oct 11, 2023

WodansSon Oct 11, 2023

WodansSon commented Oct 12, 2023

manicminer left a comment

github-actions bot commented May 12, 2024

-		{"AMLFS-Durable-Premium-40", 48},
-		{"AMLFS-Durable-Premium-125", 16},
-		{"AMLFS-Durable-Premium-250", 8},
-		{"AMLFS-Durable-Premium-500", 4},
+		{
+			Name:           "AMLFS-Durable-Premium-40",
+			MinimumStorage: 48,
+		},
+		{
+			Name:           "AMLFS-Durable-Premium-125",
+			MinimumStorage: 16,
+		},
+		{
+			Name:           "AMLFS-Durable-Premium-250",
+			MinimumStorage: 8,
+		},
+		{
+			Name:           "AMLFS-Durable-Premium-500",
+			MinimumStorage: 4,
+		},

		var _ sdk.ResourceWithCustomizeDiff = ManagedLustreFileSystemResource{}

		func GetSkuPropertiesByName(skuName string) interface{} {

		_, configSku := metadata.ResourceDiff.GetChange("sku_name")
		_, configCapacity := metadata.ResourceDiff.GetChange("storage_capacity_in_tb")

[Bug:] azurerm_managed_lustre_file_system - correct validation for storage_capacity_in_tb minimum per sku_name and remove hardcoded maximum storage_capacity_in_tb #23428

[Bug:] azurerm_managed_lustre_file_system - correct validation for storage_capacity_in_tb minimum per sku_name and remove hardcoded maximum storage_capacity_in_tb #23428

Conversation

WodansSon commented Sep 30, 2023

WodansSon commented Sep 30, 2023

garvct commented Oct 2, 2023

WodansSon commented Oct 3, 2023

manicminer 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

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

WodansSon commented Oct 12, 2023

manicminer left a comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2024

[Bug:] `azurerm_managed_lustre_file_system` - correct validation for `storage_capacity_in_tb` minimum per `sku_name` and remove hardcoded maximum `storage_capacity_in_tb` #23428

[Bug:] `azurerm_managed_lustre_file_system` - correct validation for `storage_capacity_in_tb` minimum per `sku_name` and remove hardcoded maximum `storage_capacity_in_tb` #23428