Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Terraform] [WIP] autogenerate resource_dns_managed_zone #697

Merged
merged 3 commits into from
Nov 16, 2018

Conversation

drebes
Copy link
Member

@drebes drebes commented Nov 7, 2018

This is my first attempt on generating resource_dns_managed_zone using magic-modules.

As the module is generating only against the v1 version of the API for now, which does not support update/patch on the resource, the autogenerated tests are not testing update. I also didn't include support for RecordSets yet, as that will be more complex and I wanted to start simple.

I'm still trying to figure out how testing works, for example, how can my autogenerated tests be against a zone like "tf-acctest-%s.hashicorptest.com.", but not have that domain showing up in the generated docs.


[all]

Add labels and updates to DNS managed zone

[terraform]

Start autogenerating resource_dns_managed_zone

[terraform-beta]

[ansible]

[inspec]

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks so much for doing this!

We probably want to keep resource_dns_managed_zone_test, or at the very least remove the basic test and leave the update test, or something like that- since the autogenerated tests don't test update yet. The v1 API actually does have support for update; it's just not documented. I filed a bug internally about that.

Also, adding fields in api.yaml will also update the non-terraform providers. Can you make sure they have descriptions in the PR description so that their generated PRs will have names? Once that's done I'll go ahead and trigger the magician (in the future it should trigger automatically on your PRs, but it doesn't for the first one from an individual).

products/compute/api.yaml Outdated Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
products/dns/terraform.yaml Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
templates/terraform/examples/dns_managed_zone_basic.tf.erb Outdated Show resolved Hide resolved
@drebes drebes mentioned this pull request Nov 7, 2018
@drebes drebes force-pushed the terraform-dns branch 3 times, most recently from d67cc61 to 1d75860 Compare November 7, 2018 20:01
@drebes
Copy link
Member Author

drebes commented Nov 7, 2018

I've pushed updates with the requested changes, can you take a second look?

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll go ahead and modify the PR description so you can see what I was talking about, and then I'll start the magician run.

products/dns/api.yaml Show resolved Hide resolved
products/dns/terraform.yaml Outdated Show resolved Hide resolved
provider/terraform/tests/resource_dns_managed_zone_test.go Outdated Show resolved Hide resolved
@danawillow
Copy link
Contributor

Ah, since you added a new field you also need to add version_added into dns/ansible.yaml. Here's an example of how to do it for an individual field: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/products/compute/ansible.yaml#L114. 2.8 is the value you want.

@drebes drebes force-pushed the terraform-dns branch 2 times, most recently from c677e4a to 339c17b Compare November 7, 2018 22:24
@drebes
Copy link
Member Author

drebes commented Nov 7, 2018

Done, removed the references to PATCH but updates now are done with PUT instead.

@drebes drebes force-pushed the terraform-dns branch 4 times, most recently from 4d826ae to 3d7eb7e Compare November 8, 2018 18:11
@drebes
Copy link
Member Author

drebes commented Nov 8, 2018

I see that the generated code is now updating the resource with PUT, wouldn't it be better to do it with PATCH if the API supports it? Also, for some reason the ForceNew for name and dnsName are gone.

@danawillow
Copy link
Contributor

I don't think there's much of a difference. If you think PATCH is better, then you're welcome to add it back with a comment explaining why :)

Adding an input: true on name/dnsname should add the ForceNew back.

@modular-magician
Copy link
Collaborator

I am a robot that works on MagicModules PRs!

I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#107
depends: hashicorp/terraform-provider-google#2428
depends: modular-magician/ansible#128

- !ruby/object:Api::Type::String
name: 'dnsName'
description: |
The DNS name of this managed zone, for instance "example.com.".
required: true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked through the API and this is required for creation.

products/dns/terraform.yaml Outdated Show resolved Hide resolved
@drebes
Copy link
Member Author

drebes commented Nov 9, 2018

In the end I also reverted the previous changes so that only the description and labels get updated, through PATCH. PATCH semantics is much cleaner as the request will have only the fields that change, and I confirmed that gcloud is also PATCHing these fields on update. The generated code is also easy to follow.

@drebes
Copy link
Member Author

drebes commented Nov 9, 2018

And now the generated code passes the acceptance tests:

$ make testacc TESTARGS='-run=TestAccDnsManagedZone_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccDnsManagedZone_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-google	[no test files]
=== RUN   TestAccDnsManagedZone_basic
=== PAUSE TestAccDnsManagedZone_basic
=== RUN   TestAccDnsManagedZone_update
=== PAUSE TestAccDnsManagedZone_update
=== RUN   TestAccDnsManagedZone_importWithProject
=== PAUSE TestAccDnsManagedZone_importWithProject
=== CONT  TestAccDnsManagedZone_basic
=== CONT  TestAccDnsManagedZone_importWithProject
=== CONT  TestAccDnsManagedZone_update
--- PASS: TestAccDnsManagedZone_importWithProject (7.04s)
--- PASS: TestAccDnsManagedZone_basic (7.09s)
--- PASS: TestAccDnsManagedZone_update (9.25s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	9.289s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-google/scripts	(cached) [no tests to run]

@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 95e7e1f) have been included in your existing downstream PRs.

@danawillow
Copy link
Contributor

I just tried running the magician again and it looks like the PR has merge conflicts. Mind fixing those and letting me know once you have?

@drebes
Copy link
Member Author

drebes commented Nov 16, 2018

I just rebased, can you check if that fixes the conflicts?

@modular-magician
Copy link
Collaborator

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 124d2a2) have been included in your existing downstream PRs.

@danawillow
Copy link
Contributor

LGTM, thanks for doing this!

Tracked submodules are build/terraform-beta build/terraform build/ansible build/inspec.
@modular-magician modular-magician merged commit dd7eb9e into GoogleCloudPlatform:master Nov 16, 2018
@drebes drebes deleted the terraform-dns branch March 4, 2019 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants