Resource arm dns cname record #182

sebastus · 2017-07-20T23:21:38Z

No description provided.

tombuildsstuff · 2017-07-21T13:04:47Z

Tests pass:

$ acctests azurerm TestAccAzureRMDnsCNameRecord_
=== RUN   TestAccAzureRMDnsCNameRecord_importBasic
--- PASS: TestAccAzureRMDnsCNameRecord_importBasic (101.52s)
=== RUN   TestAccAzureRMDnsCNameRecord_basic
--- PASS: TestAccAzureRMDnsCNameRecord_basic (106.86s)
=== RUN   TestAccAzureRMDnsCNameRecord_subdomain
--- PASS: TestAccAzureRMDnsCNameRecord_subdomain (88.39s)
=== RUN   TestAccAzureRMDnsCNameRecord_updateRecords
--- PASS: TestAccAzureRMDnsCNameRecord_updateRecords (104.18s)
=== RUN   TestAccAzureRMDnsCNameRecord_withTags
--- PASS: TestAccAzureRMDnsCNameRecord_withTags (111.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	512.600s

tombuildsstuff

Hey @sebastus

Thanks for this PR - I've taken a look and left some a few minor comments in-line but this generally looks good :)

Thanks!

tombuildsstuff · 2017-07-21T19:26:39Z

azurerm/resource_arm_dns_cname_record.go

+	d.Set("resource_group_name", resGroup)
+	d.Set("zone_name", zoneName)
+	d.Set("ttl", *result.TTL)
+	d.Set("etag", *result.Etag)


we should be able to remove the * for these two sets as d.Set() should take care of that for us automatically

tombuildsstuff · 2017-07-21T19:47:06Z

azurerm/resource_arm_dns_cname_record.go

-	if !createResponse.IsSuccessful() {
-		return fmt.Errorf("Error creating DNS CName Record: %s", createResponse.Error)
+	props := dns.RecordSetProperties{
+		Metadata:    metadata,


from what I can see, these are the same thing, but just to confirm: is metadata the same thing as tags? The API documentation doesn't make it clear - but we currently submit these as tags?

In the portal and in the sdk that property is called metadata, but it's otherwise identical to tags on other resources. and yes, in the tf file they are tags. I'll add a test to cover it.

test was already present.

tombuildsstuff · 2017-07-21T19:48:38Z

azurerm/resource_arm_dns_cname_record.go

+	d.Set("zone_name", zoneName)
+	d.Set("ttl", *result.TTL)
+	d.Set("etag", *result.Etag)
+	d.Set("record", cName)


can we check to see that result.RecordSetProperties.CnameRecord isn't nil here - unfortunately swagger/server-side changes could lead to Terraform crashing if we don't check - so it'd be good to change this too:

if props := result.RecordSetProperties; props != nil { if record := props.CnameRecord; record != nil { d.Set("record", record.Cname) } }

(again, we shouldn't need the * here because d.Set will handle that automatically)

tombuildsstuff · 2017-07-21T19:49:50Z

azurerm/resource_arm_dns_cname_record.go


-	flattenAndSetTags(d, &resp.Tags)
+	flattenAndSetTags(d, result.Metadata)


(same question about Metadata+Tags)

same answer.

I guess the question back to you is: do you prefer the provider match the nomenclature in the tf file or that in the sdk?

do you prefer the provider match the nomenclature in the tf file or that in the sdk?

so we should be aiming for consistency with the schema (i.e. what's used in the *.tf file) - rather than renaming this to match the SDK; providing the field we send to/from the API is actually tags (albeit renamed), then that's fine :) i.e. users should be able to upgrade from resources made with the Riviera SDK to resources made with the Azure SDK without any code changes :)

(from what I can see, if this is tags then I'd consider this inconsistency some design feedback for the Azure API's, as the rest of the Azure API's refer to this field as tags)

tombuildsstuff · 2017-07-21T19:50:40Z

azurerm/resource_arm_dns_cname_record.go

@@ -54,112 +54,112 @@ func resourceArmDnsCNameRecord() *schema.Resource {
 				Required: true,
 			},

+			"etag": {


can we add this to the documentation too?

Tom - I'm new here so still learning. eTag can be left out - I'll remove it.

tombuildsstuff

Hey @sebastus

Thanks for pushing those updates - I've taken another look and left a couple of comments but this is looking good :)

Thanks!

tombuildsstuff · 2017-07-24T14:08:15Z

azurerm/resource_arm_dns_cname_record.go

+	props := dns.RecordSetProperties{
+		Metadata:    metadata,
+		TTL:         &ttl,
+		CnameRecord: &cnameRecord,


minor: can we combine this into the properties object below?

tombuildsstuff · 2017-07-24T14:09:06Z

azurerm/resource_arm_dns_cname_record.go

-	readResponse, err := readRequest.Execute()
+	//last parameter is set to empty to allow updates to records after creation
+	// (per SDK, set it to '*' to prevent updates, all other values are ignored)
+	resp, err := dnsClient.CreateOrUpdate(resGroup, zoneName, name, dns.CNAME, parameters, "", "")


what's the penultimate argument for here? in general we'd tend to split these out into assigned variables if there's multiple

i.e.

foo := "bar" bar := "foo" ...CreateOrUpdate(..., foo, bar)

tombuildsstuff · 2017-07-24T14:09:54Z

azurerm/resource_arm_dns_cname_record.go

+
+	resp, error := dnsClient.Delete(resGroup, zoneName, name, dns.CNAME, "")
+	if resp.StatusCode != http.StatusOK {
+		return fmt.Errorf("Error deleting DNS CNAME Record %s: %s", name, error)


can we make the second argument %+v to output the full error here?

tombuildsstuff · 2017-07-24T14:10:22Z

azurerm/resource_arm_dns_cname_record_test.go

 		if err != nil {
-			return fmt.Errorf("Bad: GetCNAMERecordSet: %s", err)
+			return nil


I think we should be checking for the 404 above this, and then returning the error if it's present - rather than hiding this?

…e-sdk-for-go

tombuildsstuff

Hey @sebastus

Thanks for this contribution - due to changes within Azure detailed in #192 in order for us to merge this change in I've push a commit which contains fixes for the outstanding comments.

Your contribution is still included (and still credited to you), with the appropriate modifications. Feel free to ask about any of the changes.

Tests pass:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMDnsC
=== RUN   TestAccAzureRMDnsCNameRecord_importBasic
--- PASS: TestAccAzureRMDnsCNameRecord_importBasic (71.30s)
=== RUN   TestAccAzureRMDnsCNameRecord_importWithTags
--- PASS: TestAccAzureRMDnsCNameRecord_importWithTags (80.29s)
=== RUN   TestAccAzureRMDnsCNameRecord_basic
--- PASS: TestAccAzureRMDnsCNameRecord_basic (70.55s)
=== RUN   TestAccAzureRMDnsCNameRecord_subdomain
--- PASS: TestAccAzureRMDnsCNameRecord_subdomain (86.35s)
=== RUN   TestAccAzureRMDnsCNameRecord_updateRecords
--- PASS: TestAccAzureRMDnsCNameRecord_updateRecords (98.04s)
=== RUN   TestAccAzureRMDnsCNameRecord_withTags
--- PASS: TestAccAzureRMDnsCNameRecord_withTags (94.55s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	501.101s

Thanks!

ghost · 2020-04-01T15:22:37Z

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!

sebastus mentioned this pull request Jul 21, 2017

Remove Riviera from resource_arm_dns_cname_record azdevops/terraform#49

Closed

tombuildsstuff reviewed Jul 21, 2017

View reviewed changes

tombuildsstuff added the waiting-response label Jul 21, 2017

tombuildsstuff reviewed Jul 24, 2017

View reviewed changes

tombuildsstuff mentioned this pull request Jul 24, 2017

Upgrade to use API version 2016-04-01 #192

Closed

9 tasks

tombuildsstuff self-assigned this Jul 26, 2017

sebastus and others added 4 commits July 26, 2017 13:03

resource_arm_dns_cname_record: switch dependency from riviera to azur…

87c9528

…e-sdk-for-go

resource_arm_dns_cname_record: switch dependency from riviera to azur…

01208e4

…e-sdk-for-go

response to PR feedback

e9c352c

Refactoring + fixing PR comments

5f08f11

tombuildsstuff approved these changes Jul 26, 2017

View reviewed changes

Linting

a949ada

tombuildsstuff removed the waiting-response label Jul 26, 2017

tombuildsstuff merged commit 6026a72 into hashicorp:master Jul 26, 2017

sebastus deleted the resource_arm_dns_cname_record branch September 6, 2017 21:37

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource arm dns cname record #182

Resource arm dns cname record #182

sebastus commented Jul 20, 2017

tombuildsstuff commented Jul 21, 2017

tombuildsstuff left a comment

tombuildsstuff Jul 21, 2017

sebastus Jul 22, 2017

tombuildsstuff Jul 21, 2017

sebastus Jul 22, 2017

sebastus Jul 22, 2017

tombuildsstuff Jul 21, 2017

sebastus Jul 22, 2017

tombuildsstuff Jul 21, 2017

sebastus Jul 22, 2017

sebastus Jul 22, 2017

tombuildsstuff Jul 22, 2017

tombuildsstuff Jul 21, 2017

sebastus Jul 22, 2017

tombuildsstuff left a comment

tombuildsstuff Jul 24, 2017

tombuildsstuff Jul 24, 2017

tombuildsstuff Jul 24, 2017

tombuildsstuff Jul 24, 2017

tombuildsstuff left a comment

ghost commented Apr 1, 2020


		flattenAndSetTags(d, &resp.Tags)
		flattenAndSetTags(d, result.Metadata)

Resource arm dns cname record #182

Resource arm dns cname record #182

Conversation

sebastus commented Jul 20, 2017

tombuildsstuff commented Jul 21, 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

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

tombuildsstuff left a comment

Choose a reason for hiding this comment

ghost commented Apr 1, 2020