Add 'resources' reading and 'location_id' setting on Firebase Project #3319

yinzara · 2020-03-30T07:24:11Z

Signed-off-by: Matt Morrissette [email protected]

firebase: Added new `firebase_project_location` resource to finalize the firebase location.

yinzara · 2020-03-30T07:24:27Z

==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccFirebaseProject_firebaseProject -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN TestAccFirebaseProject_firebaseProjectBasicExample
=== PAUSE TestAccFirebaseProject_firebaseProjectBasicExample
=== RUN TestAccFirebaseProject_firebaseProjectFullExample
=== PAUSE TestAccFirebaseProject_firebaseProjectFullExample
=== CONT TestAccFirebaseProject_firebaseProjectBasicExample
=== CONT TestAccFirebaseProject_firebaseProjectFullExample
--- PASS: TestAccFirebaseProject_firebaseProjectBasicExample (33.91s)
--- PASS: TestAccFirebaseProject_firebaseProjectFullExample (117.91s)
PASS
ok github.com/terraform-providers/terraform-provider-google-beta/google-beta 118.428s

modular-magician · 2020-03-30T07:25:10Z

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.

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

modular-magician · 2020-03-30T07:29:25Z

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

Diff report:

Terraform GA: Diff ( 1 file changed, 3 insertions(+))
Terraform Beta: Diff ( 5 files changed, 256 insertions(+), 6 deletions(-))

rileykarson · 2020-03-30T16:34:22Z

@chrisst: Assigning review to you for your domain context, feel free to send back to me if needed and I can look deeper into it.

modular-magician · 2020-03-30T16:51:31Z

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

Diff report:

Terraform GA: Diff ( 1 file changed, 3 insertions(+))
Terraform Beta: Diff ( 5 files changed, 256 insertions(+), 6 deletions(-))

chrisst

Thank you for the contribution!

First of all there is a lot going on in this PR, so I would prefer if you were able to break it up a bit. The extra output fields are a pretty straight forward PR and could be merged in quickly.

I avoided adding the finalizeLocation API support because it's a singleton API with that is irreversible. It's worth putting in it's own PR at minimum and probably it's own resource. We often split resources apart when there are multiple endpoints on a single API with different payloads or different update logic. This allows us to keep custom code to a minimum which makes for more maintainable resources. We refer to these as "fine grained" resources.

I've added some quick thoughts as well so that we can reduce the number of round trips on this PR.

chrisst · 2020-03-30T17:04:35Z

third_party/terraform/data_sources/data_source_google_firebase_project.go.erb

