Add google_app_engine_application resource. #2147

paddycarver · 2018-10-02T10:04:33Z

Deprecate the app_engine sub-block of google_project, and create a
google_app_engine_application resource instead. Also, add some tests for
its behaviour, as well as some documentation for it.

Note that this is largely an implementation of the ideas discussed in
#2118, except we're not using CustomizeDiff to reject deletions without
our special flag set, because CustomizeDiff apparently doesn't run on
Delete. Who knew? This leaves us rejecting the deletion at apply time,
which is less than ideal, but the only other option I see is to silently
not delete the resource, and that's... not ideal, either.

This also stops the app_engine sub-block on google_project from forcing
new when it's removed, and sets it to computed, so users can safely move
from using the sub-block to using the resource without state surgery or
deleting their entire project. This does mean it's impossible to delete
an App Engine application from a sub-block now, but seeing as that was
the same situation before, and we just papered over it by making the
project recreate itself in that situation, and people Were Not Fans of
that, I'm considering that an acceptable casualty.

Deprecate the app_engine sub-block of google_project, and create a google_app_engine_application resource instead. Also, add some tests for its behaviour, as well as some documentation for it. Note that this is largely an implementation of the ideas discussed in #2118, except we're not using CustomizeDiff to reject deletions without our special flag set, because CustomizeDiff apparently doesn't run on Delete. Who knew? This leaves us rejecting the deletion at apply time, which is less than ideal, but the only other option I see is to silently not delete the resource, and that's... not ideal, either. This also stops the app_engine sub-block on google_project from forcing new when it's removed, and sets it to computed, so users can safely move from using the sub-block to using the resource without state surgery or deleting their entire project. This does mean it's impossible to delete an App Engine application from a sub-block now, but seeing as that was the same situation before, and we just papered over it by making the project recreate itself in that situation, and people Were Not Fans of that, I'm considering that an acceptable casualty.

danawillow · 2018-10-02T22:07:52Z

google/resource_app_engine_application.go

