-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Firebase hosting siteconfig #6790
Firebase hosting siteconfig #6790
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @rileykarson, 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. |
This PR is blocked on #6598, putting in draft state for now |
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 ( 3 files changed, 260 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|TestAccFirebaseHostingSite_firebasehostingSiteUpdate|TestAccFirebaseHostingSite_firebasehostingSiteFullExample|TestAccFirebaseHostingSite_firebasehostingSiteBasicExample|TestAccFirebaseHostingSiteConfig_firebasehostingSiteconfigFullExample|TestAccFirebaseHostingSiteConfig_firebasehostingSiteconfigBasicExample |
Tests passed during RECORDING mode: All tests passed |
1cc58d5
to
309f733
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. Terraform GA: Diff ( 1 file changed, 130 insertions(+)) |
309f733
to
6d3c8ac
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. Terraform GA: Diff ( 1 file changed, 130 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 |
620a119
to
f9a98da
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. Terraform GA: Diff ( 1 file changed, 126 insertions(+)) |
Tests analyticsTotal tests: All tests passed in REPLAYING mode |
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 ( 1 file changed, 124 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeForwardingRule_update |
6a4739c
to
a7e9a82
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. Terraform GA: Diff ( 1 file changed, 115 insertions(+)) |
Tests analyticsTotal tests: All tests passed in REPLAYING mode |
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.
Made a pass, comments inline!
Any idea why the config methods don't show up in cloud.google.com? I checked both prod & staging and they're not there, although they appear in discovery docs.
description: The default URL for the site in the form of https://{name}.web.app | ||
- !ruby/object:Api::Resource | ||
name: 'SiteConfig' |
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.
Do you mind adding references
linking to https://firebase.devsite.corp.google.com/docs/reference/hosting/rest/v1beta1/projects.sites? Thanks!
getConfig
would be preferred, but it doesn't seem to appear in c.g.c docs yet.
create_url: sites/{{site_id}}/config?update_mask=maxVersions,cloudLoggingEnabled # SiteConfig is created as part of Site creation, treat as an update | ||
create_verb: :PATCH | ||
update_verb: :PATCH |
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.
Given we support update we'll need a handwritten update test (https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/README.md#update-tests). I'm seeing our current generated resource documentation doesn't say to do so. I'll fix that.
test_env_vars: | ||
project_id: :PROJECT_NAME | ||
- !ruby/object:Provider::Terraform::Examples | ||
name: "firebasehosting_siteconfig_full" |
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.
nit: Given how small the resource is, let's just consolidate the _full
test into the basic
test.
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 SiteConfig is a singleton child of Site, do you mean to have two Sites in one file? Which one should be the primary_resource_id
then?
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.
We can just test all the fields in _basic
and delete the _full
example!
Good callout on the public docs. I just learned that SiteConfig is semi-internal/deprecated. I'm going to close this PR for now. |
Add resource google_firebase_hosting_site_config
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)