-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Terraform config for Firebase App Hosting Backend resource #13040
base: main
Are you sure you want to change the base?
Conversation
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_firebase_app_hosting_backend" "primary" {
codebase {
repository = # value needed
root_directory = # value needed
}
managed_resources {
run_service {
service = # value needed
}
}
}
|
Note that |
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @roaks3, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_firebase_app_hosting_backend" "primary" {
codebase {
repository = # value needed
root_directory = # value needed
}
managed_resources {
run_service {
service = # value needed
}
}
}
|
f4be781
to
debde51
Compare
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_firebase_app_hosting_backend" "primary" {
codebase {
repository = # value needed
root_directory = # value needed
}
}
|
Tests analyticsTotal tests: 4588 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 4588 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Tests analyticsTotal tests: 4588 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_firebase_app_hosting_backend" "primary" {
codebase {
repository = # value needed
root_directory = # value needed
}
}
|
Tests analyticsTotal tests: 4592 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
@roaks3 this PR is ready for review. The other test failures seem to be irrelevant. |
@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 1 week. Please take a look! Use the label |
@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 2 weeks. Please take a look! Use the label |
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.
Code LGTM, but had a few test-related comments
@@ -0,0 +1,76 @@ | |||
# Service account setup only required once per project. |
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.
For this example, are you intending for users to use this as a reference and potentially copy-and-paste this config as a canonical example to work from? I just couldn't tell, but if it is not suitable for documentation, you may want to rework things so that it does serve that audience (and you may want test-only or docs-only examples).
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.
Yes, there are some setup that's required only once per project, as opposed to once per backend. It's common for users to just copy-pasta code, so I want them to at least copy everything they need.
project = "{{index $.TestEnvVars "project_id"}}" | ||
|
||
# Must be firebase-app-hosting-compute | ||
account_id = "firebase-app-hosting-compute" |
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.
This should use vars
so that it gets sweepable prefix/suffix added
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.
This actually must be firebase-app-hosting-compute
(see comment in the previous line). Otherwise, the experience in other surfaces (such as the Firebase Console UI) will be broken. This is publicly documented at https://firebase.google.com/support/guides/service-accounts
display_name = "Firebase App Hosting compute service account" | ||
|
||
# Do not throw if already exists | ||
create_ignore_already_exists = true |
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.
What is the purpose of including this? Tests should be deleting this account when they complete
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.
See comment above (we have to use this name so sweepers can't reach it).
disable_on_destroy = false | ||
} | ||
|
||
resource "google_firebase_app_hosting_backend" "example" { |
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.
This being the only test I see 2 main issues:
- We never check if optional fields can be omitted. This would normally be done by having one of the test configs be minimal (only required fields), and another with all possible fields specified.
- We never check if mutable fields can be changed (like app_id, service_account, etc.). Unless there is some limitation, we should use an update test for them.
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.
Right, added.
@@ -0,0 +1,70 @@ | |||
resource "google_firebase_app_hosting_backend" "example" { |
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.
Since these are both going to be shown in the docs as examples, we should be consistent with the ordering ie. dependencies-first or dependencies-last. I think dependencies-last is more common, but either should be fine.
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_firebase_app_hosting_backend" "primary" {
codebase {
repository = # value needed
root_directory = # value needed
}
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. |
Tests analyticsTotal tests: 4647 Click here to see the affected service packages
Action takenFound 11 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
766d8aa
to
bdaee54
Compare
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_firebase_app_hosting_backend" "primary" {
codebase {
repository = # value needed
root_directory = # value needed
}
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. |
Tests analyticsTotal tests: 4647 Click here to see the affected service packages
Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
bdaee54
to
5ac6ba8
Compare
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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_firebase_app_hosting_backend" "primary" {
codebase {
repository = # value needed
root_directory = # value needed
}
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. |
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.