-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_postgresql_flexible_server - mark public_network_access_enabled as optional #25812
Changes from 12 commits
7168d14
0ef5691
ca141dc
e2b2a52
3ab28ee
97087eb
c5971c5
8fe8a0b
ea8f0dd
a20a509
7555138
d1fea46
66efd11
a4f67cc
7c5f48e
de337e5
a4bd274
853cc27
19ae87e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -278,7 +278,7 @@ func resourcePostgresqlFlexibleServer() *pluginsdk.Resource { | |||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
"public_network_access_enabled": { | ||||||||||||||||||||||||||||||
Type: pluginsdk.TypeBool, | ||||||||||||||||||||||||||||||
Computed: true, | ||||||||||||||||||||||||||||||
Optional: true, | ||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
"replication_role": { | ||||||||||||||||||||||||||||||
|
@@ -767,7 +767,7 @@ func resourcePostgresqlFlexibleServerUpdate(d *pluginsdk.ResourceData, meta inte | |||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if d.HasChange("private_dns_zone_id") { | ||||||||||||||||||||||||||||||
if d.HasChange("private_dns_zone_id") || d.HasChange("public_network_access_enabled") { | ||||||||||||||||||||||||||||||
parameters.Properties.Network = expandArmServerNetwork(d) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -964,6 +964,15 @@ func expandArmServerNetwork(d *pluginsdk.ResourceData) *servers.Network { | |||||||||||||||||||||||||||||
network.PrivateDnsZoneArmResourceId = utils.String(v.(string)) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// `d.GetOk()` cannot identify if the bool property `public_network_access_enabled` is set or not in the tf config since d.GetOk() always returns `false` when `public_network_access_enabled` is set to `false` and `public_network_access_enabled` isn't set in the tf config | ||||||||||||||||||||||||||||||
if !pluginsdk.IsExplicitlyNullInConfig(d, "public_network_access_enabled") { | ||||||||||||||||||||||||||||||
publicNetworkAccessEnabled := servers.ServerPublicNetworkAccessStateDisabled | ||||||||||||||||||||||||||||||
if d.Get("public_network_access_enabled").(bool) { | ||||||||||||||||||||||||||||||
publicNetworkAccessEnabled = servers.ServerPublicNetworkAccessStateEnabled | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
network.PublicNetworkAccess = pointer.To(publicNetworkAccessEnabled) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By defaulting this in the Read we can just pull from the config:
Suggested change
Note: since the Azure API defaults this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I above mentioned, I assume that we can't simply hard-code the default value for public_network_access_enabled. So I may not apply this suggestion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return &network | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be a breaking change for existing users? as it will now set everyone to false whenthe default iirc is true?
as such we should make it optional + computed to account for existing resources + users changing the default outside terraform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. When user created the resource without
delegated_subnet_id
,private_dns_zone_id
andpublic_network_access_enabled
using old AzureRM version and then user upgraded to new AzureRM version,tf plan
would cause difference ifpublic_network_access_enabled
isn't set in the tf config. Though there is TF difference but it's expected since service API would return the default valuetrue
forpublic_network_access_enabled
whendelegated_subnet_id
andprivate_dns_zone_id
aren't set.OK. I will use optional + computed. Then the existing resources wouldn't be impacted anymore.