[MS] Adding DNS PTR Record #141

echuvyrov · 2017-06-28T02:31:39Z

Enabling the creation of DNS PTR Record

whiskeyjay · 2017-06-28T06:22:44Z

azurerm/resource_arm_dns_ptr_record.go

+			"records": &schema.Schema{
+				Type:     schema.TypeSet,
+				Required: true,
+				Elem:     &schema.Schema{Type: schema.TypeString},


Can this contain zero records?

It cannot, it would be invalid

whiskeyjay · 2017-06-28T06:27:19Z

azurerm/resource_arm_dns_ptr_record.go

+	}
+}
+
+func resourceArmDnsPtrRecordCreate(d *schema.ResourceData, meta interface{}) error {


resourceArmDnsPtrRecordCreateOrUpdate

whiskeyjay · 2017-06-28T06:28:14Z

azurerm/resource_arm_dns_ptr_record.go

+		RecordSetProperties: &props,
+	}
+
+	_, err := dnsClient.CreateOrUpdate(resGroup, zoneName, name, dns.PTR, parameters, "", "")


It is not obvious what the last two parameters are and why are they empty strings.

good catch, changed second-to-last parameter to properly be the eTag of the record and added comments explaining why the last parameter is empty

whiskeyjay · 2017-06-28T06:34:05Z

azurerm/resource_arm_dns_ptr_record.go

+		return err
+	}
+
+	rec, err := dnsClient.Get(resGroup, zoneName, name, dns.PTR)


I believe the dnsClient.CreateOrUpdate method's "response" return value already contains the resource ID, there is no need to call Get just to retrieve the ID value.

rec, err := dnsClient.CreateOrUpdate(...) if err != nil { return err } d.SetId(*rec.ID)

good catch, changed

tombuildsstuff

Hey @echuvyrov

Thanks for this PR - I've taken a look and left some comments in-line - but this is looking good!

Thanks!

tombuildsstuff · 2017-06-28T07:37:42Z

azurerm/provider.go

