Replace DNS NS record objects with records array #991

phekmat · 2018-03-16T21:58:26Z

Replace and deprecate the azurerm_dns_ns_record.record objects with
a single records array to simplify working with DNS zone resources

The motivation behind this was primarily to be able to use azurerm_dns_zone.name_servers directly, but it's equally useful with plain variables. TXT records have the same issue and it would be nice to give it the same treatment.

katbyte

Hi @phekmat,

Thank you for opening this pr! It mostly looks good to me. Outside of the comments i have left inline:

could we continue to use the old property in create/update if the new one is not set?
could we create a copy of all tests with the old property on its own to ensure that it still works as expected until the deprecated property is removed?

Thanks 🙂 Look forward to getting this merged

katbyte · 2018-05-19T00:19:27Z

azurerm/resource_arm_dns_ns_record.go

@@ -152,31 +166,41 @@ func resourceArmDnsNsRecordDelete(d *schema.ResourceData, meta interface{}) erro
 	return nil
 }

-func flattenAzureRmDnsNsRecords(records *[]dns.NsRecord) []map[string]interface{} {
+func flattenAzureRmDnsNsRecordsSet(records *[]dns.NsRecord) []map[string]interface{} {


Could we just get a quick //todo remove this when "record" is removed here?

katbyte · 2018-05-19T00:27:09Z

azurerm/resource_arm_dns_ns_record.go

@@ -122,7 +132,11 @@ func resourceArmDnsNsRecordRead(d *schema.ResourceData, meta interface{}) error
 	d.Set("zone_name", zoneName)
 	d.Set("ttl", resp.TTL)

-	if err := d.Set("record", flattenAzureRmDnsNsRecords(resp.NsRecords)); err != nil {
+	if err := d.Set("records", flattenAzureRmDnsNsRecords(resp.NsRecords)); err != nil {
+		return err


could we wrap this error with a bit more detail, such as:

if err := d.Set("records", flattenAzureRmDnsNsRecords(resp.NsRecords)); err != nil { return fmt.Errorf("Error setting `records `: %+v", err) }

katbyte · 2018-05-19T00:27:16Z

azurerm/resource_arm_dns_ns_record.go

+		return err
+	}
+
+	if err := d.Set("record", flattenAzureRmDnsNsRecordsSet(resp.NsRecords)); err != nil {


same as above

tombuildsstuff

hey @phekmat

Thanks for pushing those updates - I've taken a look through and left some comments inline (primarily, I think we can remove the state migration since this should be covered by the refresh function), but this is mostly looking good to me :)

Thanks!

tombuildsstuff · 2018-04-03T11:55:29Z

azurerm/resource_arm_dns_ns_record.go

@@ -18,6 +18,8 @@ func resourceArmDnsNsRecord() *schema.Resource {
 		Importer: &schema.ResourceImporter{
 			State: schema.ImportStatePassthrough,
 		},
+		MigrateState:  resourceArmDNSNsMigrateState,
+		SchemaVersion: 1,


a migration shouldn't be needed for going from a Set -> a List - so I think we can remove this

tombuildsstuff · 2018-04-03T11:57:19Z

azurerm/resource_arm_dns_ns_record.go

-			"record": {
-				Type:     schema.TypeSet,
+			"records": {
+				Type:     schema.TypeList,


does the ordering matter for this? the reason for picking a Set over a List is where the fields can be returned out of order (vs a list where the ordering matters)

I had the same concern. I couldn't find an explicit guarantee that Azure won't change the order someday, but it does respect the order in the portal and the REST API/CLI for both reads and writes.

tombuildsstuff · 2018-05-24T08:37:50Z

azurerm/resource_arm_dns_ns_record.go

@@ -33,9 +35,21 @@ func resourceArmDnsNsRecord() *schema.Resource {
 				Required: true,
 			},

+			"records": {
+				Type: schema.TypeList,
+				//TODO: add `MinItems: 1`` and `Required: true` once we remove the `record` attribute


minor MinItems: 1 is implied by the fact that this would be required, so we only actually need Required: true :)

Ah, good to know. I updated the TODO

tombuildsstuff · 2018-05-24T08:39:43Z

azurerm/resource_arm_dns_ns_record.go

+	var records []dns.NsRecord
+
+	//TODO: remove this once we remove the `record` attribute
+	if d.HasChange("record") {


IMO we should flip this around to check for records then record (as the else statement) since records is now the recommended field

Works for me. Done

tombuildsstuff · 2018-05-24T08:40:17Z

azurerm/resource_arm_dns_ns_record_migration.go

+	"github.com/hashicorp/terraform/terraform"
+)
+
+func resourceArmDNSNsMigrateState(


(as above) I think we can remove this, since this should be handled by a refresh which happens during a plan/apply?

tombuildsstuff · 2018-05-24T08:43:43Z

azurerm/resource_arm_dns_ns_record.go

@@ -33,9 +35,21 @@ func resourceArmDnsNsRecord() *schema.Resource {
 				Required: true,
 			},

+			"records": {


can we also update the documentation for this field? this is hosted in the website/docs/r/dns_ns_record.html.markdown file

Done. I added a warning that one or the other is required, but I'm open to changing the verbiage.

katbyte

Hello @phekmat,

Thank you for the changes, this LGTM except that one of the tests is failing:

=== RUN   TestAccAzureRMDnsNsRecord_withTags
--- FAIL: TestAccAzureRMDnsNsRecord_withTags (105.96s)
    testing.go:513: Step 1 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: azurerm_dns_ns_record.test
          records.0: "ns2.contoso.com" => "ns1.contoso.com"
          records.1: "ns1.contoso.com" => "ns2.contoso.com"

This looks to be a ordering issue, maybe a set is required after all?

katbyte · 2018-06-04T19:46:32Z

azurerm/resource_arm_dns_ns_record.go

@@ -18,6 +18,8 @@ func resourceArmDnsNsRecord() *schema.Resource {
 		Importer: &schema.ResourceImporter{
 			State: schema.ImportStatePassthrough,
 		},
+		// MigrateState:  resourceArmDNSNsMigrateState,


Could we remove these comments?

phekmat · 2018-06-04T20:45:03Z

@katbyte That's actually a bug I introduced with the last commit in inverting the if condition in expandAzureRmDnsNsRecords. I'll fix that and re-run the acceptance tests locally to verify

- Replace and deprecate the `azurerm_dns_ns_record.record` objects with a single `records` array to simplify working with DNS zone resources - Duplicate tests to have coverage for the deprecated attribute - Add test for migrating from `record` attributes to `records` - Add TODOs to remove code once the deprecated attribute is removed - Update docs for the new attribute

phekmat · 2018-06-04T21:32:57Z

I've fixed the issue causing the test failure (though I think it can be simplified; I'm not 100% sure). I've also added a test for changing from record to records.

katbyte

@phekmat,

Thank you for the changes, this LGTM now 👍

katbyte · 2018-06-06T01:24:24Z

Tests pass:

ghost · 2020-03-31T14:52:01Z

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!

tombuildsstuff added enhancement service/dns labels Mar 22, 2018

phekmat force-pushed the master branch from d3f42af to 1d58d2b Compare April 3, 2018 17:23

tombuildsstuff assigned katbyte May 18, 2018

katbyte requested changes May 19, 2018

View reviewed changes

tombuildsstuff added this to the Soon milestone May 24, 2018

tombuildsstuff suggested changes May 24, 2018

View reviewed changes

tombuildsstuff added the waiting-response label May 24, 2018

phekmat force-pushed the master branch from eb9cd36 to 1e7773d Compare June 1, 2018 23:03

katbyte requested changes Jun 4, 2018

View reviewed changes

phekmat force-pushed the master branch from 1e7773d to 14be304 Compare June 4, 2018 21:30

katbyte approved these changes Jun 6, 2018

View reviewed changes

katbyte merged commit eac118c into hashicorp:master Jun 6, 2018

katbyte modified the milestones: Soon, 1.7.0 Jun 6, 2018

katbyte removed the waiting-response label Jun 6, 2018

katbyte added a commit that referenced this pull request Jun 6, 2018

Update CHANGELOG.md to include #991

57003c8

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace DNS NS record objects with records array #991

Replace DNS NS record objects with records array #991

phekmat commented Mar 16, 2018

katbyte left a comment

katbyte May 19, 2018

katbyte May 19, 2018

katbyte May 19, 2018

tombuildsstuff left a comment

tombuildsstuff Apr 3, 2018

phekmat Jun 1, 2018

tombuildsstuff Apr 3, 2018

phekmat Jun 1, 2018

tombuildsstuff May 24, 2018

phekmat Jun 1, 2018

tombuildsstuff May 24, 2018

phekmat Jun 1, 2018

tombuildsstuff May 24, 2018

phekmat Jun 1, 2018

tombuildsstuff May 24, 2018

phekmat Jun 1, 2018

katbyte left a comment

katbyte Jun 4, 2018

phekmat Jun 4, 2018

phekmat commented Jun 4, 2018

phekmat commented Jun 4, 2018

katbyte left a comment

katbyte commented Jun 6, 2018

ghost commented Mar 31, 2020

Replace DNS NS record objects with records array #991

Replace DNS NS record objects with records array #991

Conversation

phekmat commented Mar 16, 2018

katbyte 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

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

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phekmat commented Jun 4, 2018

phekmat commented Jun 4, 2018

katbyte left a comment

Choose a reason for hiding this comment

katbyte commented Jun 6, 2018

ghost commented Mar 31, 2020