`azurerm_mysql_flexible_server` - convert `public_network_access_enabled` from attribute to argument #28890

wyattfry · 2025-02-26T17:02:53Z

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 comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This PR converts the existing attribute public_network_access_enabled into an optional argument, defaulting to true.

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 relevant 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)

------- Stdout: -------
=== RUN   TestAccMySqlFlexibleServer_updatePublicNetworkAccess
=== PAUSE TestAccMySqlFlexibleServer_updatePublicNetworkAccess
=== CONT  TestAccMySqlFlexibleServer_updatePublicNetworkAccess
--- PASS: TestAccMySqlFlexibleServer_updatePublicNetworkAccess (573.76s)
PASS

I only included the test results for the one I added that just tests the property in question and not all the other tests for this service because most of the other ones have been failing for months and are worth discussing as a team before trying to fix them.

Change Log

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

azurerm_mysql_flexible_server - convert public_network_access_enabled from attribute to argument [GH-XXXX]

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 #26156

Note

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

wyattfry · 2025-02-26T20:44:52Z

internal/services/mysql/mysql_flexible_server_resource_test.go

+			Check: acceptance.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+			),


Are these Checks made unnecessary by the ImportSteps?

We have it here to confirm it exists in Azure. Sometimes it doesn't even though it created because Azure is weird. You're probably right that we don't need it if we're checking import but it only costs an extra api call to doubly make sure it at least exists in Azure before we move on. Similar to how we had those old checks that we don't do anymore, we might move in that direction of removing this as well but for now, let's keep it consistent with how we do other tests

Roger. I was considering removing them, but I looked through the code base and couldn't find any precedent of test steps without checks, so I left them in.

wyattfry · 2025-02-26T20:45:33Z

internal/services/mysql/mysql_flexible_server_resource_test.go

