adds scaling properties to appengine standard version

[wvanderdeijl]

Release Note Template for Downstream PRs (will be copied)

appengine: Added `automatic_scaling` `basic_scaling` and `manual_scaling` to `google_app_engine_standard_app_version`

This adds automaticScaling, basicScaling, and manualScaling properties to AppEngine Standard. These are similar to the scaling properties that already existed on AppEngine Flexible, but there are some notable differences. Based on the Google API documentation and trying all properties, I've removed all the properties that are not allowed for AppEngine Standard as we have different resources for AppEngine Standard and AppEngine Flex.

I have also marked deployment as required for AppEngine Standard. I couldn't find a reason in the commit history why this was marked as not required, and testing has shown that omitting the deployment throws a 400.

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@slevenick, please review this PR or find an appropriate assignee.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 685 insertions(+), 36 deletions(-))
Terraform Beta: Diff ( 3 files changed, 685 insertions(+), 36 deletions(-))
TF Conversion: Diff ( 1 file changed, 205 insertions(+))
Inspec: Diff ( 8 files changed, 216 insertions(+), 1 deletion(-))

[modular-magician]

Oops! It looks like you're using an unknown release-note type in your changelog entries:

• REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

[slevenick]

Thanks for the addition!

Looks like this has broken a couple existing tests:
TestAccAppEngineServiceSplitTraffic_appEngineServiceSplitTrafficExample
and
TestAccAppEngineApplicationUrlDispatchRules_appEngineApplicationUrlDispatchRulesBasicExample.

They break because
- "basic_scaling": one ofautomatic_scaling,basic_scaling,manual_scaling must be specified

Do we need one of them to be defined in all cases? If so, can you add one of those fields to these tests?

[slevenick]

See above comment

[wvanderdeijl]

It was not my intention to make the scaling properties mandatory. That might also break existing deployment scripts from users. The previous version did not even have these properties. I've confirmed that AppEngine will use automaticScaling when nothing is specified.

I've changed exactly_one_of to conflicts in the assumption that this means the properties are no longer required, but cannot co-exist, but this is my first contribution to magic modules so my understanding on how everything works is still very limited.

Let me know if this is the wrong approach, or if you want me to squash the commits.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 685 insertions(+), 36 deletions(-))
Terraform Beta: Diff ( 3 files changed, 685 insertions(+), 36 deletions(-))
TF Conversion: Diff ( 1 file changed, 205 insertions(+))
Inspec: Diff ( 8 files changed, 216 insertions(+), 1 deletion(-))

[slevenick]

Looking at the API, conflicts is definitely the right option for these.

Had a couple questions on how updates will work

[slevenick]

Tried this myself, looks like leaving it blank returns an API error stating it is required. Thanks for the update

[slevenick]

Does it make sense to be able to set this via terraform if it is explicitly supposed to be updated later via a different interface?

I'm thinking of the case where someone sets a value via terraform, then updates it via another interface. Generally the API will start returning the new value, causing a diff as terraform tries to reset it to the original value. Maybe we just want to be able to set this and require users to ignore it via lifecycle ignore_changes?

Do you have an idea of how this will handle changes to this field?

[wvanderdeijl]

The not about updating using the Modules API came from the REST API docs. I looked into this and this only applies to the python 2 appEngine (which is now deprecated). I clarified that in the updated description.
For all other runtimes you would just have to use the AppEngine Admin API to update the scaling properties.
However, I do see why there are use cases where it makes sense to adjust the manual-scaling properties at runtime. I've added a note to the description explaining that users that intend to adjust this at runtime (outside of terraform) should specify this in ignore_changes.

[slevenick]

Can you throw as many of these fields as possible in here so they are tested?

Preferably we would make a new handwritten test file similar to this one: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/third_party/terraform/tests/resource_app_engine_application_test.go

Where we could add some of these fields, then perform an update to either change the scaling mode and update some of these values.

[wvanderdeijl]

I've added all the scaling fields to this test.

