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

Jump to bottom

add app engine flex version #3110

Merged

megan07 merged 12 commits into GoogleCloudPlatform:master from hashicorp:megan_app_engine_flex_version

Mar 11, 2020

Contributor

megan07 commented Feb 11, 2020 •

edited

Loading

Added flexible app version resource.

Fixes hashicorp/terraform-provider-google#5025

This uses the same API as google_app_engine_standard_app_version, but they have different schemas (some fields are flex-only, etc) so we have decided to split them into two different resources.

Release Note Template for Downstream PRs (will be copied)

appengine: added new resource `google_appengine_flexible_app_version"

googlebot added the cla: yes label

megan07 force-pushed the megan_app_engine_flex_version branch from a823e69 to e47f880 Compare

February 11, 2020 22:21

Collaborator

modular-magician commented Feb 12, 2020

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

Diff report:

Terraform GA: Diff ( 7 files changed, 4795 insertions(+), 7 deletions(-))
Terraform Beta: Diff ( 7 files changed, 4795 insertions(+), 7 deletions(-))
TF Conversion: Diff ( 1 file changed, 1608 insertions(+))

megan07 commented

View reviewed changes

products/appengine/api.yaml Show resolved Hide resolved

megan07 requested review from chrisst and c2thorn

February 12, 2020 15:43

c2thorn reviewed

View reviewed changes

third_party/terraform/tests/resource_app_engine_flexible_app_version_sweeper_test.go Outdated

    
              func init() {

              	resource.AddTestSweepers("AppEngineFlexibleAppVersion", &resource.Sweeper{

              		Name: "AppEngineFlexibleAppVersion",

Member

c2thorn Feb 13, 2020 •

edited

Loading

This sweeper should sweep Standard/Flexible indiscriminately right? Might be worth renaming to reflect that. Mostly so I don't forget and add a sweeper for standard in the future...

c2thorn requested changes

View reviewed changes

Member

c2thorn left a comment

Just looked at the sweeper for now

third_party/terraform/tests/resource_app_engine_flexible_app_version_sweeper_test.go Outdated

Comment on lines 35 to 50

    
              	// Setup variables to replace in list template

              	d := &ResourceDataMock{

              		FieldsInSchema: map[string]interface{}{

              			"project":  config.Project,

              			"region":   region,

              			"location": region,

              			"zone":     "-",

              		},

              	}

              	listTemplate := strings.Split("https://appengine.googleapis.com/v1/apps/{{project}}/services", "?")[0]

              	listUrl, err := replaceVars(d, config, listTemplate)

              	if err != nil {

              		log.Printf("[INFO][SWEEPER_LOG] error preparing sweeper list url: %s", err)

              		return nil

              	}

Member

c2thorn Feb 13, 2020

These lines are in the sweeper template because the we won't know what the URL will look like until generation. Fortunately we with a handwritten sweeper we can just replace these lines with:

	listUrl := "https://appengine.googleapis.com/v1/apps/" + config.Project + "/services"

third_party/terraform/tests/resource_app_engine_flexible_app_version_sweeper_test.go Outdated

    
              	for _, ri := range rl {

              		obj := ri.(map[string]interface{})

              		if obj["id"] == nil {

              			log.Printf("[INFO][SWEEPER_LOG] %s resource name was nil", resourceName)

Member

c2thorn Feb 13, 2020

It is on my TODO list to handle id's in the sweeper generation, so resources like these would be covered. For now, I think it is important to keep the distinction between id & name, especially since both are present in the api.yaml

Suggested change

      
            			log.Printf("[INFO][SWEEPER_LOG] %s resource name was nil", resourceName)
          
            			log.Printf("[INFO][SWEEPER_LOG] %s resource id was nil", resourceName)

third_party/terraform/tests/resource_app_engine_flexible_app_version_sweeper_test.go Outdated

    
              			return nil

              		}

              		name := GetResourceNameFromSelfLink(obj["id"].(string))

Member

c2thorn Feb 13, 2020

id is all we need

Suggested change

      
            		name := GetResourceNameFromSelfLink(obj["id"].(string))
          
            		id := obj["id"].(string)

third_party/terraform/tests/resource_app_engine_flexible_app_version_sweeper_test.go Outdated

    
              		name := GetResourceNameFromSelfLink(obj["id"].(string))

              		// Only sweep resources with the test prefix

              		if !strings.HasPrefix(name, "tf-test") {

Member

c2thorn Feb 13, 2020

Suggested change

      
            		if !strings.HasPrefix(name, "tf-test") {
          
            		if !strings.HasPrefix(id, "tf-test") {

third_party/terraform/tests/resource_app_engine_flexible_app_version_sweeper_test.go Outdated

Comment on lines 83 to 89

    
              		deleteTemplate := "https://appengine.googleapis.com/v1/apps/{{project}}/services/{{service}}"

              		deleteUrl, err := replaceVars(d, config, deleteTemplate)

              		if err != nil {

              			log.Printf("[INFO][SWEEPER_LOG] error preparing delete url: %s", err)

              			return nil

              		}

              		deleteUrl = deleteUrl + name

Member

c2thorn Feb 13, 2020

See: my explanation on the list URL code.
We can replace these lines with:

deleteUrl := "https://appengine.googleapis.com/v1/apps/" + config.Project + "/services/" + id

third_party/terraform/tests/resource_app_engine_flexible_app_version_sweeper_test.go Outdated

    
              		if err != nil {

              			log.Printf("[INFO][SWEEPER_LOG] Error deleting for url %s : %s", deleteUrl, err)

              		} else {

              			log.Printf("[INFO][SWEEPER_LOG] Sent delete request for %s resource: %s", resourceName, d.Get("service"))

Member

c2thorn Feb 13, 2020

Suggested change

      
            			log.Printf("[INFO][SWEEPER_LOG] Sent delete request for %s resource: %s", resourceName, d.Get("service"))
          
            			log.Printf("[INFO][SWEEPER_LOG] Sent delete request for %s resource: %s", resourceName, id)

chrisst suggested changes

View reviewed changes

Contributor

chrisst left a comment

This is a Big resource (capital B) so I can't promise much more than spot checking some of the fields to see if they make sense. But I have some overall thoughts as well:

It appears to be a true superset of the StandardAppVersion resource and they share the same endpoints. It is probably good to add a comment in the api.yaml indicating the relationship as I imagine that any bug fix on one resource will need to be added to the other. This is effectively "cross linking" the resources for developers, but should also be done in the public docs as well.

The surface area of this resource is huge, but you only have a single test. I would add a couple more testable examples, as this will improve test coverage and also help our users figure out how it should be used. For example handlers appears to be the most important field of the resource and there's no go example of how to use it. Also there should be some hand written update tests otherwise the update method is completely untested. Extra tests will be extra important with large resources like this as reviewers are unlikely to catch the small stuff like fields that should be required or defaulted.

products/appengine/api.yaml Outdated

    
                - !ruby/object:Api::Resource

                  name: 'FlexibleAppVersion'

                  description: |

                    Flexible App Version resource to create a new version of flexible GAE Application.

Contributor

chrisst Feb 12, 2020

I think it's worth explaining how this resource is different from the standard app engine resource and cross linking the other resource in the doc will go a long way to helping users find the right one. Linking to https://cloud.google.com/appengine/docs/the-appengine-environments would be a good call.

products/appengine/api.yaml Outdated

    
                  collection_url_key: 'versions'

                  base_url: 'apps/{{project}}/services/{{service}}/versions'

                  delete_url: 'apps/{{project}}/services/{{service}}/versions/{{version_id}}'

                  delete_verb: :DELETE

Contributor

chrisst Feb 12, 2020

isn't this the default already?

products/appengine/api.yaml Outdated

    
                  references: !ruby/object:Api::Resource::ReferenceLinks

                    guides:

                      'Official Documentation':

                        'https://cloud.google.com/appengine/docs/admin-api/deploying-overview'

Contributor

chrisst Feb 12, 2020

I'm not sure if this is the most helpful "overview" link. This appears to be more about the admin api and less about the resource at large. Would https://cloud.google.com/appengine/docs/flexible be more appropriate?

products/appengine/terraform.yaml Outdated

    
                      default_from_api: true

                    betaSettings: !ruby/object:Overrides::Terraform::PropertyOverride

                      default_from_api: true

                    livenessCheck: !ruby/object:Overrides::Terraform::PropertyOverride

Contributor

chrisst Feb 13, 2020

I'm curious if both livenessCheck and readinessCheck need to be defaulted from the API. Since they are set to at_least_one_of 1 will always be defined in the config, are you seeing default values coming back from the non-defined field? Is this something where we could just ignore what was coming back from the API for the non-intended block instead of marking them both computed?

Optional/Computed doesn't always work the same way for all fields so I personally try to avoid it.

Contributor Author

megan07 Feb 19, 2020

Yes, there are default values coming back. I will just set the defaults in api.yaml, which might be a better option.

products/appengine/terraform.yaml

    
                      default_from_api: true

                    automaticScaling.maxConcurrentRequests: !ruby/object:Overrides::Terraform::PropertyOverride

                      default_from_api: true

                    betaSettings: !ruby/object:Overrides::Terraform::PropertyOverride

Contributor

chrisst Feb 13, 2020

Are there default betaSettings that come from the api?

Contributor Author

megan07 Feb 19, 2020

Yes, there are, use_deployment_manager, no_appserver_affinity, and has_docker_image that I've seen.

Contributor Author

megan07 Feb 21, 2020

This is confusing - those values are returned in the response, but if I do an update (which is just a POST for a new version), it says those values (no_appserver_affinity and use_deployment_manager) aren't allowed to be set in the flexible environment. Which makes me think we should ignore_read these, however, if there aren't conflicts with other fields, we probably want to write them to state.

Contributor

chrisst Feb 24, 2020

Can it be set by the user at all? If they can't be sent in a Create or Update then I would just remove the field completely. We can always add them back in as fully computed if a user needs this as an output. Growing a resource is much easier than shrinking it.

products/appengine/api.yaml Outdated

    
                          description: |

                            Security (HTTPS) enforcement for this URL.

                          values:

                            - :SECURE_UNSPECIFIED

Contributor

chrisst Feb 13, 2020

A lot of these enums have a 0 value (default value) of SOMETHING_UNSPECIFIED. But the API almost always rejects a request that contains one of these values. AFAIK it's primarily used for internal error handling of uninitialized objects. 99% of the time we should not expose these values to Terraform users.

products/appengine/api.yaml Outdated

    
                            - :REDIRECT_HTTP_RESPONSE_CODE_307

                        - !ruby/object:Api::Type::NestedObject

                          name: 'script'

                          # TODO (mbang): Exactly one of script, staticFiles, or apiEndpoint must be set

Contributor

chrisst Feb 13, 2020

TODO?

Contributor Author

megan07 Feb 13, 2020

Currently ExactlyOneOf won't' work for arrays (or fields whose parent's are arrays). We're hoping to eventually make that possible, so I added this in places where I want to update when we have that ability.

products/appengine/api.yaml Outdated

    
                        - !ruby/object:Api::Type::Enum

                          name: 'redirectHttpResponseCode'

                          description: |

                            Redirect codes.

Contributor

chrisst Feb 13, 2020

Is this a user supplied value? If yes is there a better description of how it should be used?

products/appengine/api.yaml Outdated

    
                    - !ruby/object:Api::Type::NestedObject

                      name: 'vpcAccessConnector'

                      description: |

                        The entrypoint for the application.

Contributor

chrisst Feb 13, 2020

copy/pasted from entrypoint?

products/appengine/terraform.yaml Outdated

    
                    threadsafe: !ruby/object:Overrides::Terraform::PropertyOverride

                      ignore_read: true

                    instanceClass: !ruby/object:Overrides::Terraform::PropertyOverride

                      ignore_read: true

Contributor

chrisst Feb 13, 2020

This feels like a lot of ignore read values. Some like deployment make more sense to not care about, but instanceClass seems like something Terraform would want to correct. Adding something to the description to indicate that upstream changes are ignored might be helpful for some of them if there is a reason that's not super intuitive. For Terraform specific behavior like this you can override description here in the terraform.yaml.

megan07 force-pushed the megan_app_engine_flex_version branch from 059c0b4 to 27e7aa7 Compare

February 21, 2020 20:17

Collaborator

modular-magician commented Feb 21, 2020

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

Diff report:

Terraform GA: Diff ( 10 files changed, 4016 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 10 files changed, 4016 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1262 insertions(+))

Contributor Author

megan07 commented Feb 21, 2020

I ended up removing a few fields - I'm super confused, and hope that's fine, just going to note them here:

handlers and libraries - after testing with both nodejs and python, I could send it in the request, but it would not be included in the response. the best i could conclude is that it's not an option with the flex environment, as it's included in the api.yaml documentation for the standard version but not for flexible
error_handlers - i didn't test this one, i can, but it seemed to follow the same pattern as handlers
threadsafe - i tested with both nodejs and python and in both cases, if i set threadsafe = false, it would fail, but if i didn't set it, it would default to true, it is also not included in the api.yaml documentation for the flexible environment, for any of the runtimes.

I also wrote an update test, but it failed due to 27e7aa7#r382746058 so I took it out and we can decide maybe decide how to handle that?

megan07 requested review from chrisst and c2thorn

February 21, 2020 20:27

Contributor

khanali21 commented Feb 24, 2020

@megan07 Thank you very much for picking this up, I was going to start working on it. I will keep watch on this. If you need help let me know.

chrisst reviewed

View reviewed changes

Contributor

chrisst left a comment

Can you elaborate on why the update test was failing? I can't tell from the linked commit.

products/appengine/terraform.yaml

    
                      default_from_api: true

                    automaticScaling.maxConcurrentRequests: !ruby/object:Overrides::Terraform::PropertyOverride

                      default_from_api: true

                    betaSettings: !ruby/object:Overrides::Terraform::PropertyOverride

Contributor

chrisst Feb 24, 2020

Can it be set by the user at all? If they can't be sent in a Create or Update then I would just remove the field completely. We can always add them back in as fully computed if a user needs this as an output. Growing a resource is much easier than shrinking it.

third_party/terraform/tests/resource_app_engine_flexible_app_version_test.go Outdated

    
              resource "google_storage_bucket_object" "object" {

                name   = "hello-world-django.zip"

                bucket = google_storage_bucket.bucket.name

                source = "./test-fixtures/appengine/hello-world-django.zip"

Contributor

chrisst Feb 24, 2020

Does this have to be a zip file? I would prefer adding this as a file or folder so that git can do file by file diffs, especially if this ever needs updating.

megan07 added 8 commits

March 9, 2020 09:56


          added flexible app version

0a34c95


          add sweeper file for flexible_app_version

4feaa4b


          sweeper changes, add website link, etc

c4d5797


          add exclude to inspec.yaml

84d2b8a


          add exclude to ansible.yaml

87484ff


          missed

ff03704


          sweeper review comments

7ef2e7d


          add test, other changes

a3e18ed

megan07 force-pushed the megan_app_engine_flex_version branch from 27e7aa7 to a3e18ed Compare

March 9, 2020 16:09

Collaborator

modular-magician commented Mar 9, 2020

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

Diff report:

Terraform GA: Diff ( 10 files changed, 4016 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 10 files changed, 4016 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1262 insertions(+))


          update test to flask, ignore read on beta_settings for now

51e39cc

Collaborator

modular-magician commented Mar 10, 2020

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

Diff report:

Terraform GA: Diff ( 13 files changed, 4086 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 13 files changed, 4086 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1262 insertions(+))


          add copyright to app.yaml

df7c725

Collaborator

modular-magician commented Mar 10, 2020

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

Diff report:

Terraform GA: Diff ( 13 files changed, 4099 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 13 files changed, 4099 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1262 insertions(+))

megan07 requested a review from chrisst

March 10, 2020 17:15


          rm django zip

38828f3

Collaborator

modular-magician commented Mar 10, 2020

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

Diff report:

Terraform GA: Diff ( 12 files changed, 4099 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 12 files changed, 4099 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1262 insertions(+))

chrisst approved these changes

View reviewed changes

Contributor

chrisst left a comment

All my comments are test related at this point, nothing critical.

products/appengine/terraform.yaml

+                    entrypoint: !ruby/object:Overrides::Terraform::PropertyOverride
+                      ignore_read: true
+                    envVariables: !ruby/object:Overrides::Terraform::PropertyOverride
+                      ignore_read: true

Contributor

chrisst Mar 10, 2020

If these are things that can be updated by other tools after create you should override the description here indicating that Terraform won't detect drift.

third_party/terraform/tests/resource_app_engine_flexible_app_version_test.go

+                }
+                liveness_check {
+                  path = ""

Contributor

chrisst Mar 10, 2020

It would be nice to have more of these fields being updated if possible. Especially anything where the empty case makes sense. ie: if it makes sense to set a liveness_check.path and then remove it later by passing an empty string it should be set in the initial create and then empty string in a second update.

templates/terraform/examples/app_engine_flexible_app_version.tf.erb Outdated

+                delete_service_on_destroy = true
+              }
+              resource "google_app_engine_flexible_app_version" "myflexapp_v2" {

Contributor

chrisst Mar 10, 2020

nit: I have a slight preference for these to be 2 different example blocks instead of 2 app_versions in a single example. If possible I like to have each example be a single use case so that the example can have a title block like "NodeJs Example with scaling" and a user doesn't have to parse both whole resource blocks to understand the difference between the two. As pointed out by a user aws s3 bucket does a better job of this than many of our resources.

Contributor Author

megan07 Mar 11, 2020

I'm totally fine with that, I copied this from the standard app (again), and it appears both versions are the same. I'm guessing the example of it was just having 2 versions to the same service? But if that doesn't seem like a great thing to show in an example, I'm happy to remove v2.

Contributor

chrisst Mar 11, 2020

The only difference I saw was noop_on_destroy = true vs delete_service_on_destroy = true which doesn't seem like it's worth all of the wasted real estate on the docs page.

c2thorn approved these changes

View reviewed changes

Member

c2thorn left a comment

Sweeper looks good to me


          update after review comments

b8135b8

Collaborator

modular-magician commented Mar 11, 2020

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

Diff report:

Terraform GA: Diff ( 12 files changed, 4117 insertions(+), 14 deletions(-))
Terraform Beta: Diff ( 12 files changed, 4117 insertions(+), 14 deletions(-))
TF Conversion: Diff ( 1 file changed, 1262 insertions(+))

megan07 merged commit 1d0cc7c into GoogleCloudPlatform:master

This was referenced Mar 11, 2020

add app engine flex version GoogleCloudPlatform/terraform-google-conversion#388

Merged

add app engine flex version hashicorp/terraform-provider-google#5882

Merged

add app engine flex version hashicorp/terraform-provider-google-beta#1849

Merged

nathkn pushed a commit to nathkn/magic-modules that referenced this pull request


          add app engine flex version (GoogleCloudPlatform#3110)

0f93a39

* added flexible app version

* add sweeper file for flexible_app_version

* sweeper changes, add website link, etc

* add exclude to inspec.yaml

* add exclude to ansible.yaml

* missed

* sweeper review comments

* add test, other changes

* update test to flask, ignore read on beta_settings for now

* add copyright to app.yaml

* rm django zip

* update after review comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels