add OpenStack DNS V2 RecordSet resource #14813

jrperritt · 2017-05-25T00:43:32Z

In addition, while I was testing I stumbled upon a panic condition while updating a DNS zone, so the third commit here addresses that.

jrperritt · 2017-05-25T00:48:52Z

@jtopjian for review at your convenience. I was able to use your stand-alone Designate environment to test this. It was much easier to understand than the AIO.

jtopjian

In general, this works really well. There's a variety of changes to put the finishing touches on it.

The biggest is the import stuff. Second is the TypeList.

Let me know if you have questions or need help with any of it.

jtopjian · 2017-05-25T01:22:47Z

builtin/providers/openstack/resource_openstack_dns_recordset_v2.go

+		Target:     []string{"ACTIVE"},
+		Pending:    []string{"PENDING"},
+		Refresh:    waitForDNSRecordSet(dnsClient, zoneID, n.ID),
+		Timeout:    d.Timeout(schema.TimeoutCreate),


schema.TimeoutUpdate

jtopjian · 2017-05-25T01:26:43Z

builtin/providers/openstack/resource_openstack_dns_recordset_v2_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckDNSV2RecordSetExists("openstack_dns_recordset_v2.recordset_1", &recordset),
+					resource.TestCheckResourceAttr(
+						"openstack_dns_recordset_v2.recordset_1", "description", "a record set"),


Can you add checks for the records where appropriate? Something like:

resource.TestCheckResourceAttr( "openstack_dns_recordset_v2.recordset_1", "records.0", "10.1.0.0"),

jtopjian · 2017-05-25T01:27:39Z

website/source/docs/providers/openstack/r/dns_recordset_v2.html.markdown

+  description = "An example record set"
+  ttl = 3000
+  type = "A"
+}


Can you add a record to the example?

jtopjian · 2017-05-25T01:47:33Z

website/source/docs/providers/openstack/r/dns_recordset_v2.html.markdown