+	}
+}
+
+func appEngineApplicationURLDispatchRuleResource() *schema.Resource {


nit: any reason these are funcs and not vars?

I think it's conventional/how I learned how to do it? If I had to guess, it's to keep important mutable state out of the global scope (the Schema is a pointer, someone mutating it will have a big effect on everyone else, probably unintentionally) but I could be very wrong about that.

danawillow · 2018-10-02T22:10:18Z

google/resource_app_engine_application.go

+	pid := d.Id()
+
+	app, err := config.clientAppEngine.Apps.Get(pid).Do()
+	if err != nil && !isGoogleApiErrorWithCode(err, 404) {


I think since it's its own resource now, you can do a regular handleNotFoundError call

danawillow · 2018-10-02T22:12:48Z

google/resource_app_engine_application.go

+}
+
+func resourceAppEngineApplicationDelete(d *schema.ResourceData, meta interface{}) error {
+	log.Println("[DEBUG] App Engine applications cannot be destroyed once created. The project must be deleted to delete the application.")


it doesn't really matter since from my memory log levels only apply to tf core and not the providers, but if they did make a difference, i would say this should be a WARNING like we do for KmsKeyRing

danawillow · 2018-10-02T22:13:16Z

google/resource_app_engine_application.go

+	if err != nil {
+		return err
+	}
+	app.Id = project


this line can be removed, it's handled in the expander

danawillow · 2018-10-02T22:13:57Z

google/resource_app_engine_application.go

+	if len(blocks) < 1 {
+		return nil, nil
+	}
+	if len(blocks) > 1 {


this is already a plan-time check, so no need to check it again

danawillow · 2018-10-02T22:17:17Z

google/resource_app_engine_application.go

+				Optional: true,
+				Computed: true,
+			},
+			"location_id": &schema.Schema{


Is this updatable? It's not in the updateMask if so

Nope. I guess I'll make it ForceNew, even though that will cause a problem if people do update it? I'm not 100% sure what a good user experience is in this case. I guess I could customize diff to reject this, at least.

danawillow · 2018-10-02T22:28:30Z

website/docs/r/app_engine_application.html.markdown

+ Allows management of an App Engine application.
+---
+
+# google\_app_engine_application


I would assume we either need slashes before all underscores or before no underscores

danawillow · 2018-10-02T22:29:09Z

website/docs/r/app_engine_application.html.markdown

+
+```hcl
+resource "google_project" "my_project" {
+  name = "My Project"


nit: alignment

danawillow · 2018-10-02T22:29:15Z

website/docs/r/app_engine_application.html.markdown

+
+resource "google_app_engine_application" "app" {
+  project         = "${google_project.my_project.project_id}"
+  location_id     = "us-central'


nit: too many spaces after location_id

danawillow · 2018-10-02T22:29:39Z

website/docs/r/app_engine_application.html.markdown

+
+* `auth_domain` - (Optional) The domain to authenticate users with when using App Engine's User API.
+
+* `serving_status` - (Optional) The serving status of the app. Note that this can't be updated at the moment.


the code makes it look like it can, is this sentence wrong or should the code be updated?

paddycarver · 2018-10-02T23:35:50Z

@danawillow ready for round two, I think!

danawillow

Our migration story is going to be import I assume, right?

danawillow · 2018-10-03T02:15:03Z

google/resource_app_engine_application.go

+func appEngineApplicationLocationIDCustomizeDiff(d *schema.ResourceDiff, meta interface{}) error {
+	old, new := d.GetChange("location_id")
+	if old != "" && old != new {
+		return fmt.Errorf("Cannot change location_id once the resource is created.")


Oh clever! Let's do this for project too.

danawillow · 2018-10-03T02:16:38Z

google/resource_app_engine_application.go

+	// Wait for the operation to complete
+	waitErr := appEngineOperationWait(config.clientAppEngine, op, project, "App Engine app to create")
+	if waitErr != nil {
+		return waitErr


Either we need to d.SetId("") here or move the d.SetId(project) call to after this. I'd recommend the first just for consistency with the rest of the resources.

paddycarver · 2018-10-03T06:36:28Z

Our migration story is going to be import I assume, right?

Yup!

Import the app.
Add the app to your config.
Remove the app_engine sub-block from google_project. Removing it won't ForceNew now.

…appengine Add google_app_engine_application resource.

ghost · 2018-11-16T13:56:43Z

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!

ghost added the size/xl label Oct 2, 2018

paddycarver requested a review from danawillow October 2, 2018 10:04

This was referenced Oct 2, 2018

TF tries to destroy/recreate older projects with GAE enabled upon upgrade #1561

Closed

Proposal: move app_engine sub-block to its own resource #2118

Closed

Pretend to delete, no error.

3ab8e95

danawillow reviewed Oct 2, 2018

View reviewed changes

Adjust for Dana's comments.

691ae99

paddycarver added this to the 1.19.0 milestone Oct 2, 2018

danawillow approved these changes Oct 3, 2018

View reviewed changes

Fix Dana's comments, round two!

8f6d256

paddycarver merged commit 69589e5 into master Oct 3, 2018

mkozjak mentioned this pull request Oct 5, 2018

App Engine recreated with resource "google_app_engine_application" #2175

Closed

paultyng deleted the paddy_new_appengine branch October 29, 2018 19:27

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018

Merge pull request hashicorp#2147 from terraform-providers/paddy_new_…

2b23f45

…appengine Add google_app_engine_application resource.

ghost locked and limited conversation to collaborators Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add google_app_engine_application resource. #2147

Add google_app_engine_application resource. #2147

paddycarver commented Oct 2, 2018

danawillow Oct 2, 2018

paddycarver Oct 2, 2018

danawillow Oct 2, 2018

danawillow Oct 2, 2018

danawillow Oct 2, 2018

danawillow Oct 2, 2018

danawillow Oct 2, 2018

paddycarver Oct 2, 2018

danawillow Oct 2, 2018

danawillow Oct 2, 2018

danawillow Oct 2, 2018

danawillow Oct 2, 2018

paddycarver commented Oct 2, 2018

danawillow left a comment

danawillow Oct 3, 2018

danawillow Oct 3, 2018

paddycarver commented Oct 3, 2018

ghost commented Nov 16, 2018


		* `auth_domain` - (Optional) The domain to authenticate users with when using App Engine's User API.

		* `serving_status` - (Optional) The serving status of the app. Note that this can't be updated at the moment.

Add google_app_engine_application resource. #2147

Add google_app_engine_application resource. #2147

Conversation

paddycarver commented Oct 2, 2018

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

paddycarver commented Oct 2, 2018

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

paddycarver commented Oct 3, 2018

ghost commented Nov 16, 2018