I also tried to create a handwritten test by copying resource_app_engine_standard_app_version_test.go. Unfortunately this is my first ever attempt at writing go and I am unable to run the tests locally:

% make testacc TEST=./google TESTARGS='-run=TestAccAppEngineStandardAppVersion_update'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccAppEngineStandardAppVersion_update -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccAppEngineStandardAppVersion_update
=== PAUSE TestAccAppEngineStandardAppVersion_update
=== CONT  TestAccAppEngineStandardAppVersion_update
    TestAccAppEngineStandardAppVersion_update: testing.go:635: Step 0 error: errors during apply:

        Error: error creating project tf-test-appeng-stdiuz8pa3mkx (tf-test-appeng-stdiuz8pa3mkx): googleapi: Error 400: field [parent] has issue [Parent id must be numeric.], badRequest. If you received a 403 error, make sure you have the `roles/resourcemanager.projectCreator` permission

          on /var/folders/wf/xm7lxbtn1nbbvzfzb3r4v89r0000gn/T/tf-test027251687/main.tf line 2:
          (source code not available)


--- FAIL: TestAccAppEngineStandardAppVersion_update (2.01s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-google/google	2.463s
FAIL
make: *** [testacc] Error 1

I can see that the automated build in this PR has failed, so I must have done something wrong. But I do not have the privileges to see the automated build results at https://console.cloud.google.com/cloud-build/builds/572880b0-0f3a-4361-be02-23818735f1fd?project=673497134629 and I can't run the tests locally as they appear to be missing the org-id (although I have set the GOOGLE_ORG env var to our domain name).

Can you see what is failing in the CI build or point me to instructions on how to run these tests locally?

Never mind; I got the integration test working locally by temporarily setting the org_id and billing_account to a fixed value in third_party/terraform/tests/resource_app_engine_standard_app_version_test.go (but obviously did not commit that).

This new test started as a copy of third_party/terraform/tests/resource_app_engine_flexible_app_version_test.go but I changed some things:

• removed assigning roles/compute.networkUser to the gae-api-prod serviceAccount as that is specific to appEngine flex (appEngine standard uses different serviceAccount and does not need this role)
• runtime python was not a valid value, so changed this to python38
• removed runtime_api_version as you cannot set that with python 3.x
• removed appEngine flex specific field: resources, liveness_check, readiness_check, network
• do not add the app.yaml file to the deployment bucket as all settings are supplied by terraform and not by a yaml file
• initial version uses automaticScaling with F2 instanceClass and this is then updated to basicScaling
• minor stuff like renaming all "flex" to "standard"

[slevenick]

Awesome, thanks for all the work!

This guide has info on setting the org_id and billing account as environment variables: https://github.com/terraform-providers/terraform-provider-google/blob/master/.github/CONTRIBUTING.md#tests

[wvanderdeijl]

I had already tried to set the env vars GOOGLE_ORG to the domain name of our organisation and GOOGLE_BILLING_ACCOUNT to the guid-like ID of our billing account. But that doesn't seem to work with the code from the terraform test: https://github.com/wvanderdeijl/magic-modules/blob/app-engine-standard-scaling/third_party/terraform/tests/resource_app_engine_standard_app_version_test.go#L12-L13

% make testacc TEST=./google TESTARGS='-run=TestAccAppEngineStandardAppVersion_update'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccAppEngineStandardAppVersion_update -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccAppEngineStandardAppVersion_update
=== PAUSE TestAccAppEngineStandardAppVersion_update
=== CONT  TestAccAppEngineStandardAppVersion_update
    TestAccAppEngineStandardAppVersion_update: testing.go:635: Step 0 error: errors during apply:

        Error: error creating project tf-test-appeng-std8o3aqjdj4r (tf-test-appeng-std8o3aqjdj4r): googleapi: Error 400: field [parent] has issue [Parent id must be numeric.], badRequest. If you received a 403 error, make sure you have the `roles/resourcemanager.projectCreator` permission

          on /var/folders/wf/xm7lxbtn1nbbvzfzb3r4v89r0000gn/T/tf-test299260747/main.tf line 2:
          (source code not available)


--- FAIL: TestAccAppEngineStandardAppVersion_update (2.35s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-google/google	3.443s
FAIL
make: *** [testacc] Error 1

I also tried setting GOOGLE_ORG to the numeric id of our organisation, but that throws the same error.

Anyhow; it does work on your CI servers so it must be something I'm doing wrong (or missing from CONTRIBUTING.md) but it doesn't seem relevant to this PR.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 717 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 5 files changed, 717 insertions(+), 40 deletions(-))
TF Conversion: Diff ( 1 file changed, 205 insertions(+))
Inspec: Diff ( 8 files changed, 216 insertions(+), 1 deletion(-))

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 944 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 6 files changed, 944 insertions(+), 40 deletions(-))
TF Conversion: Diff ( 1 file changed, 205 insertions(+))
Inspec: Diff ( 8 files changed, 216 insertions(+), 1 deletion(-))

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 944 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 6 files changed, 944 insertions(+), 40 deletions(-))
TF Conversion: Diff ( 1 file changed, 205 insertions(+))
Inspec: Diff ( 8 files changed, 216 insertions(+), 1 deletion(-))

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 927 insertions(+), 41 deletions(-))
Terraform Beta: Diff ( 6 files changed, 927 insertions(+), 41 deletions(-))
TF Conversion: Diff ( 1 file changed, 205 insertions(+))
Inspec: Diff ( 8 files changed, 216 insertions(+), 1 deletion(-))

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 926 insertions(+), 41 deletions(-))
Terraform Beta: Diff ( 6 files changed, 926 insertions(+), 41 deletions(-))
TF Conversion: Diff ( 1 file changed, 205 insertions(+))
Inspec: Diff ( 8 files changed, 216 insertions(+), 1 deletion(-))

