DNS: support for private dns zones #1404

andyliuliming · 2018-06-17T15:56:30Z

Fixes #793

stintel · 2018-06-18T07:57:25Z

Nice. But please get rid of the empty merge commit. I would also suggest to squash
68305e1 and a71a387 and make sure to edit the commit message. Please use correct spelling when doing so. Finally reference in the commit message that it closes #793.

tombuildsstuff

hey @andyliuliming

Thanks for this PR - apologies for the delayed review here! I've taken a look through and left some comments in-line, but if we can fix up the comments we should be able to run the tests and get this merged 👍

Thanks!

tombuildsstuff · 2018-06-29T10:20:01Z

azurerm/data_source_dns_zone.go

+			for _, rvn := range *rvns {
+				registrationVNets = append(registrationVNets, *rvn.ID)
+			}
+			if err := d.Set("registration_virtual_networks", registrationVNets); err != nil {


we need to ensure this field's set in all circumstances, so that empty lists are set if it's not present in the response; as such can we make this:

registrationVnets := make([]string, 0) if rvns := props.RegistrationVirtualNetworks; rvns != nil { for _, rvn := range *rvns { registrationVNets = append(registrationVNets, *rvn.ID) } } if err := d.Set("registration_virtual_networks", registrationVNets); err != nil { return fmt.Errorf("Error flattening `registration_virtual_networks `: %+v", err) }

(also can we pull this out into a flatten method to match the other resources)

tombuildsstuff · 2018-06-29T10:20:06Z

azurerm/data_source_dns_zone.go

+			if err := d.Set("resolution_virtual_networks", resolutionVNets); err != nil {
+				return err
+			}
+		}


(same here)

tombuildsstuff · 2018-06-29T11:09:47Z

vendor/vendor.json

-			"path": "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2017-10-01/dns",
-			"revision": "7971189ecf5a584b9211f2527737f94bb979644e",
+			"checksumSHA1": "t6ChlDNU9eaDOo9OgEf/RpWbLZE=",
+			"path": "github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2018-03-01-preview/dns",


this SDK's actually deprecated - could we switch this out for github.com/Azure/azure-sdk-for-go/services/preview/dns/mgmt/2018-03-01-preview/dns which is the same but doesn't have the deprecation

tombuildsstuff · 2018-06-29T11:14:14Z

azurerm/data_source_dns_zone.go

