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.

Sign up for GitHub

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

Make App Engine applications updatable #1621

Merged
merged 8 commits into from
Jun 11, 2018
Merged

Make App Engine applications updatable #1621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d0a6b2b
Make App Engine applications updatable.
paddycarver May 31, 2018
9ebeeda
Fix forcenew for location_id.
paddycarver Jun 5, 2018
6d040db
Really fix location_id force new.
paddycarver Jun 7, 2018
16ab5bb
Reorder fields.
paddycarver Jun 8, 2018
4999637
Remove unnecessary CustomizeDiff function.
paddycarver Jun 8, 2018
6910d82
SetPartial on app_engine.
paddycarver Jun 8, 2018
4176a98
terraform fmt all configs in project_test.
paddycarver Jun 8, 2018
8296c16
Remove extraneous Exists checks in App Engine tests.
paddycarver Jun 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 79 additions & 45 deletions google/resource_google_project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,28 +104,15 @@ func resourceGoogleProject() *schema.Resource {
func appEngineResource() *schema.Resource {
return &schema.Resource{
Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"url_dispatch_rule": &schema.Schema{
Type: schema.TypeList,
Computed: true,
Elem: appEngineURLDispatchRuleResource(),
},
"auth_domain": &schema.Schema{
Type: schema.TypeString,
Optional: true,
// We're having trouble with PATCH throwing 400s/500s, so we need this
// to force a new resource until we can get updating working.
ForceNew: true,
Computed: true,
},
"location_id": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{
"northamerica-northeast1",
"us-central",
Expand All @@ -140,16 +127,9 @@ func appEngineResource() *schema.Resource {
"australia-southeast1",
}, false),
},
"code_bucket": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"serving_status": &schema.Schema{
Type: schema.TypeString,
Optional: true,
// We're having trouble with PATCH throwing 400s/500s, so we need this
// to force a new resource until we can get updating working.
ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{
"UNSPECIFIED",
"SERVING",
Expand All @@ -158,6 +138,26 @@ func appEngineResource() *schema.Resource {
}, false),
Computed: true,
},
"feature_settings": &schema.Schema{
Type: schema.TypeList,
Optional: true,
Computed: true,
MaxItems: 1,
Elem: appEngineFeatureSettingsResource(),
},
"name": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"url_dispatch_rule": &schema.Schema{
Type: schema.TypeList,
Computed: true,
Elem: appEngineURLDispatchRuleResource(),
},
"code_bucket": &schema.Schema{
Type: schema.TypeString,
Computed: true,
},
"default_hostname": &schema.Schema{
Type: schema.TypeString,
Computed: true,
Expand All @@ -170,16 +170,6 @@ func appEngineResource() *schema.Resource {
Type: schema.TypeString,
Computed: true,
},
"feature_settings": &schema.Schema{
Type: schema.TypeList,
Optional: true,
Computed: true,
// We're having trouble with PATCH throwing 400s/500s, so we need this
// to force a new resource until we can get updating working.
ForceNew: true,
MaxItems: 1,
Elem: appEngineFeatureSettingsResource(),
},
},
}
}
Expand Down Expand Up @@ -215,8 +205,14 @@ func appEngineFeatureSettingsResource() *schema.Resource {
}

func resourceGoogleProjectCustomizeDiff(diff *schema.ResourceDiff, meta interface{}) error {
// don't need to check if changed, the call is a no-op/error if there's no change
diff.ForceNew("app_engine")
if old, new := diff.GetChange("app_engine.#"); old != nil && new != nil && old.(int) > 0 && new.(int) < 1 {
// if we're going from app_engine set to unset, we need to delete the project, app_engine has no delete
return diff.ForceNew("app_engine")
} else if old, _ := diff.GetChange("app_engine.0.location_id"); diff.HasChange("app_engine.0.location_id") && old != nil && old.(string) != "" {
// if location_id was already set, and has a new value, that forces a new app
// if location_id wasn't set, don't force a new value, as we're just enabling app engine
return diff.ForceNew("app_engine.0.location_id")
}
return nil
}