[wvanderdeijl]

keeping the ignore_read was not an option as that failed the integration test. But since the field itself is optional and the default value depends om the type of scaling, I decided to use default_from_api. I chose that based on other fields and and assumption on what this does.
But I am a little outside of my comfort zone here, so please check if this is the correct approach

[slevenick]

Hmmm, I'd be curious as to why ignore_read causes the test to fail. Does it happen during the update step when you modify the instance class? What does the failure look like?

Generally default_from_api does make sense in this case, as it marks the field as Computed which means it can take its value from the API version if it is not specified.

[wvanderdeijl]

I'm not 100% sure what is happening with ignore_read, but it seems like it is not reading back the instanceClass property so it determines it to be empty, which fails the test as the test expects it to be F2:

% make testacc TEST=./google TESTARGS='-run=TestAccAppEngineStandardAppVersion_update'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccAppEngineStandardAppVersion_update -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccAppEngineStandardAppVersion_update
=== PAUSE TestAccAppEngineStandardAppVersion_update
=== CONT  TestAccAppEngineStandardAppVersion_update
    TestAccAppEngineStandardAppVersion_update: testing.go:635: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) {
        }


        (map[string]string) (len=1) {
         (string) (len=14) "instance_class": (string) (len=2) "F2"
        }

--- FAIL: TestAccAppEngineStandardAppVersion_update (190.12s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-google/google	190.634s
FAIL
make: *** [testacc] Error 1

[wvanderdeijl]

I also figured that default_from_api sounds like a less brute force method than simply ignore_read which would make default_from_api preferable. But that is just my gut feeling based on my own expectations what these options might mean.

[slevenick]

Ah, that actually makes sense, and ignore_read was probably incorrect in the first place.

The step that the test is failing on is an import test, which imports the resource from the existing resource in GCP. Because instance_class is ignore_read, it cannot fetch this information from GCP and fails because the imported state doesn't match the previous state.

There was no way for this to work during import before

[wvanderdeijl]

@slevenick is there anything else you want me to look at or change, or is this one good to go?

[slevenick]

Thanks for the contribution!