Make App Engine applications updatable #1621
Conversation
No longer ForceNew when adding an App Engine application to a project, when modifying the auth domain, modifying the serving status, or modifying the feature settings.
google/resource_google_project.go
Outdated
| @@ -103,6 +103,7 @@ func resourceGoogleProject() *schema.Resource { | |||
|
|
|||
| func appEngineResource() *schema.Resource { | |||
| return &schema.Resource{ | |||
| CustomizeDiff: resourceGoogleProjectAppEngineCustomizeDiff, | |||
| Schema: map[string]*schema.Schema{ | |||
| "name": &schema.Schema{ | |||
| Type: schema.TypeString, | |||
There was a problem hiding this comment.
Not actually related to this change, but can you order the properties in Required -> Optional -> Computed order? I'm a big fan of that to make it easy to immediately see which fields are settable.
google/resource_google_project.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func resourceGoogleProjectAppEngineCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error { |
There was a problem hiding this comment.
This can probably be removed
There was a problem hiding this comment.
Oops, sorry, this is a remnant of my futzing around with customize diff -_-
google/resource_google_project.go
Outdated
| log.Printf("[DEBUG] Updated App Engine App") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
May as well d.SetPartial here in case someone comes and adds a new HasChange after this
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testAccProject_appEngineNoApp(pid, org), | ||
| Check: resource.ComposeTestCheckFunc( |
There was a problem hiding this comment.
If you want to keep the checks I won't tell you no, but they aren't necessary if you check by importing
There was a problem hiding this comment.
I think in this case import wouldn't actually check them, because they're computed fields not set in the config, so I believe without the checks, if the fields just aren't being set, ImportStateVerify won't complain. Because (from what I understand) ImportStateVerify just creates a new, empty state, imports the resource again, and checks it against the state from creating the resource. If there was a bug where these weren't being set, in both cases it would have them not set, so ImportStateVerify would think that's fine. And because they're computed and not config fields, there'd be no diff.
There was a problem hiding this comment.
Cool, makes sense! Regardless, you can still take out testAccCheckGoogleProjectExists
| func testAccProject_appEngineUpdate(pid, org string) string { | ||
| return fmt.Sprintf(` | ||
| resource "google_project" "acceptance" { | ||
| project_id = "%s" |
There was a problem hiding this comment.
nit: can you align the =s here? (as if you were to run terraform fmt)
| @@ -402,6 +459,15 @@ resource "google_folder" "folder1" { | |||
| `, pid, projectName, folderName, org) | |||
| } | |||
|
|
|||
| func testAccProject_appEngineNoApp(pid, org string) string { | |||
There was a problem hiding this comment.
this is pretty much identical to testAccProject_create (which lives in resource_google_project_iam_policy_test.go) if you wanted to reuse that (a few other resources are doing so)
There was a problem hiding this comment.
I have a weakly-held opinion that it's better to have the configs for semantically different things separated, even if the implementation ends up looking the same, to minimize unintended consequences of changes. But if you feel strongly about it, I'm happy to change this.
|
Think I addressed all the feedback here. :) |
|
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
locked and limited conversation to collaborators
No longer ForceNew on every little change to an App Engine application. Now adding an App Engine application to a project doesn't force a new project, only removing an application from a project forces a new project. Also, updating the location_id forces a new project, but all other fields can be updated in place.
This depends on #1620 and should only be merged after that is.