+func dataSourceGoogleFirebaseProject() *schema.Resource {
+	return &schema.Resource{
+		Read: resourceFirebaseProjectRead,
+		Schema: map[string]*schema.Schema{


We prefer to keep the datasource and resource schema as close as possible so we can use datasourceSchemaFromResourceSchema to derive the schema instead of duplicating it.

The schemas are actually identical other than "Compute/Optional/Required/Description". I didn't even realize there was a function that handled that. Do I just use that function to derive the schema or can the entire data source be generated?

I don't mind splitting this into a separate PR if you'd prefer since it's really unrelated to adding the location_id.

The function is just to derive the schema. But if it makes sense you can also just call through to the "read" method of the resource too. It can keep the datasource very small in terms of code and also updates to the resource such as new fields don't have to be replicated. A example of this is the compute router datasource

chrisst · 2020-03-30T17:06:48Z

templates/terraform/extra_schema_entry/firebase_project.go.erb

@@ -0,0 +1,57 @@
+"resources": {


I don't understand why you have defined this schema both in the api.yaml and this extra schema. You should only need to define it in the api.yaml

The issue is that the "location_id" is really a computed property of the Firebase Project. The only way to change this property is to call the separate "projectLocation:finalize" endpoint. Since I want the "location_id" property to be "Optional" in Terraform but never actually used on create, there is no way I can express that in the api.yaml.

The only way was to mark the property as output in the api yaml (as is the resources Array) and then in the terraform.yaml, exclude the resources property all together and define custom code for the schema.

products/firebase/terraform.yaml

chrisst · 2020-03-30T17:14:26Z

templates/terraform/constants/firebase_project.go.erb

+		return nil
+	}
+	transformed := make(map[string]interface{})
+	transformed["hosting_site"] = original["hostingSite"]


The generated flatteners should already handle the renaming of the fields. I don't think you should need a custom flattener at all.

Unfortunately as stated above, if I mark the property as excluded in terraform.yaml, I had to define the flatteners in custom code. Those are actually the generated flatteners but just moved into custom code after I marked the resources property as excluded.

yinzara · 2020-03-30T18:54:26Z

So at a high level, you're saying instead of making the special "location_id" property handling, I should just leave it as computed and create a separate "google_firebase_project_location" fine grained resource with a location_id and project properties? I assume that would of course be a separate PR entirely.

If I do that, do you want the new output properties and the data source in two separate PRs or can those be combined and remain in this PR?

chrisst · 2020-03-30T19:52:25Z

At a high level - correct. This resource and datasource can continue to keep all the computed properties on them and just leave the location to be set by the new resource. I'm also happy to tackle this part if you don't have time.

yinzara · 2020-03-30T22:14:47Z

I think it's actually pretty straight forward if done that way (I don't know if there's even any custom code) so I think I can get it.

I've actually moved on to implementing the Firebase WebApp with the goals of getting the config of the webapp (specifically the apiKey and authDomain) to send to Cloud Build so that it can be set as a substitution during the build. The issue is that the "Config" is retrieved via a separate single GET API call to a "/config" endpoint after creating the WebApp.

I initially thought of creating a second resource WebAppConfig but realized that it would cause the generation of the associated resource when I only wanted a data source. I can't find a way of generating the "read" method of a resource without causing the provider.go to include it in the resources list.

I have a few thoughts,

generate the resource and copy the read function (and flatteners) into a new "firebase_web_app_config" datasource in the third_party/terraform/data_sources
Add a "config" property to the WebApp that is the properties I could have made the WebAppConfig resource which will generate the appropriate flatteners. Add custom code (I'm not sure which ones) to call the other endpoint during the read operation. Add an optional firebase_web_app_config data source to use generated functions.
?

yinzara · 2020-03-30T23:35:23Z

Ok so I forced pushed a new commit that removes all the Project changes and instead creates a new fine grained "google_firebase_project_location" resource with a "location_id" property.

==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccFirebaseProject -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccFirebaseProject_firebaseProjectBasicExample
=== PAUSE TestAccFirebaseProject_firebaseProjectBasicExample
=== RUN   TestAccFirebaseProjectLocation_firebaseProjectLocationBasicExample
=== PAUSE TestAccFirebaseProjectLocation_firebaseProjectLocationBasicExample
=== CONT  TestAccFirebaseProject_firebaseProjectBasicExample
=== CONT  TestAccFirebaseProjectLocation_firebaseProjectLocationBasicExample
--- PASS: TestAccFirebaseProject_firebaseProjectBasicExample (34.64s)
--- PASS: TestAccFirebaseProjectLocation_firebaseProjectLocationBasicExample (77.37s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google-beta/google-beta	77.941s

modular-magician · 2020-03-30T23:38:40Z

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

Diff report:

Terraform Beta: Diff ( 6 files changed, 378 insertions(+), 6 deletions(-))

Removed the custom decoder and replaced with the "fine grain" resource operator "nested_query". Updated the import_id so that importing wouldn't crash terraform

chrisst

This looks way cleaner. I added a commit to take advantage of one of the lesser known features of magic modules that we added recently. Let me know if it makes sense and I'll merge this in. Thanks!

modular-magician · 2020-03-31T18:05:20Z

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

Diff report:

Terraform Beta: Diff ( 6 files changed, 418 insertions(+), 6 deletions(-))

yinzara · 2020-04-01T00:39:42Z

Looks great. Ty!

…gleCloudPlatform#3319) * Add FirebaseLocation fine grained resource * Updates for firebase finalize location Removed the custom decoder and replaced with the "fine grain" resource operator "nested_query". Updated the import_id so that importing wouldn't crash terraform Co-authored-by: Chris Stephens <[email protected]>

googlebot added the cla: yes label Mar 30, 2020

modular-magician added the community label Mar 30, 2020

rileykarson requested a review from chrisst March 30, 2020 16:32

yinzara force-pushed the feature/firebase-project-location branch from 7f0dc0e to d14a641 Compare March 30, 2020 16:46

chrisst suggested changes Mar 30, 2020

View reviewed changes

chrisst self-assigned this Mar 30, 2020

Add FirebaseLocation fine grained resource

039f029

yinzara force-pushed the feature/firebase-project-location branch from d14a641 to 039f029 Compare March 30, 2020 23:33

Updates for firebase finalize location

7e3349c

Removed the custom decoder and replaced with the "fine grain" resource operator "nested_query". Updated the import_id so that importing wouldn't crash terraform

chrisst approved these changes Mar 31, 2020

View reviewed changes

chrisst merged commit 498bd8c into GoogleCloudPlatform:master Apr 1, 2020

chrisst deleted the feature/firebase-project-location branch April 1, 2020 03:10

modular-magician mentioned this pull request Apr 1, 2020

Add 'resources' reading and 'location_id' setting on Firebase Project hashicorp/terraform-provider-google-beta#1919

Merged

chrisst mentioned this pull request Apr 1, 2020

Add Support for Firebase hashicorp/terraform-provider-google#2973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'resources' reading and 'location_id' setting on Firebase Project #3319

Add 'resources' reading and 'location_id' setting on Firebase Project #3319

yinzara commented Mar 30, 2020 •

edited by chrisst

Loading

yinzara commented Mar 30, 2020

modular-magician commented Mar 30, 2020

modular-magician commented Mar 30, 2020

rileykarson commented Mar 30, 2020

modular-magician commented Mar 30, 2020

chrisst left a comment

chrisst Mar 30, 2020

yinzara Mar 30, 2020

yinzara Mar 30, 2020

chrisst Mar 30, 2020

chrisst Mar 30, 2020

yinzara Mar 30, 2020

chrisst Mar 30, 2020

yinzara Mar 30, 2020

yinzara commented Mar 30, 2020 •

edited

Loading

chrisst commented Mar 30, 2020

yinzara commented Mar 30, 2020 •

edited

Loading

yinzara commented Mar 30, 2020

modular-magician commented Mar 30, 2020

chrisst left a comment

modular-magician commented Mar 31, 2020

yinzara commented Apr 1, 2020

Add 'resources' reading and 'location_id' setting on Firebase Project #3319

Add 'resources' reading and 'location_id' setting on Firebase Project #3319

Conversation

yinzara commented Mar 30, 2020 • edited by chrisst Loading

yinzara commented Mar 30, 2020

modular-magician commented Mar 30, 2020

modular-magician commented Mar 30, 2020

Diff report:

rileykarson commented Mar 30, 2020

modular-magician commented Mar 30, 2020

Diff report:

chrisst left a 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

yinzara commented Mar 30, 2020 • edited Loading

chrisst commented Mar 30, 2020

yinzara commented Mar 30, 2020 • edited Loading

yinzara commented Mar 30, 2020

modular-magician commented Mar 30, 2020

Diff report:

chrisst left a comment

Choose a reason for hiding this comment

modular-magician commented Mar 31, 2020

Diff report:

yinzara commented Apr 1, 2020

yinzara commented Mar 30, 2020 •

edited by chrisst

Loading

yinzara commented Mar 30, 2020 •

edited

Loading

yinzara commented Mar 30, 2020 •

edited

Loading