Add app engine domain mappings resource #2216

megan07 · 2019-08-20T21:05:59Z

Fixes hashicorp/terraform-provider-google#3204

Add the new resource google_appengine_domain_mapping

The test creates the appropriate resource, however, I wasn't able to get the certificate provisioned. I wasn't able to get it provisioned in the console either (so it may be a permissions issue?). Not sure if that will be a requirement in the tests or not.

Release Note for Downstream PRs (will be copied)

Add the new resource `google_appengine_domain_mapping`

modular-magician · 2019-08-21T16:07:26Z

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of aec397020ca70990b179086d59c3fb093dfc2522.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1079
depends: GoogleCloudPlatform/terraform-google-conversion#175
depends: hashicorp/terraform-provider-google#4310

danawillow

The test passed (https://ci-oss.hashicorp.engineering/viewLog.html?buildId=70373&tab=buildResultsDiv&buildTypeId=GoogleCloud_ProviderGoogleCloudGoogleProject) so I wouldn't worry too much about the certificate thing.

Don't forget to add the resource to google.erb as well!

I'm also going to go ahead and add a changelog label to this PR because I don't think it'll get auto-changelogged without one

danawillow · 2019-08-21T19:47:29Z

products/appengine/api.yaml

+    description: |
+      A domain serving an App Engine application.
+    base_url: 'apps/{{project}}/domainMappings'
+    self_link: 'apps/{{project}}/domainMappings/{{domain_name}}'


Since domain_name is a TF override, I think instead you'll want to make that {{id}} and then override it in TF, or if it works, you might just be able to replace the whole string with {{name}}

danawillow · 2019-08-21T19:56:25Z

products/appengine/api.yaml

+          By default, overrides are rejected.
+        url_param_only: true
+        values:
+          - :UNSPECIFIED_DOMAIN_OVERRIDE_STRATEGY


It's probably not worth allowing people to set this value (and all the other UNSPECIFIED enum values)

danawillow · 2019-08-21T19:58:35Z

products/appengine/api.yaml

+          - !ruby/object:Api::Type::Enum
+            name: 'sslManagementType'
+            description: |
+              SSL management type for this domain. If `AUTOMATIC`, a managed certificate is automatically provisioned.


Does this mean that certificateId should be computed?

I believe so. That was one of the things I was trying to test, because as I was testing this I never actually saw certificateId be populated, but it seems like it should be. But I was getting that provisioning error, so I assumed that's why. But if this is set to MANUAL then it won't be computed.

danawillow · 2019-08-21T20:01:26Z

products/vpcaccess/api.yaml

-          - DELETING
-          - ERROR
-          - UPDATING
+          - :STATE_UNSPECIFIED


This change looks good, but not relevant to this resource. Can it be made separately?

danawillow · 2019-08-21T20:03:21Z

templates/terraform/constants/domain_mapping.erb

@@ -0,0 +1,15 @@
+// While the domain mapping is setting up the certificate id, GET returns a field
+// `pendingManagedCertificateId`, which will eventually be empty when `certificateID`


Isn't that field output-only? If so, what happens if you get rid of the diffsuppress (since those should only be needed for fields that are user-specified)?

I've removed this now, however, one concern I have, and part of the reason I wanted to test it with a generated certificate, is because I think pendingManagedCertificateId will go from computed to empty (which is fine), but I'm guessing that certificateId will go from empty to computed, in which case I'm guessing we might see a diff.

danawillow · 2019-08-21T20:06:35Z

products/appengine/terraform.yaml

+  DomainMapping: !ruby/object:Overrides::Terraform::ResourceOverride
+    id_format: "{{domain_name}}"
+    import_format: ["{{domain_name}}"]
+    examples:


Can you also add a handwritten test that tests update?

modular-magician · 2019-08-22T20:24:31Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, ffe36d1.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: modular-magician/ansible#367

modular-magician · 2019-08-22T20:42:19Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 034cf41.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

khanali21 · 2019-09-04T12:29:16Z

@rileykarson @megan07 would you know if anyone is working on the dispatch url for appengine service?

rileykarson · 2019-09-04T17:08:52Z

Since there's no issue in https://github.com/terraform-providers/terraform-provider-google/issues, I'd imagine there is not.

danawillow · 2019-09-05T20:19:32Z

templates/terraform/pre_update/app_engine_domain_mapping.erb

+	# limitations under the License.
+-%>
+updateMask := []string{}
+if d.HasChange("ssl_settings") {


Does this file mean that update_mask_fields doesn't quite work for this resource? If so, could you add a comment explaining why? (probably best to stick it here and in terraform.yaml)

I was not aware that field existed - and it worked! So I changed it to that :). Thanks!

danawillow · 2019-09-05T20:19:56Z

third_party/terraform/tests/resource_app_engine_domain_mapping_test.go

+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckBigtableAppProfileDestroy,


You probably want a different destroy

modular-magician · 2019-09-06T14:50:30Z

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 2054c13.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

googlebot added the cla: yes label Aug 20, 2019

megan07 force-pushed the megan_domain_mappings branch from 653a7d3 to aec3970 Compare August 21, 2019 15:54

modular-magician added downstream-generated terraform-google-conversion terraform-provider-google-beta terraform-provider-google labels Aug 21, 2019

megan07 requested a review from danawillow August 21, 2019 16:10

danawillow suggested changes Aug 21, 2019

View reviewed changes

danawillow added the changelog: new-resource label Aug 21, 2019

megan07 force-pushed the megan_domain_mappings branch from aec3970 to ffe36d1 Compare August 22, 2019 20:11

modular-magician added the ansible label Aug 22, 2019

megan07 requested a review from danawillow August 26, 2019 14:55

danawillow approved these changes Sep 5, 2019

View reviewed changes

megan07 force-pushed the megan_domain_mappings branch from 034cf41 to 2054c13 Compare September 6, 2019 14:38

modular-magician added the automerged label Sep 6, 2019

megan07 added 8 commits September 6, 2019 11:38

add domain mappings for app engine

39be42f

adding domain mapping

6fa67a0

exclude domain mapping from ansible.yaml

b3d68da

remove exclusion from ansible.yaml

61d7a49

exclude DomainMapping from ansible.yaml

86f3702

address review comments

e34e015

remove certificateIdDiffSuppress

3cdd11c

comments from review

5277a87

megan07 force-pushed the megan_domain_mappings branch from 2054c13 to 5277a87 Compare September 6, 2019 16:38

modular-magician merged commit cde88b5 into GoogleCloudPlatform:master Sep 6, 2019

This was referenced Oct 2, 2019

New GCP module: gcp_compute_node_template ansible/ansible#63064

Merged

New GCP module: gcp_compute_node_template_info ansible/ansible#63065

Merged

nfelt mentioned this pull request Oct 23, 2019

google_app_engine_domain_mapping AUTOMATIC certificate API is broken hashicorp/terraform-provider-google#4741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add app engine domain mappings resource #2216

Add app engine domain mappings resource #2216

megan07 commented Aug 20, 2019 •

edited

Loading

modular-magician commented Aug 21, 2019

danawillow left a comment

danawillow Aug 21, 2019

danawillow Aug 21, 2019

danawillow Aug 21, 2019

megan07 Aug 21, 2019 •

edited

Loading

danawillow Aug 21, 2019

danawillow Aug 21, 2019

megan07 Aug 22, 2019

danawillow Aug 21, 2019

modular-magician commented Aug 22, 2019

modular-magician commented Aug 22, 2019

khanali21 commented Sep 4, 2019

rileykarson commented Sep 4, 2019

danawillow Sep 5, 2019

megan07 Sep 6, 2019

danawillow Sep 5, 2019

modular-magician commented Sep 6, 2019

		@@ -0,0 +1,15 @@
		// While the domain mapping is setting up the certificate id, GET returns a field
		// `pendingManagedCertificateId`, which will eventually be empty when `certificateID`

Add app engine domain mappings resource #2216

Add app engine domain mappings resource #2216

Conversation

megan07 commented Aug 20, 2019 • edited Loading

Release Note for Downstream PRs (will be copied)

modular-magician commented Aug 21, 2019

Pull request statuses

New Pull Requests

danawillow 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

megan07 Aug 21, 2019 • edited Loading

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

modular-magician commented Aug 22, 2019

Pull request statuses

New Pull Requests

modular-magician commented Aug 22, 2019

Pull request statuses

New Pull Requests

khanali21 commented Sep 4, 2019

rileykarson commented Sep 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Sep 6, 2019

Pull request statuses

New Pull Requests

megan07 commented Aug 20, 2019 •

edited

Loading

megan07 Aug 21, 2019 •

edited

Loading