+				Computed: true,
+			},
+
+			"registration_virtual_networks": {


since this is a list of ID's - I think this'd be better as registration_virtual_network_ids

good suggestion.

tombuildsstuff · 2018-06-29T11:14:44Z

azurerm/data_source_dns_zone.go

+				Elem:     &schema.Schema{Type: schema.TypeString},
+			},
+
+			"resolution_virtual_networks": {


since this is a list of ID's - I think this'd be better as resolution_virtual_network_ids

tombuildsstuff · 2018-06-29T11:20:52Z

azurerm/data_source_dns_zone.go

@@ -43,6 +43,29 @@ func dataSourceArmDnsZone() *schema.Resource {
 				Set:      schema.HashString,
 			},

+			"zone_type": {


can we update the documentation to include these new fields? This is kept in ./website/docs/d/dns_zone.html.markdown

tombuildsstuff · 2018-06-29T11:21:33Z

azurerm/data_source_dns_zone.go

+				Type:     schema.TypeString,
+				Default:  nil,
+				Optional: true,
+				Computed: true,


since this is a Data Source this only needs Type: schema.TypeString and Computed: true - can we remove Default/Optional?

tombuildsstuff · 2018-06-29T11:22:46Z

azurerm/data_source_dns_zone.go

+				Type:     schema.TypeList,
+				Default:  nil,
+				Optional: true,
+				Computed: true,


since this is a Data Source and these values are returned (and not specified as inputs) - can we remove Default/Optional here?

tombuildsstuff · 2018-06-29T11:22:58Z

azurerm/data_source_dns_zone.go

+			"resolution_virtual_networks": {
+				Type:     schema.TypeList,
+				Default:  nil,
+				Optional: true,


since this is a Data Source and these values are returned (and not specified as inputs) - can we remove Default/Optional here?

tombuildsstuff · 2018-06-29T11:27:09Z

azurerm/resource_arm_dns_zone.go

@@ -45,6 +45,29 @@ func resourceArmDnsZone() *schema.Resource {
 				Set:      schema.HashString,
 			},

+			"zone_type": {


(as above) can we document these new fields? The documentation for this new resource can be found in ./website/docs/r/dns_zone.html.markdown

…ers/terraform-provider-azurerm/issues/793.w

andyliuliming · 2018-07-01T05:55:27Z

@tombuildsstuff Thanks Tom for the review. comments resolved. please help review again if it meet your requirements : )

- Switching to use flatten/expand functions - Making the Resource fields consistent in the docs - Documenting the new fields in the Data Source

tombuildsstuff

hey @andyliuliming

Thanks for pushing those changes - I've taken a look through and left some minor comments (which I'm going to push a commit for, I hope you don't mind!), but this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2018-07-02T07:51:21Z

azurerm/resource_arm_dns_zone.go


 	parameters := dns.Zone{
 		Location: &location,
 		Tags:     expandTags(tags),
+		ZoneProperties: &dns.ZoneProperties{
+			ZoneType:                    convertZoneType(zoneType),


we can switch this out for:

ZoneType: dns.ZoneType(zoneType),

hmmm, I don't think so, because we are ignoring the case.
so if customer specify "pRiVaTe" then dns.ZoneType(zoneType) would pass though "pRiVaTe" to the rest api, but azure rest api will refuse that.

most of the Azure API's are case insensitive for Enum's, but this one isn't - we can update the field to be strict about the type and then parse this as above

tombuildsstuff · 2018-07-02T07:52:05Z

azurerm/resource_arm_dns_zone.go

+		if err := d.Set("resolution_virtual_network_ids", resolutionVirtualNetworks); err != nil {
+			return err
+		}
+	}


can we ensure the field resolution_virtual_network_ids is set in either case?

tombuildsstuff · 2018-07-02T07:52:14Z

azurerm/resource_arm_dns_zone.go

+		}
+		if err := d.Set("registration_virtual_network_ids", registrationVirtualNetworks); err != nil {
+			return err
+		}
 	}


can we ensure the field registration_virtual_network_ids is set in either case?

tombuildsstuff · 2018-07-02T07:53:00Z

azurerm/data_source_dns_zone.go

+				Type:     schema.TypeList,
+				Computed: true,
+				Elem:     &schema.Schema{Type: schema.TypeString},
+			},


can we document these three fields in the Data Source too?

tombuildsstuff · 2018-07-02T08:15:30Z

azurerm/resource_arm_dns_zone.go

+		}
+		if err := d.Set("name_servers", nameServers); err != nil {
+			return err
+		}
 	}


can we split these three out into separate flatten methods?

tombuildsstuff · 2018-07-02T08:15:43Z

azurerm/resource_arm_dns_zone.go

+		resolutionVNetSubResources = append(resolutionVNetSubResources, dns.SubResource{
+			ID: &id,
+		})
+	}


can we pull these out into separate expand methods?

tombuildsstuff · 2018-07-02T10:33:48Z

@andyliuliming just realised I don't have merge permissions to your fork since it's a fork of a fork, have opened andyliuliming#1 with the changes mentioned above (I hope you don't mind!). Once that's merged we should be good to merge this :)

Changes from hashicorp#1404

tombuildsstuff · 2018-07-02T13:23:35Z

DNS Zone tests pass:

ghost · 2020-03-30T15:26:39Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

andyliuliming force-pushed the andliu-private_dns_zones branch from 196082d to a71a387 Compare June 18, 2018 05:09

andyliuliming mentioned this pull request Jun 18, 2018

