-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 BlockingFunctionsConfig, RecaptchaConfig and QuotaConfig fields to identityplatform config #8402
Add BlockingFunctionsConfig, RecaptchaConfig and QuotaConfig fields to identityplatform config #8402
Conversation
Hello! I am a robot. It looks like you are a community contributor. @slevenick, 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. Terraform GA: Diff ( 3 files changed, 563 insertions(+)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
@slevenick This is a redo of PR/8064 after addressing @roaks3 's comments. |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules |
|
Temporarily enable VCR to run the tests. Also, provide a more user's friendly desc for the quota field.
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, 563 insertions(+), 1 deletion(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccIdentityPlatformConfig_identityPlatformConfigBasicExample|TestAccComputeFirewallPolicyRule_multipleRules |
|
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.
googleapi: Error 400: INVALID_CONFIG : SignUp quota must start between now and 365 days from now.
When this quota will take affect. | ||
- !ruby/object:Api::Type::String | ||
name: 'quotaDuration' | ||
description: | |
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.
Should we also add an example of how to structure the duration string? E.g.
A duration in seconds with up to nine fractional digits, terminated by 's'. Example: "3.5s". |
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, 570 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules |
|
Fix the quota start_time format.
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, 570 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules |
|
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.
Fails with:
Field 'startTime', Illegal timestamp format; timestamps must end with 'Z' or have a valid timezone offset."
Are you able to run the test yourself?
I got that error locally before I added the call to UTC(), which I assumed would fix the problem. I'll try a further fix locally and update the PR once it passes. |
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, 572 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten |
|
@slevenick Running the test locally passes all the field validation steps but produces the error below at the end. Perhaps it had to do with my env settings. Github is not showing the test on the list failed tests. Can you confirm? Error:
|
The test is skipped in VCR, so I have to run it myself. I think your local failure indicates a problem with how the resource works. I'm not sure exactly why though. |
I see. Thanks for running the test. I enabled VCR, let's see what the logs will reveal. |
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, 572 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccIdentityPlatformConfig_identityPlatformConfigBasicExample|TestAccContainerAwsNodePool_BetaBasicHandWritten |
Rerun these tests in REPLAYING mode to catch issues
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.
|
Error: Error updating Config "projects/tf-test-my-projectt1rb1914oe/config": Patch "https://identitytoolkit.googleapis.com/v2/projects/tf-test-my-projectt1rb1914oe/config?alt=json&updateMask=autodeleteAnonymousUsers%2CblockingFunctions%2Cquota%2CauthorizedDomains": Requested interaction not found Ah, I guess this is why VCR was skipped in the first place: hashicorp/terraform-provider-google#14158 It leaves me with no options other than tying to run the test locally. @slevenick I would appreciate it if you do the same. |
However, FWIW the test passed during recording mode. |
Re-enable skip_vcr due to hashicorp/terraform-provider-google#14158.
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, 572 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsNodePool_BetaBasicHandWritten |
|
…o identityplatform config (GoogleCloudPlatform#8402) * Add Add BlockingFunctionsConfig, AuthorizedDomains and QuotaConfig fields to Config.yaml * adding new fields to identity_platform_config_basic.tf.erb * Update Config.yaml Temporarily enable VCR to run the tests. Also, provide a more user's friendly desc for the quota field. * Fix the failing test * Update Config.yaml Fix the quota start_time format. * Attempt 2: Fix the failing test * Update Config.yaml Enabling VCR. * Update Config.yaml Re-enable skip_vcr due to hashicorp/terraform-provider-google#14158.
Add BlockingFunctionsConfig, AuthorizedDomains and QuotaConfig fields to identityplatform config
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)