@@ -1359,3 +1388,20 @@ resource "azurerm_mysql_flexible_server" "test" {
 }
 `, r.template(data), acceptance.WriteOnlyKeyVaultSecretTemplate(data, secret), data.RandomInteger, version)
 }
+
+func (r MySqlFlexibleServerResource) publicNetworkAccess(data acceptance.TestData, publicNetworkAccessEnabled string) string {


NB: I made publicNetworkAccessEnabled a string (i.e. not a boolean) so it could be true, false or null.

mbfrahry

Hey @wyattfry, looks good but I've just got some questions around a few areas before giving it a final sign off

mbfrahry · 2025-02-26T21:18:49Z

internal/services/mysql/mysql_flexible_server_resource.go

+			"public_network_access_enabled": {
+				Type:     pluginsdk.TypeBool,
+				Optional: true,
+				Computed: true,


Do we know if this is defaulted to something if we create it? We could default it rather than Compute it if so

Yes, Azure defaults it to true

Sweet! Then we can default it to True as well

Swagger indicates that the value of publicNetworkAccess is "Disabled" when server has VNet integration. Could you double confirm that the default value is always "Enabled"?

Thanks Neil! I will test it.

mbfrahry · 2025-02-26T21:23:03Z

internal/services/mysql/mysql_flexible_server_resource_test.go

+			Check: acceptance.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+			),


We have it here to confirm it exists in Azure. Sometimes it doesn't even though it created because Azure is weird. You're probably right that we don't need it if we're checking import but it only costs an extra api call to doubly make sure it at least exists in Azure before we move on. Similar to how we had those old checks that we don't do anymore, we might move in that direction of removing this as well but for now, let's keep it consistent with how we do other tests

mbfrahry · 2025-02-26T21:24:25Z

internal/services/mysql/mysql_flexible_server_resource_test.go

+
+	data.ResourceTest(t, r, []acceptance.TestStep{
+		{
+			Config: r.publicNetworkAccess(data, "null"),


We normally don't test null but we might want to so I'm curious what are you hoping to see out of this test step and whether we should do this going forward?

Testing the default behavior. The Azure REST API has the public access enabled as optional, so it doesn't need to be sent, so this is just to ensure we don't have one of those Computed / Optional situations I like had before, where when not specified, the first plan after the initial create wants to change the setting to the zero value.

But if we set a default on the Terraform layer that matches the default on the Azure layer, then testing with null here would be less valuable.

Yea...so I think since Azure defaults public_network_access enabled to true, we can also default it to true and then don't have to test null

mbfrahry · 2025-02-26T22:29:58Z

internal/services/mysql/mysql_flexible_server_resource.go

@@ -243,6 +243,12 @@ func resourceMysqlFlexibleServer() *pluginsdk.Resource {
 				ValidateFunc: privatezones.ValidatePrivateDnsZoneID,
 			},

+			"public_network_access_enabled": {


One more note here, Microsoft is adding more options to public_network_access_enabled so a boolean won't work when that change gets added. We should rename this to public_network_access and have it be a string

Roger, thanks! Will do.

wyattfry added 2 commits February 26, 2025 11:52

take 2

a86cc25

reorganize acc test

b291a9d

github-actions bot added service/mysql size/M enhancement labels Feb 26, 2025

remove loggin

ca35049

github-actions bot added the documentation label Feb 26, 2025

wyattfry added 2 commits February 26, 2025 15:32

remove cruft

cbefea1

remove public network default

f053e78

wyattfry mentioned this pull request Feb 26, 2025

azurerm_mysql_flexible_server - convert public_network_access_enabled from attribute to argument #28874

Closed

14 tasks

wyattfry commented Feb 26, 2025

View reviewed changes

wyattfry marked this pull request as ready for review February 26, 2025 20:51

wyattfry requested a review from a team as a code owner February 26, 2025 20:51

mbfrahry requested changes Feb 26, 2025

View reviewed changes

github-actions bot assigned mbfrahry Feb 26, 2025

github-actions bot added the waiting-response label Feb 26, 2025

wyattfry marked this pull request as draft February 26, 2025 22:17

github-actions bot removed the waiting-response label Feb 26, 2025

mbfrahry reviewed Feb 26, 2025

View reviewed changes

github-actions bot added the waiting-response label Feb 26, 2025

wip

4d31a1c

github-actions bot added waiting-response and removed waiting-response labels Feb 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_mysql_flexible_server` - convert `public_network_access_enabled` from attribute to argument #28890

`azurerm_mysql_flexible_server` - convert `public_network_access_enabled` from attribute to argument #28890

wyattfry commented Feb 26, 2025 •

edited

Loading

wyattfry Feb 26, 2025

mbfrahry Feb 26, 2025

wyattfry Feb 26, 2025

wyattfry Feb 26, 2025 •

edited

Loading

mbfrahry left a comment

mbfrahry Feb 26, 2025

wyattfry Feb 26, 2025

mbfrahry Feb 26, 2025

neil-yechenwei Feb 27, 2025

wyattfry Feb 27, 2025

mbfrahry Feb 26, 2025

mbfrahry Feb 26, 2025 •

edited

Loading

wyattfry Feb 26, 2025 •

edited

Loading

mbfrahry Feb 26, 2025

mbfrahry Feb 26, 2025

wyattfry Feb 27, 2025

azurerm_mysql_flexible_server - convert public_network_access_enabled from attribute to argument #28890

Are you sure you want to change the base?

azurerm_mysql_flexible_server - convert public_network_access_enabled from attribute to argument #28890

Conversation

wyattfry commented Feb 26, 2025 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyattfry Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

mbfrahry 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

mbfrahry Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

wyattfry Feb 26, 2025 • edited Loading

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

`azurerm_mysql_flexible_server` - convert `public_network_access_enabled` from attribute to argument #28890

`azurerm_mysql_flexible_server` - convert `public_network_access_enabled` from attribute to argument #28890

wyattfry commented Feb 26, 2025 •

edited

Loading

wyattfry Feb 26, 2025 •

edited

Loading

mbfrahry Feb 26, 2025 •

edited

Loading

wyattfry Feb 26, 2025 •

edited

Loading