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.
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
Add AppleApp to Firebase. #6630
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @megan07, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 127 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccCGCSnippet_eventarcWorkflowsExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
mmv1/templates/terraform/examples/firebase_apple_app_basic.tf.erb
Outdated
Show resolved
Hide resolved
project = google_project.default.project_id | ||
} | ||
|
||
resource "google_firebase_apple_app" "<%= ctx[:primary_resource_id] %>" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we likely want to have 2 samples in general:
- a minimal sample that sets only the minimally required fields
- a full sample that sets all optional values with example values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 - please also add an update test.
mmv1/templates/terraform/examples/firebase_apple_app_basic.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/templates/terraform/examples/firebase_apple_app_basic.tf.erb
Outdated
Show resolved
Hide resolved
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 115 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccComputeForwardingRule_update|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 115 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigQueryDataTable_bigtable |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 231 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 231 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking really good! Just a few minor cleanups noted below
mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…pdate_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…pdate_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
…pdate_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Thanks Stephen for the suggestions. All is done. My only concern is that I'm not sure if removing the test_vars_overrides section will pass the tests. As I mentioned before team_id and app_store_id are integers and moving them to the vars section will append their set values by random string in the test, and therefore fail our API validators. I tried this a few times before and always got "Error: Error creating AppleApp: googleapi: Error 400: Request contains an invalid argument". I decided to give it one more try and let's see how the tests go. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 7 files changed, 254 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccFirebaseAppleApp_update|TestAccFirebaseAppleApp_firebaseAppleAppBasicExample|TestAccFirebaseAppleApp_firebaseAppleAppFullExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
As expected, test failed because of the added random suffixes to these 2 variables. { 2022/11/11 21:06:06 [DEBUG] Google API Response Details: { |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 7 files changed, 254 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaseAppleApp_firebaseAppleAppFullExample|TestAccComputeForwardingRule_update |
@melinath is there anything blocking this PR at this point? |
LGTM - doing a manual test run just to be safe: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/353024 |
* Update api.yaml * Update api.yaml * Update api.yaml * Update api.yaml * Update terraform.yaml * Create firebase_apple_app_basic.tf.erb * Responding to tylerg-dev's comments * Fixing the failing TestAccFirebaseAppleApp_firebaseAppleAppBasicExample test * Add a handwritten sweeper to hard-delete the AppleApp * Fix typo in update_mask * Fix var name in test * Add delete request body * Change appId to app_id in the sweeper delete url * Use custom_delete for deleting the AppleApp resource * Update api.yaml * Remove the repeated block in terraform.api * Add a 5 sec in the post_delete template * Move the 5 sec delay to the custom delete code * Add "Optional" to the deletion_policy description. * Address melinath@ comments * Add a manual update test * Remove unnecessary randomness. Also, move attributes to the vars section so they show up in docs. * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Adding back the test_vars_overrides. Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
* Update api.yaml * Update api.yaml * Update api.yaml * Update api.yaml * Update terraform.yaml * Create firebase_apple_app_basic.tf.erb * Responding to tylerg-dev's comments * Fixing the failing TestAccFirebaseAppleApp_firebaseAppleAppBasicExample test * Add a handwritten sweeper to hard-delete the AppleApp * Fix typo in update_mask * Fix var name in test * Add delete request body * Change appId to app_id in the sweeper delete url * Use custom_delete for deleting the AppleApp resource * Update api.yaml * Remove the repeated block in terraform.api * Add a 5 sec in the post_delete template * Move the 5 sec delay to the custom delete code * Add "Optional" to the deletion_policy description. * Address melinath@ comments * Add a manual update test * Remove unnecessary randomness. Also, move attributes to the vars section so they show up in docs. * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Adding back the test_vars_overrides. Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
* Update api.yaml * Update api.yaml * Update api.yaml * Update api.yaml * Update terraform.yaml * Create firebase_apple_app_basic.tf.erb * Responding to tylerg-dev's comments * Fixing the failing TestAccFirebaseAppleApp_firebaseAppleAppBasicExample test * Add a handwritten sweeper to hard-delete the AppleApp * Fix typo in update_mask * Fix var name in test * Add delete request body * Change appId to app_id in the sweeper delete url * Use custom_delete for deleting the AppleApp resource * Update api.yaml * Remove the repeated block in terraform.api * Add a 5 sec in the post_delete template * Move the 5 sec delay to the custom delete code * Add "Optional" to the deletion_policy description. * Address melinath@ comments * Add a manual update test * Remove unnecessary randomness. Also, move attributes to the vars section so they show up in docs. * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/products/firebase/terraform.yaml Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Update mmv1/third_party/terraform/tests/resource_firebase_apple_app_update_test.go.erb Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com> * Adding back the test_vars_overrides. Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Add AppleApp resource to Firebase.
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)