azurerm_postgresql_flexible_server: fix storage_tier calculation

[HappyTobi]

Community Note

• Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
• Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

The PR fix an issue with azurerm_postgresql_flexible_server.
The issue can be reproduces when deploying an instance without a storage_tier property.
The redeployment of the same instance will fail when updating the storage_mb without setting the storage_tier.

The fix will handle the "autoupdate" of the storage_tier property when the value is not updated manually. (like the azure portal).

PR Checklist

• I have followed the guidelines in our Contributing Documentation.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
• I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
• I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
  For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

• I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
• I have written new tests for my resource or datasource changes & updated any relevent documentation.
• I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
• (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

• My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

For state migrations please test the changes locally and provide details here, such as the versions involved in testing the migration path. For further details on testing state migration changes please see our guide on state migrations in the contributor documentation. -->

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

• azurerm_resource - support for the thing1 property [GH-00000]

This is a (please select all that apply):

• Bug Fix
• New Feature (ie adding a service, resource, or data source)
• Enhancement
• Breaking Change

Related Issue(s)

Fixes #25938

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

[neil-yechenwei]

Suggest run all related test cases to ensure this fix wouldn't introduce the regression issue.

[neil-yechenwei]

There is a regression issue on the existing test case.

------- Stdout: -------
=== RUN   TestAccPostgresqlFlexibleServer_geoRestore
=== PAUSE TestAccPostgresqlFlexibleServer_geoRestore
=== CONT  TestAccPostgresqlFlexibleServer_geoRestore
    testcase.go:113: Step 3/4 error: After applying this test step, the plan was not empty.
        stdout:
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        Terraform will perform the following actions:
          # azurerm_postgresql_flexible_server.geo_restore will be updated in-place
          ~ resource "azurerm_postgresql_flexible_server" "geo_restore" {
                id                                = "/subscriptions/*******/resourceGroups/acctestRG-postgresql-240514110942768666/providers/Microsoft.DBforPostgreSQL/flexibleServers/acctest-fs-restore-240514110942768666"
                name                              = "acctest-fs-restore-240514110942768666"
              + storage_tier                      = "P4"
                # (14 unchanged attributes hidden)
                # (1 unchanged block hidden)
            }
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccPostgresqlFlexibleServer_geoRestore (2753.46s)
FAIL

[neil-yechenwei]

Thanks for this PR - I've taken a look through and left some comments inline.

[HappyTobi]

Test runs fine:
TF_ACC=1 go test -v ./internal/services/postgres -run=TestAccPostgresqlFlexibleServer_updateOnlyWithStorageMb -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN TestAccPostgresqlFlexibleServer_updateOnlyWithStorageMb
=== PAUSE TestAccPostgresqlFlexibleServer_updateOnlyWithStorageMb
=== CONT TestAccPostgresqlFlexibleServer_updateOnlyWithStorageMb
--- PASS: TestAccPostgresqlFlexibleServer_updateOnlyWithStorageMb (683.51s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/postgres 683.547s

[WodansSon]

@HappyTobi, thank you for opening this PR! Love to see the community contributions! Thank you so much for that! Looking at the linked issue and the PR, I think there maybe an easier way to fix this, but by all means please correct me if I am overlooking something. There is a hierarchy here, top level being storage_mb, that field trumps all, secondly is the storage_tier. In the update function the only thing we should have to do is get the new storage_mb field value and see if the current storage_tier is in the returned valid values for the defined storage_mb from the already exposed InitializeFlexibleServerStorageTierDefaults function. We should then check to see if the storage_tier is actually in the configuration file or not, as Neil has stated above. If the storage_tier field is not in the configuration file we should just update that field value to be the default value that was returned from the InitializeFlexibleServerStorageTierDefaults function, if it is in the configuration file and the storage_tier is not in the list of of valid storage tiers from the InitializeFlexibleServerStorageTierDefaults function, we should then raise the error that it is not a supported storage_tier for the defined storage_mb? Does that make sense, am I overlooking something?

[WodansSon]

@HappyTobi, interesting side tangent... your test name got me thinking... updateOnlyWithStorageMb should the reverse be possible as well (e.g., updateOnlyWithStorageTier) ?

[HappyTobi]

Added a 2nd test:

TF_ACC=1 go test -v ./internal/services/postgres -run=TestAccPostgresqlFlexibleServer_updateOnlyWithStorageTier -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN TestAccPostgresqlFlexibleServer_updateOnlyWithStorageTier
=== PAUSE TestAccPostgresqlFlexibleServer_updateOnlyWithStorageTier
=== CONT TestAccPostgresqlFlexibleServer_updateOnlyWithStorageTier

--- PASS: TestAccPostgresqlFlexibleServer_updateOnlyWithStorageTier (934.99s)
PASS

[HappyTobi]

ate that field value to be the default value that was returned from the InitializeFlexibleServerStorageTierDefaults function, if it is in the configuration file and the storage_tier is not in the list of of valid storage tiers from the InitializeFlexibleServerStorageTierDefaults function, we should then raise the error that it is not a supported storage_tier for the defined storage_mb? Does that make sense

Yes that make sense and the implementation reflect that already.
The change I've done is that I always use the "default" 'storage_tier' for the configures storage_mb / size.

[WodansSon]

I think we can remove all of this code and handle everything in the !isValid check below:

Suggested change

	// set default tier for storage when there is now configuration available
	if v := diff.GetRawConfig().AsValueMap()["storage_tier"]; v.IsNull() {
	newTier = string(storageTiers.DefaultTier)
	// set the new value that the update function will pickup and deploy new new storage tier.
	diff.SetNew("storage_tier", newTier)
	}

[WodansSon]

What do you think if we just replace the current if !isValid { code below with this instead:

Suggested change

	if !isValid {
	if strings.EqualFold(oldTierRaw.(string), newTier) {
	// The tier value did not change, so we need to determin if they are
	// using the default value for the tier, or they actually defined the
	// tier in the config or not... If they did not define
	// the tier in the config we need to assign a new valid default
	// tier for the newMb value. However, if the tier is in the config
	// this is a valid error and should be returned...
	if v := diff.GetRawConfig().AsValueMap()["storage_tier"]; v.IsNull() {
	diff.SetNew("storage_tier", string(storageTiers.DefaultTier))
	log.Printf("[DEBUG]: 'storage_tier' was not valid and was not in the config assigning new default 'storage_tier' %q -> %q\n", newTier, storageTiers.DefaultTier)
	return nil
	}
	}
	return fmt.Errorf("invalid 'storage_tier' %q for defined 'storage_mb' size '%d', expected one of [%s]", newTier, newMb, azure.QuotedStringSlice(*storageTiers.ValidTiers))
	}

[HappyTobi]

This will change the logic a bit.
When you remove the "storage_tier" from you tf file, the default tier will not be set, it will use the one that you set before!

[WodansSon]

I think this is a great start to the test coverage, yet I think there are many more scenarios around this especially if we are now setting defaults when storage mb changes. I would like to see tests for storage tier set above storage tier default value, then remove the storage tier from the configuration (e.g., storage mb set to 65536 with a storage tier of P10 in the config file) then remove the storage tier P10 in the config. It should then revert tier from P10 -> P6, etc. Does that make sense?

[HappyTobi]

For sure will add one more test

[WodansSon]

@HappyTobi, thanks for pushing those changes, this LGTM now! 🚀

[github-actions]

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.