+
+resource "openstack_dns_recordset_v2" "rs.example.com" {
+  zone_id = "${openstack_dns_zone_v2.example_zone.id}"
+  name = "example.com."


Can you make this www.example.com. (or rs.example.com as the name is above)? Just to clarify to the user that this is where they will want to create subdomains for their zone.

jtopjian · 2017-05-25T01:57:17Z

builtin/providers/openstack/resource_openstack_dns_recordset_v2.go

+				ForceNew: false,
+			},
+			"records": &schema.Schema{
+				Type:     schema.TypeSet,


The good news is that we can use TypeList. The DNS API is returning the records in the same order as specified. That's a rarity and it's also a better user experience since they can reference records using standard integer index (as well as be able to map/interpolate count with a series of Compute instances, each with an IP address of the records!).

The bad news is that TypeSet will need changed to TypeList. It's pretty minor, though. You should be able to iterate over it by doing d.Get("records").([]interface). Grep for TypeList in another OpenStack resource for examples and let me know if you need help.

Ah, nice, I'd forgotten there was a TypeList type.

jtopjian · 2017-05-25T02:04:57Z

website/source/docs/providers/openstack/r/dns_recordset_v2.html.markdown

+## Import
+
+This resource can be imported by specifying all three arguments, separated
+by a forward slash:


whoops. Just noticed this was carried over from the original doc I copied when I made the zone docs. This should read:

This resource can be imported by specifying the zone ID and recordset ID, separated by a forward slash.

I will take care of the zone doc.

jtopjian · 2017-05-25T02:14:27Z

website/source/docs/providers/openstack/r/dns_recordset_v2.html.markdown

+by a forward slash:
+
+```
+$ terraform import openstack_dns_recordset_v2.recordset_1 <recordset_id>


So the idea of terraform import is to create a terraform.tfstate file that is populated by resources that do not exist in a .tf file. While import will not create the HCL, it will allow Terraform to recognize that those resources exist (so you can base other resources off of them and such).

When importing, you have to specify enough information to bootstrap Terraform to be able to look up the rest of the information. In this case, a Zone ID and RR ID is required. When doing this on the command line as it is now, the following error happens:

# terraform import openstack_dns_recordset_v2.foo 5805f911-1e86-44b1-8637-dfa73c2d3176 openstack_dns_recordset_v2.foo: Importing from ID "5805f911-1e86-44b1-8637-dfa73c2d3176"... openstack_dns_recordset_v2.foo: Import complete! Imported openstack_dns_recordset_v2 (ID: 5805f911-1e86-44b1-8637-dfa73c2d3176) openstack_dns_recordset_v2.foo: Refreshing state... (ID: 5805f911-1e86-44b1-8637-dfa73c2d3176) Error importing: 1 error(s) occurred: * openstack_dns_recordset_v2.foo (import id: 5805f911-1e86-44b1-8637-dfa73c2d3176): 1 error(s) occurred: * import openstack_dns_recordset_v2.foo result: 5805f911-1e86-44b1-8637-dfa73c2d3176: import openstack_dns_recordset_v2.foo (id: 5805f911-1e86-44b1-8637-dfa73c2d3176): Terraform detected a resource with this ID doesn't exist. Please verify the ID is correct. You cannot import non-existent resources using Terraform import.

So we need to use both Zone ID and RR ID. The catch is that the command-line only accepts one argument. So that's where the "separated by a forward slash" comes in.

See the resource_openstack_compute_floatingip_associate_v2.go resource for how to handle this. That resource is an edge case because all resource attributes are shoved in the faux ID. Here we'll just do the zone and rr.

Out of all the changes requested, this is the most important as it involves what the ID of the resource will be. It won't be fun to change that later.

If you're wondering why the _importBasic acc test passed, it's because all bootstrap information was available in the _basic test and it only checks that all resource attributes are populated correctly in the Read.

Let me know if you need a hand with this.

edit: resource_openstack_compute_volume_attach_v2.go is another resource to use as an example. I just remembered resource_openstack_compute_floatingip_associate_v2.go now mixes the compute and networking API, which was done afterwards, and might make some of the inline comments confusing.

In this case, a Zone ID and RR ID is required

Ah, OK; I'd suspected that, but didn't see it anywhere else (except of course it's clearly in the file you mentioned). I should have just tried the command-line import command. Will fix.

jtopjian · 2017-05-25T02:18:28Z

I was able to use your stand-alone Designate environment to test this. It was much easier to understand than the AIO.

That's awesome - I'm glad you got it working.

jtopjian · 2017-05-25T04:16:54Z

builtin/providers/openstack/resource_openstack_dns_recordset_v2.go


 	d.Set("name", n.Name)
 	d.Set("description", n.Description)
 	d.Set("ttl", n.TTL)
 	d.Set("type", n.Type)
 	d.Set("records", n.Records)
 	d.Set("region", GetRegion(d))
-	d.Set("zone_id", n.ZoneID)


@jrperritt d.Set("zone_id", zoneID)

👍 I was just narrowing down the search space for things that were causing errors. I found it in the test (I hadn't added parseDNSV2RecordSetID). Running the tests now to verify they pass.

ah - gotcha. Everything else looks good. You're on a mission tonight :)

Let me know when you're good and I'll do one final sweep.

And as I just found out (and I'm sure you already knew), the import verification condition will fail if the zone_id isn't set.

=== RUN TestAccDNSV2RecordSet_importBasic --- FAIL: TestAccDNSV2RecordSet_importBasic (192.40s) testing.go:280: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected. (map[string]string) { } (map[string]string) (len=1) { (string) (len=7) "zone_id": (string) (len=36) "68d14ad5-b21c-4a2a-9208-ca01dfa3677c" }

Can you see anything obvious that may still be wrong with the importBasic test setup?

I fixed it locally by adding what I mentioned above and it passed twice for me.

Maybe I just didn't pull down the most recent changes I made. OK, I think I'm done.

Yep, confirmed that I didn't fetch the most recent changes I made. All tests passing for me now, too.

jrperritt · 2017-05-25T04:34:22Z

@jtopjian I think I've addressed all the feedback. Thank you for the review and the help.

jtopjian · 2017-05-25T04:46:51Z

No problem at all. Nice work on this and thank you for making all of the changes.

I just pulled a clean branch and running the tests one last time.

jtopjian · 2017-05-25T04:58:10Z

Good to go! Thank you for adding this!

==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/25 04:42:42 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/openstack -v -run=TestAccDNSV2RecordSet -timeout 120m
=== RUN   TestAccDNSV2RecordSet_importBasic
--- PASS: TestAccDNSV2RecordSet_importBasic (201.94s)
=== RUN   TestAccDNSV2RecordSet_basic
--- PASS: TestAccDNSV2RecordSet_basic (252.42s)
=== RUN   TestAccDNSV2RecordSet_readTTL
--- PASS: TestAccDNSV2RecordSet_readTTL (201.49s)
=== RUN   TestAccDNSV2RecordSet_timeout
--- PASS: TestAccDNSV2RecordSet_timeout (201.34s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/openstack      857.207s

ghost · 2020-04-12T01:56:45Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jrperritt added 3 commits May 24, 2017 13:09

vendor gophercloud dns v2 recordset deps

0474aae

openstack dns v2 recordset resource

64ccb65

fix type assertion panic condition

b6d43a7

jtopjian suggested changes May 25, 2017

View reviewed changes

jtopjian added new-resource labels May 25, 2017

jrperritt force-pushed the 14735 branch 3 times, most recently from 042ff48 to 92d859d Compare May 25, 2017 04:15

jtopjian reviewed May 25, 2017

View reviewed changes

address pr review feedback

89822d8

jrperritt force-pushed the 14735 branch from 92d859d to 89822d8 Compare May 25, 2017 04:21

jtopjian approved these changes May 25, 2017

View reviewed changes

jtopjian merged commit e1c8d76 into hashicorp:master May 25, 2017

jtopjian mentioned this pull request May 25, 2017

provider/openstack: Add DNS RecordSet V2 resource #14735

Closed

jrperritt deleted the 14735 branch May 25, 2017 12:08

s-urbaniak mentioned this pull request Jul 4, 2017

openstack: DNS for Nova network coreos/tectonic-installer#9

Closed

ghost locked and limited conversation to collaborators Apr 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add OpenStack DNS V2 RecordSet resource #14813

add OpenStack DNS V2 RecordSet resource #14813

jrperritt commented May 25, 2017

jrperritt commented May 25, 2017 •

edited

Loading

jtopjian left a comment

jtopjian May 25, 2017

jrperritt May 25, 2017

jtopjian May 25, 2017

jrperritt May 25, 2017

jtopjian May 25, 2017

jrperritt May 25, 2017

jtopjian May 25, 2017

jrperritt May 25, 2017

jtopjian May 25, 2017

jrperritt May 25, 2017 •

edited

Loading

jtopjian May 25, 2017

jrperritt May 25, 2017

jtopjian May 25, 2017 •

edited

Loading

jrperritt May 25, 2017

jtopjian commented May 25, 2017

jtopjian May 25, 2017

jrperritt May 25, 2017

jtopjian May 25, 2017

jrperritt May 25, 2017

jrperritt May 25, 2017

jtopjian May 25, 2017

jrperritt May 25, 2017

jrperritt May 25, 2017

jrperritt commented May 25, 2017

jtopjian commented May 25, 2017

jtopjian commented May 25, 2017

ghost commented Apr 12, 2020

add OpenStack DNS V2 RecordSet resource #14813

add OpenStack DNS V2 RecordSet resource #14813

Conversation

jrperritt commented May 25, 2017

jrperritt commented May 25, 2017 • edited Loading

jtopjian 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

jrperritt May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtopjian May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtopjian commented May 25, 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

jrperritt commented May 25, 2017

jtopjian commented May 25, 2017

jtopjian commented May 25, 2017

ghost commented Apr 12, 2020

jrperritt commented May 25, 2017 •

edited

Loading

jrperritt May 25, 2017 •

edited

Loading

jtopjian May 25, 2017 •

edited

Loading