Expand Down Expand Up @@ -276,19 +272,10 @@ func resourceGoogleProjectCreate(d *schema.ResourceData, meta interface{}) error
return fmt.Errorf("Error enabling the App Engine Admin API required to configure App Engine applications: %s", err)
}
log.Printf("[DEBUG] Enabled App Engine")
app.Id = pid
log.Printf("[DEBUG] Creating App Engine App")
op, err := config.clientAppEngine.Apps.Create(app).Do()
err = createAppEngineApp(config, pid, app)
if err != nil {
return fmt.Errorf("Error creating App Engine application: %s", err.Error())
}

// Wait for the operation to complete
waitErr := appEngineOperationWait(config.clientAppEngine, op, pid, "App Engine app to create")
if waitErr != nil {
return waitErr
return err
}
log.Printf("[DEBUG] Created App Engine App")
}

err = resourceGoogleProjectRead(d, meta)
Expand All @@ -313,6 +300,23 @@ func resourceGoogleProjectCreate(d *schema.ResourceData, meta interface{}) error
return nil
}

func createAppEngineApp(config *Config, pid string, app *appengine.Application) error {
app.Id = pid
log.Printf("[DEBUG] Creating App Engine App")
op, err := config.clientAppEngine.Apps.Create(app).Do()
if err != nil {
return fmt.Errorf("Error creating App Engine application: %s", err.Error())
}

// Wait for the operation to complete
waitErr := appEngineOperationWait(config.clientAppEngine, op, pid, "App Engine app to create")
if waitErr != nil {
return waitErr
}
log.Printf("[DEBUG] Created App Engine App")
return nil
}

func resourceGoogleProjectRead(d *schema.ResourceData, meta interface{}) error {
config := meta.(*Config)
pid := d.Id()
Expand Down Expand Up @@ -492,10 +496,40 @@ func resourceGoogleProjectUpdate(d *schema.ResourceData, meta interface{}) error
d.SetPartial("labels")
}

// ignore app_engine changes, they don't work anyways.
// App Engine App has changed
if ok := d.HasChange("app_engine"); ok {
app, err := expandAppEngineApp(d)
if err != nil {
return err
}
// ignore if app is now not set; that should force new resource using customizediff
if app != nil {
if old, new := d.GetChange("app_engine.#"); (old == nil || old.(int) < 1) && new != nil && new.(int) > 0 {
err = createAppEngineApp(config, pid, app)
if err != nil {
return err
}
} else {
log.Printf("[DEBUG] Updating App Engine App")
op, err := config.clientAppEngine.Apps.Patch(pid, app).UpdateMask("authDomain,servingStatus,featureSettings.splitHealthChecks").Do()
if err != nil {
return fmt.Errorf("Error creating App Engine application: %s", err.Error())
}

// Wait for the operation to complete
waitErr := appEngineOperationWait(config.clientAppEngine, op, pid, "App Engine app to update")
if waitErr != nil {
return waitErr
}
log.Printf("[DEBUG] Updated App Engine App")
}
d.SetPartial("app_engine")
}
}

d.Partial(false)

return nil
return resourceGoogleProjectRead(d, meta)
}