@@ -124,6 +124,8 @@ func Provider() terraform.ResourceProvider {

 			"azurerm_application_insights": resourceArmApplicationInsights(),

+			//PTR record uses Azure Go SDK


minor: can we move this sorted alphabetically under the Azure SDK for Go comment?

tombuildsstuff · 2017-06-28T07:46:02Z

azurerm/resource_arm_dns_ptr_record.go

+
+	metadata := expandTags(tags)
+
+	recordStrings := d.Get("records").(*schema.Set).List()


we'd generally tend to pull this out into a separate function - could we update this to match?

tombuildsstuff · 2017-06-28T07:46:58Z

azurerm/resource_arm_dns_ptr_record.go

+
+	resp, err := dnsClient.Get(resGroup, zoneName, name, dns.PTR)
+	if err != nil {
+		return fmt.Errorf("Error reading DNS PTR record %s: %s", name, err)


this last one should be %+v as it's an error

changed, thanks for catching that!

tombuildsstuff · 2017-06-28T07:47:25Z

azurerm/resource_arm_dns_ptr_record.go

+	d.Set("zonename", zoneName)
+	d.Set("ttl", resp.TTL)
+
+	if resp.PtrRecords != nil {


we'd generally pull this out into a Flatten method - could we update this to match?

tombuildsstuff · 2017-06-28T07:48:35Z

azurerm/resource_arm_dns_ptr_record_test.go

+
+func TestAccAzureRMDnsPtrRecord_basic(t *testing.T) {
+	ri := acctest.RandInt()
+	config := fmt.Sprintf(testAccAzureRMDnsPtrRecord_basic, ri, ri, ri)


could we switch this to match the newer resources by making this a function which returns a formatted string?

tombuildsstuff · 2017-06-28T07:48:43Z

azurerm/resource_arm_dns_ptr_record_test.go

+func TestAccAzureRMDnsPtrRecord_updateRecords(t *testing.T) {
+	ri := acctest.RandInt()
+	preConfig := fmt.Sprintf(testAccAzureRMDnsPtrRecord_basic, ri, ri, ri)
+	postConfig := fmt.Sprintf(testAccAzureRMDnsPtrRecord_updateRecords, ri, ri, ri)


(same here)

tombuildsstuff · 2017-06-28T07:48:46Z

azurerm/resource_arm_dns_ptr_record_test.go

+func TestAccAzureRMDnsPtrRecord_withTags(t *testing.T) {
+	ri := acctest.RandInt()
+	preConfig := fmt.Sprintf(testAccAzureRMDnsPtrRecord_withTags, ri, ri, ri)
+	postConfig := fmt.Sprintf(testAccAzureRMDnsPtrRecord_withTagsUpdate, ri, ri, ri)


(same here)

tombuildsstuff · 2017-06-28T07:49:01Z

azurerm/resource_arm_dns_ptr_record_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMDnsPtrRecordExists("azurerm_dns_ptr_record.test"),
+					resource.TestCheckResourceAttr(
+						"azurerm_dns_ptr_record.test", "tags.%", "2"),


do we want to check the ordering of the values here?

apologies, I'd closed the wrong comment box and had meant to put this here. on reflection this isn't such a big deal, given we're asserting the number of entries in place.

tombuildsstuff · 2017-06-28T07:49:19Z

azurerm/resource_arm_dns_ptr_record_test.go

+		conn := testAccProvider.Meta().(*ArmClient).dnsClient
+		resp, err := conn.Get(resourceGroup, zoneName, ptrName, dns.PTR)
+		if err != nil {
+			return fmt.Errorf("Bad: Get PTR RecordSet: %s", err)


this needs to be %+v

… conform with newer approach

tombuildsstuff · 2017-06-28T08:07:21Z

website/docs/r/dns_ptr_record.html.markdown

+  Create a DNS PTR Record.
+---
+
+# azurerm\_dns\_a\_record


echuvyrov · 2017-06-29T22:40:37Z

@tombuildsstuff @whiskeyjay made the changes you guys suggested, thanks! The only one I wasn't sure about was about order checking in the tests - I added tag checking, lmk if you meant something else.

tombuildsstuff

Hey @echuvyrov

Thanks for pushing those updates - I've pushed a commit to resolve a couple of minor issues I noticed below (I hope you don't mind) - and this now LGTM :)

Tests pass:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMDns
=== RUN   TestAccAzureRMDnsARecord_importBasic
--- PASS: TestAccAzureRMDnsARecord_importBasic (106.92s)
=== RUN   TestAccAzureRMDnsAAAARecord_importBasic
--- PASS: TestAccAzureRMDnsAAAARecord_importBasic (92.47s)
=== RUN   TestAccAzureRMDnsCNameRecord_importBasic
--- PASS: TestAccAzureRMDnsCNameRecord_importBasic (92.02s)
=== RUN   TestAccAzureRMDnsMxRecord_importBasic
--- PASS: TestAccAzureRMDnsMxRecord_importBasic (91.66s)
=== RUN   TestAccAzureRMDnsNsRecord_importBasic
--- PASS: TestAccAzureRMDnsNsRecord_importBasic (92.20s)
=== RUN   TestAccAzureRMDnsPtrRecord_importBasic
--- PASS: TestAccAzureRMDnsPtrRecord_importBasic (74.45s)
=== RUN   TestAccAzureRMDnsSrvRecord_importBasic
--- PASS: TestAccAzureRMDnsSrvRecord_importBasic (94.21s)
=== RUN   TestAccAzureRMDnsTxtRecord_importBasic
--- PASS: TestAccAzureRMDnsTxtRecord_importBasic (93.43s)
=== RUN   TestAccAzureRMDnsZone_importBasic
--- PASS: TestAccAzureRMDnsZone_importBasic (89.80s)
=== RUN   TestAccAzureRMDnsARecord_basic
--- PASS: TestAccAzureRMDnsARecord_basic (88.74s)
=== RUN   TestAccAzureRMDnsARecord_updateRecords
--- PASS: TestAccAzureRMDnsARecord_updateRecords (91.76s)
=== RUN   TestAccAzureRMDnsARecord_withTags
--- PASS: TestAccAzureRMDnsARecord_withTags (103.81s)
=== RUN   TestAccAzureRMDnsAAAARecord_basic
--- PASS: TestAccAzureRMDnsAAAARecord_basic (78.32s)
=== RUN   TestAccAzureRMDnsAAAARecord_updateRecords
--- PASS: TestAccAzureRMDnsAAAARecord_updateRecords (106.47s)
=== RUN   TestAccAzureRMDnsAAAARecord_withTags
--- PASS: TestAccAzureRMDnsAAAARecord_withTags (110.22s)
=== RUN   TestAccAzureRMDnsCNameRecord_basic
--- PASS: TestAccAzureRMDnsCNameRecord_basic (89.36s)
=== RUN   TestAccAzureRMDnsCNameRecord_subdomain
--- PASS: TestAccAzureRMDnsCNameRecord_subdomain (95.82s)
=== RUN   TestAccAzureRMDnsCNameRecord_updateRecords
--- PASS: TestAccAzureRMDnsCNameRecord_updateRecords (94.08s)
=== RUN   TestAccAzureRMDnsCNameRecord_withTags
--- PASS: TestAccAzureRMDnsCNameRecord_withTags (108.93s)
=== RUN   TestAccAzureRMDnsMxRecord_basic
--- PASS: TestAccAzureRMDnsMxRecord_basic (74.31s)
=== RUN   TestAccAzureRMDnsMxRecord_updateRecords
--- PASS: TestAccAzureRMDnsMxRecord_updateRecords (106.70s)
=== RUN   TestAccAzureRMDnsMxRecord_withTags
--- PASS: TestAccAzureRMDnsMxRecord_withTags (136.45s)
=== RUN   TestAccAzureRMDnsNsRecord_basic
--- PASS: TestAccAzureRMDnsNsRecord_basic (91.50s)
=== RUN   TestAccAzureRMDnsNsRecord_updateRecords
--- PASS: TestAccAzureRMDnsNsRecord_updateRecords (145.28s)
=== RUN   TestAccAzureRMDnsNsRecord_withTags
--- PASS: TestAccAzureRMDnsNsRecord_withTags (106.33s)
=== RUN   TestAccAzureRMDnsPtrRecord_basic
--- PASS: TestAccAzureRMDnsPtrRecord_basic (82.45s)
=== RUN   TestAccAzureRMDnsPtrRecord_updateRecords
--- PASS: TestAccAzureRMDnsPtrRecord_updateRecords (125.35s)
=== RUN   TestAccAzureRMDnsPtrRecord_withTags
--- PASS: TestAccAzureRMDnsPtrRecord_withTags (89.33s)
=== RUN   TestAccAzureRMDnsSrvRecord_basic
--- PASS: TestAccAzureRMDnsSrvRecord_basic (70.97s)
=== RUN   TestAccAzureRMDnsSrvRecord_updateRecords
--- PASS: TestAccAzureRMDnsSrvRecord_updateRecords (103.76s)
=== RUN   TestAccAzureRMDnsSrvRecord_withTags
--- PASS: TestAccAzureRMDnsSrvRecord_withTags (89.54s)
=== RUN   TestAccAzureRMDnsTxtRecord_basic
--- PASS: TestAccAzureRMDnsTxtRecord_basic (73.52s)
=== RUN   TestAccAzureRMDnsTxtRecord_updateRecords
--- PASS: TestAccAzureRMDnsTxtRecord_updateRecords (101.40s)
=== RUN   TestAccAzureRMDnsTxtRecord_withTags
--- PASS: TestAccAzureRMDnsTxtRecord_withTags (104.44s)
=== RUN   TestAccAzureRMDnsZone_basic
--- PASS: TestAccAzureRMDnsZone_basic (85.98s)
=== RUN   TestAccAzureRMDnsZone_withTags
--- PASS: TestAccAzureRMDnsZone_withTags (99.68s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	3481.650s

Thanks!

tombuildsstuff · 2017-06-30T12:41:22Z

azurerm/resource_arm_dns_ptr_record.go

+		return err
+	}
+
+	log.Printf("[INFO] here is actual %v", id)


can we remove this comment?

tombuildsstuff · 2017-06-30T12:46:07Z

azurerm/resource_arm_dns_ptr_record_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMDnsPtrRecordExists("azurerm_dns_ptr_record.test"),
+					resource.TestCheckResourceAttr(
+						"azurerm_dns_ptr_record.test", "tags.%", "2"),


apologies, I'd closed the wrong comment box and had meant to put this here. on reflection this isn't such a big deal, given we're asserting the number of entries in place.

tombuildsstuff · 2017-06-30T16:37:44Z

website/docs/r/dns_ptr_record.html.markdown

+
+The following attributes are exported:
+
+* `id` - The DNS PTR Record ID.


we should expose etag here too

[MS] Adding DNS PTR Record

ghost · 2020-04-01T15:35:33Z

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!

echuvyrov added 2 commits June 27, 2017 19:23

DNS PTR Reocrd - Initial Work

6a1bb2d

DNS PTR Record - Initial PR

f37e300

whiskeyjay reviewed Jun 28, 2017

View reviewed changes

tombuildsstuff requested changes Jun 28, 2017

View reviewed changes

Added import test, updated docs, converted test strings into funcs to…

93f38a0

… conform with newer approach

tombuildsstuff reviewed Jun 28, 2017

View reviewed changes

echuvyrov added 2 commits June 29, 2017 14:56

Made changes suggested for the PR

020e0d1

Added tag validation to tests

8653819

tombuildsstuff added 2 commits June 30, 2017 17:22

Cleaning up some unnecessary code

e16ed18

Adding in etags / updating the casing of ttl

e17b6db

tombuildsstuff approved these changes Jul 3, 2017

View reviewed changes

tombuildsstuff merged commit 4639a9e into hashicorp:master Jul 3, 2017

tombuildsstuff added a commit that referenced this pull request Jul 3, 2017

Updating to include #141

c5967a5

tombuildsstuff added a commit that referenced this pull request Jul 4, 2017

Merge pull request #141 from echuvyrov/dnsptrrecord

bd01190

[MS] Adding DNS PTR Record

tombuildsstuff added a commit that referenced this pull request Jul 4, 2017

Updating to include #141

65dbcce

sebastus mentioned this pull request Jul 12, 2017

catch up with the team. (#1) #165

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020

		@@ -124,6 +124,8 @@ func Provider() terraform.ResourceProvider {

		"azurerm_application_insights": resourceArmApplicationInsights(),

		//PTR record uses Azure Go SDK


		metadata := expandTags(tags)

		recordStrings := d.Get("records").(*schema.Set).List()


		The following attributes are exported:

		* `id` - The DNS PTR Record ID.

[MS] Adding DNS PTR Record #141

[MS] Adding DNS PTR Record #141

Conversation

echuvyrov commented Jun 28, 2017

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

echuvyrov commented Jun 29, 2017

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

ghost commented Apr 1, 2020