Private DNS Preview support ? #793

Closed

andyliuliming force-pushed the andliu-private_dns_zones branch 5 times, most recently from 8c27990 to 4113acc Compare June 28, 2018 06:25

DNS: upgrade to the 2018-03-01 Preview SDK

a540fc0

andyliuliming force-pushed the andliu-private_dns_zones branch from 4113acc to c6a2437 Compare June 28, 2018 06:27

andyliuliming changed the title ~~private dns zones support.~~ Private dns zones support. resolved https://github.com/terraform-providers/terraform-provider-azurerm/issues/793 Jun 28, 2018

andyliuliming changed the title ~~Private dns zones support. resolved https://github.com/terraform-providers/terraform-provider-azurerm/issues/793~~ Private dns zones support. resolved #793 Jun 28, 2018

ranieuwe mentioned this pull request Jun 28, 2018

Question: Reverse Lookup On Private DNS Zone #607

Closed

tombuildsstuff added service/dns enhancement labels Jun 29, 2018

tombuildsstuff self-requested a review June 29, 2018 09:43

tombuildsstuff suggested changes Jun 29, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jun 29, 2018

tombuildsstuff added this to the Soon milestone Jun 29, 2018

tombuildsstuff changed the title ~~Private dns zones support. resolved #793~~ DNS: support for private dns zones Jun 29, 2018

Privat dns zone support. resolved https://github.com/terraform-provid…

db23bf4

…ers/terraform-provider-azurerm/issues/793.w

andyliuliming force-pushed the andliu-private_dns_zones branch from c6a2437 to db23bf4 Compare July 1, 2018 05:51

tombuildsstuff removed the waiting-response label Jul 2, 2018

tombuildsstuff added 2 commits July 2, 2018 10:16

Refactoring from PR comments:

d4e68de

- Switching to use flatten/expand functions - Making the Resource fields consistent in the docs - Documenting the new fields in the Data Source

Making the spacing of the DNS tests consistent

1616693

tombuildsstuff approved these changes Jul 2, 2018

View reviewed changes

Making the Zone Type case sensitive

f5a8e19

tombuildsstuff modified the milestones: Soon, 1.9.0 Jul 2, 2018

tombuildsstuff mentioned this pull request Jul 2, 2018

Changes from https://github.com/terraform-providers/terraform-provider-azurerm/pull/1404 andyliuliming/terraform-provider-azurerm#1

Merged

tombuildsstuff added the waiting-response label Jul 2, 2018

andyliuliming added 2 commits July 2, 2018 21:09

Merge pull request #1 from terraform-providers/andliu-private_dns_zones

6b89d9e

Changes from hashicorp#1404

Merge branch 'master' into andliu-private_dns_zones

e0458f4

tombuildsstuff removed the waiting-response label Jul 2, 2018

Fixing the Govendor file

c766dfc

tombuildsstuff merged commit 670b1b9 into hashicorp:master Jul 2, 2018

tombuildsstuff added a commit that referenced this pull request Jul 2, 2018

Updating to include #1404

aa2d212

lfshr mentioned this pull request Jul 10, 2018

Support for Private DNS Zones #1528

Closed

tombuildsstuff mentioned this pull request Sep 1, 2019

azurerm_dns_zone: ability to add a resolution_virtual_network_id #2928

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

DNS: support for private dns zones #1404

DNS: support for private dns zones #1404

Conversation

andyliuliming commented Jun 17, 2018 • edited by tombuildsstuff Loading

stintel commented Jun 18, 2018

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

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

andyliuliming commented Jul 1, 2018 • edited Loading

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

tombuildsstuff Jul 2, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Jul 2, 2018

tombuildsstuff commented Jul 2, 2018

ghost commented Mar 30, 2020

andyliuliming commented Jun 17, 2018 •

edited by tombuildsstuff

Loading

andyliuliming commented Jul 1, 2018 •

edited

Loading

tombuildsstuff Jul 2, 2018 •

edited

Loading