func resourceGoogleProjectDelete(d *schema.ResourceData, meta interface{}) error {
Expand Down
120 changes: 104 additions & 16 deletions google/resource_google_project_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,6 @@ func TestAccProject_appEngineBasic(t *testing.T) {
{
Config: testAccProject_appEngineBasic(pid, org),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleProjectExists("google_project.acceptance", pid),
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.name"),
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.url_dispatch_rule.#"),
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.code_bucket"),
Expand All @@ -216,7 +215,7 @@ func TestAccProject_appEngineBasic(t *testing.T) {
})
}

func TestAccProject_appEngineFeatureSettings(t *testing.T) {
func TestAccProject_appEngineUpdate(t *testing.T) {
t.Parallel()

org := getTestOrgFromEnv(t)
Expand All @@ -226,11 +225,58 @@ func TestAccProject_appEngineFeatureSettings(t *testing.T) {
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccProject_appEngineFeatureSettings(pid, org),
Config: testAccProject_appEngineNoApp(pid, org),
Check: resource.ComposeTestCheckFunc(
testAccCheckGoogleProjectExists("google_project.acceptance", pid),
),
},
{
Config: testAccProject_appEngineBasic(pid, org),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.name"),
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.url_dispatch_rule.#"),
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.code_bucket"),
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.default_hostname"),
resource.TestCheckResourceAttrSet("google_project.acceptance", "app_engine.0.default_bucket"),
),
},
resource.TestStep{
ResourceName: "google_project.acceptance",
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccProject_appEngineUpdate(pid, org),
},
resource.TestStep{
ResourceName: "google_project.acceptance",
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccProject_appEngineFeatureSettings(t *testing.T) {
t.Parallel()

org := getTestOrgFromEnv(t)
pid := acctest.RandomWithPrefix("tf-test")
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccProject_appEngineFeatureSettings(pid, org),
},
resource.TestStep{
ResourceName: "google_project.acceptance",
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccProject_appEngineFeatureSettingsUpdate(pid, org),
},
resource.TestStep{
ResourceName: "google_project.acceptance",
ImportState: true,
Expand Down Expand Up @@ -360,8 +406,8 @@ func testAccProject_labels(pid, name, org string, labels map[string]string) stri
r := fmt.Sprintf(`
resource "google_project" "acceptance" {
project_id = "%s"
name = "%s"
org_id = "%s"
name = "%s"
org_id = "%s"
labels {`, pid, name, org)

l := ""
Expand All @@ -376,11 +422,11 @@ resource "google_project" "acceptance" {
func testAccProject_deleteDefaultNetwork(pid, name, org, billing string) string {
return fmt.Sprintf(`
resource "google_project" "acceptance" {
project_id = "%s"
name = "%s"
org_id = "%s"
billing_account = "%s" # requires billing to enable compute API
auto_create_network = false
project_id = "%s"
name = "%s"
org_id = "%s"
billing_account = "%s" # requires billing to enable compute API
auto_create_network = false
}`, pid, name, org, billing)
}

Expand All @@ -402,37 +448,79 @@ resource "google_folder" "folder1" {
`, pid, projectName, folderName, org)
}

func testAccProject_appEngineNoApp(pid, org string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool that's fine!

return fmt.Sprintf(`
resource "google_project" "acceptance" {
project_id = "%s"
name = "%s"
org_id = "%s"
}`, pid, pid, org)
}

func testAccProject_appEngineBasic(pid, org string) string {
return fmt.Sprintf(`
resource "google_project" "acceptance" {
project_id = "%s"
name = "%s"
org_id = "%s"
name = "%s"
org_id = "%s"

app_engine {
auth_domain = "hashicorptest.com"
location_id = "us-central"
auth_domain = "hashicorptest.com"
location_id = "us-central"
serving_status = "SERVING"
}
}`, pid, pid, org)
}

func testAccProject_appEngineUpdate(pid, org string) string {
return fmt.Sprintf(`
resource "google_project" "acceptance" {
project_id = "%s"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you align the =s here? (as if you were to run terraform fmt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

name = "%s"
org_id = "%s"

app_engine {
auth_domain = "tf-test.club"
location_id = "us-central"
serving_status = "USER_DISABLED"
}
}`, pid, pid, org)
}

func testAccProject_appEngineFeatureSettings(pid, org string) string {
return fmt.Sprintf(`
resource "google_project" "acceptance" {
project_id = "%s"
name = "%s"
org_id = "%s"
name = "%s"
org_id = "%s"

app_engine {
location_id = "us-central"

feature_settings {
"split_health_checks" = true
}
}
}`, pid, pid, org)
}

func testAccProject_appEngineFeatureSettingsUpdate(pid, org string) string {
return fmt.Sprintf(`
resource "google_project" "acceptance" {
project_id = "%s"
name = "%s"
org_id = "%s"

app_engine {
location_id = "us-central"

feature_settings {
"split_health_checks" = false
}
}
}`, pid, pid, org)
}

func skipIfEnvNotSet(t *testing.T, envs ...string) {
for _, k := range envs {
if os.Getenv(k) == "" {
Expand Down