Feature: Add 'ip_version' to Public IP Address #2019

Krueladin · 2018-10-04T03:21:58Z

Fixes #412

Krueladin · 2018-10-04T03:28:16Z

Currently added the ip_version as an optional argument to avoid breaking changes. Could be changed to be required at a later time.

katbyte

Hi @Krueladin,

Thank you for the PR, the comprehensive tests are appreciated! Aside from one comment I've left inline my only concern is the documentation should be updated to reflect the new property

Also the datasource should be updated with the new property as well.

azurerm/resource_arm_public_ip.go

katbyte · 2018-10-04T16:25:58Z

azurerm/resource_arm_public_ip.go

@@ -209,6 +230,7 @@ func resourceArmPublicIpRead(d *schema.ResourceData, meta interface{}) error {
 	if location := resp.Location; location != nil {
 		d.Set("location", azureRMNormalizeLocation(*location))
 	}
+	d.Set("ip_version", strings.ToLower(string(resp.PublicIPAddressPropertiesFormat.PublicIPAddressVersion)))


it appears you are setting this twice, the one below is the way to go

azurerm/resource_arm_public_ip.go

tombuildsstuff

hey @Krueladin

Thanks for pushing those changes - I've taken a look through and left a couple of extra (minor) comments inline, if we can fix those up then this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2018-10-05T03:50:04Z

azurerm/data_source_public_ip.go

@@ -20,6 +20,11 @@ func dataSourceArmPublicIP() *schema.Resource {

 			"resource_group_name": resourceGroupNameForDataSourceSchema(),

+			"ip_version": {
+				Type:     schema.TypeString,
+				Optional: true,


since this is a Data Source this needs to be Computed rather than Optional (since the information is retrieved but not setable)

tombuildsstuff · 2018-10-05T03:54:36Z

azurerm/resource_arm_public_ip.go

+					string(network.IPv6),
+				}, true),
+				DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
+				StateFunc:        ignoreCaseStateFunc,


since this is a new field, can we make this case-sensitive? (e.g. true -> false in the validate func, and remove the DiffSuppressFunc/StateFunc)

Do you not think it should be accepted if someone decides to use "ipv4" instead of "IPv4"?

the issue's to do with casting the string value to an Enum - if the casing doesn't match, Go defaults to the last value in the Enum, meaning the wrong value could be selected; as such we need to ensure the correct casing is used here (e.g. IPv4 rather than ipv4)

I'm making the change, but I haven't seen this as an issue yet. wouldn't the only fear of this be in reading the data? And with the data object IPv6 is the last value, but the test still is able to read IPv4. And there were many purposefully strangely cased examples in my tests. Where in particular could you see this coming from?

indeed - it's possible for the casing to be different for example where resources are provisioned via an ARM Template/the CLI and then imported

Cool. @tombuildsstuff I'll make those last changes then and that should be it. Thanks for helping with my first contribution to open source! :)

tombuildsstuff · 2018-10-05T03:55:34Z

azurerm/resource_arm_public_ip.go

+	ipVersion := network.IPVersion(d.Get("ip_version").(string))
+
+	if strings.ToLower(string(ipVersion)) == "ipv6" {
+		if strings.ToLower(string(ipAllocationMethod)) == "static" {


minor rather than lower-casing it we can use strings.EqualsFold(string(ipAllocationMethod), "static") here to use a case-invarient check

Am making the change. But I am curious, is there a particular reason for this preference?

heh sorry I missed out using the Enum here - in general we'd tend to do something like:

strings.EqualsFold(string(ipAllocationMethod), string(network.Static))

although in this specific case (if we remove the case sensitivity) - we should actually be able to do a strict equality comparison I guess? 🤔

Go would not complain in this case of comparing an enum and a string?

@Krueladin sorry, that's a typo - it would - I've updated the example

tombuildsstuff · 2018-10-05T03:56:32Z

website/docs/d/public_ip.html.markdown

@@ -107,4 +107,5 @@ output "public_ip_address" {
 * `idle_timeout_in_minutes` - Specifies the timeout for the TCP idle connection.
 * `fqdn` - Fully qualified domain name of the A DNS record associated with the public IP. This is the concatenation of the domainNameLabel and the regionalized DNS zone.
 * `ip_address` - The IP address value that was allocated.
+* `ip_version` - The IP Version to use, IPv6 or IPv4. Must be used with a 'dynamic' allocation type.


minor since this is only retrieving information about the Public IP - can we update this to a slightly different tense (and remove the information about dynamic being required, since this is readonly):

* `ip_version` - The IP version being used, for example `IPv4` or `IPv6`

Update to include 'ip_version' for data and resource documentation.

tombuildsstuff

hey @Krueladin

Thanks for pushing those changes - I've taken a look through and this now LGTM 👍 so I'll kick off the tests shortly :)

Thanks!

dismissing since changes have been pushed

azurerm/resource_arm_public_ip.go

tombuildsstuff

hey @Krueladin

Thanks for pushing those changes - this now LGTM 👍

Thanks!

``` $ acctests azurerm TestAccAzureRMPublicIpStatic_zones === RUN TestAccAzureRMPublicIpStatic_zones --- PASS: TestAccAzureRMPublicIpStatic_zones (111.60s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 113.048s ```

tombuildsstuff · 2018-10-13T05:10:04Z

hey @Krueladin

I've run the tests and pushed a couple of commits (to revert my earlier change & to add back in a missing test, I hope you don't mind) - and this now LGTM:

acctests azurerm TestAccAzureRMPublicIpStatic_basic
=== RUN   TestAccAzureRMPublicIpStatic_basic
--- PASS: TestAccAzureRMPublicIpStatic_basic (119.63s)
=== RUN   TestAccAzureRMPublicIpStatic_basic_withDNSLabel
--- PASS: TestAccAzureRMPublicIpStatic_basic_withDNSLabel (110.90s)
=== RUN   TestAccAzureRMPublicIpStatic_basic_defaultsToIPv4
--- PASS: TestAccAzureRMPublicIpStatic_basic_defaultsToIPv4 (98.85s)
=== RUN   TestAccAzureRMPublicIpStatic_basic_withIPv4
--- PASS: TestAccAzureRMPublicIpStatic_basic_withIPv4 (111.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	442.395s

New Test:

$ acctests azurerm TestAccAzureRMPublicIpStatic_zones
=== RUN   TestAccAzureRMPublicIpStatic_zones
--- PASS: TestAccAzureRMPublicIpStatic_zones (111.60s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	113.048s

Since this LGTM I'll kick off the data source tests and merge this shortly - thanks again for this contribution 👍

Thanks!

tombuildsstuff · 2018-10-13T05:24:54Z

Data Source tests pass:

$ acctests azurerm TestAccDataSourceAzureRMPublicIP
=== RUN   TestAccDataSourceAzureRMPublicIP_basic
--- PASS: TestAccDataSourceAzureRMPublicIP_basic (117.13s)
=== RUN   TestAccDataSourceAzureRMPublicIPs_namePrefix
--- PASS: TestAccDataSourceAzureRMPublicIPs_namePrefix (147.84s)
=== RUN   TestAccDataSourceAzureRMPublicIPs_assigned
--- PASS: TestAccDataSourceAzureRMPublicIPs_assigned (168.31s)
=== RUN   TestAccDataSourceAzureRMPublicIPs_allocationType
--- PASS: TestAccDataSourceAzureRMPublicIPs_allocationType (141.25s)
PASS

ghost · 2019-03-06T14:23:54Z

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 👉 [email protected]. Thanks!

ghost added the size/L label Oct 4, 2018

Krueladin force-pushed the feature-public-ip-version branch from be408f1 to b2fbe78 Compare October 4, 2018 03:22

Krueladin changed the title ~~Feature: Add 'ip_version' to Public IPv6 Address~~ Feature: Add 'ip_version' to Public IP Address Oct 4, 2018

metacpp requested a review from JunyiYi October 4, 2018 06:11

metacpp added enhancement service/public-ip labels Oct 4, 2018

metacpp modified the milestone: Soon Oct 4, 2018

katbyte previously requested changes Oct 4, 2018

View reviewed changes

katbyte added this to the 1.17.0 milestone Oct 4, 2018

Krueladin force-pushed the feature-public-ip-version branch 2 times, most recently from f798e98 to 50d5f79 Compare October 5, 2018 03:16

Krueladin mentioned this pull request Oct 5, 2018

Feature Req - Ability to create an IPv6 address #412

Closed

tombuildsstuff approved these changes Oct 5, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Oct 5, 2018

Krueladin added 3 commits October 4, 2018 21:55

Feature: Add 'ip_version' to Public IP Address

4c3f858

Public IP Data Source Info for 'ip_version'

470b49a

Public IP Documentation

25adb97

Update to include 'ip_version' for data and resource documentation.

Krueladin force-pushed the feature-public-ip-version branch from 50d5f79 to 25adb97 Compare October 5, 2018 05:00

tombuildsstuff approved these changes Oct 10, 2018

View reviewed changes

Making the IP Version field case sensitive

36076c6

tombuildsstuff reviewed Oct 12, 2018

View reviewed changes

azurerm/resource_arm_public_ip.go Outdated Show resolved Hide resolved

tombuildsstuff approved these changes Oct 12, 2018

View reviewed changes

tombuildsstuff removed the waiting-response label Oct 12, 2018

tombuildsstuff removed the request for review from JunyiYi October 12, 2018 22:33

tombuildsstuff added 2 commits October 12, 2018 18:58

Fixing the broken tests

589bedc

Adding back in the Zones test

eef79d6

``` $ acctests azurerm TestAccAzureRMPublicIpStatic_zones === RUN TestAccAzureRMPublicIpStatic_zones --- PASS: TestAccAzureRMPublicIpStatic_zones (111.60s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 113.048s ```

tombuildsstuff merged commit 5862ac5 into hashicorp:master Oct 13, 2018

tombuildsstuff added a commit that referenced this pull request Oct 13, 2018

Updating to include #2019

c187201

ghost locked and limited conversation to collaborators Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add 'ip_version' to Public IP Address #2019

Feature: Add 'ip_version' to Public IP Address #2019

Krueladin commented Oct 4, 2018 •

edited by tombuildsstuff

Loading

Krueladin commented Oct 4, 2018 •

edited

Loading

katbyte left a comment

katbyte Oct 4, 2018

tombuildsstuff left a comment

tombuildsstuff Oct 5, 2018

tombuildsstuff Oct 5, 2018

Krueladin Oct 5, 2018

tombuildsstuff Oct 8, 2018

Krueladin Oct 10, 2018

tombuildsstuff Oct 10, 2018

Krueladin Oct 11, 2018

tombuildsstuff Oct 5, 2018

Krueladin Oct 5, 2018

tombuildsstuff Oct 5, 2018 •

edited

Loading

Krueladin Oct 5, 2018

tombuildsstuff Oct 8, 2018 •

edited

Loading

tombuildsstuff Oct 5, 2018

tombuildsstuff left a comment

tombuildsstuff left a comment

tombuildsstuff commented Oct 13, 2018 •

edited

Loading

tombuildsstuff commented Oct 13, 2018

ghost commented Mar 6, 2019

Feature: Add 'ip_version' to Public IP Address #2019

Feature: Add 'ip_version' to Public IP Address #2019

Conversation

Krueladin commented Oct 4, 2018 • edited by tombuildsstuff Loading

Krueladin commented Oct 4, 2018 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff 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

tombuildsstuff Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Oct 13, 2018 • edited Loading

tombuildsstuff commented Oct 13, 2018

ghost commented Mar 6, 2019

Krueladin commented Oct 4, 2018 •

edited by tombuildsstuff

Loading

Krueladin commented Oct 4, 2018 •

edited

Loading

tombuildsstuff Oct 5, 2018 •

edited

Loading

tombuildsstuff Oct 8, 2018 •

edited

Loading

tombuildsstuff commented Oct 13, 2018 •

edited

Loading