Skip to content
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 client, permissions, monitoring and mfa configs to google_identity_platform_config #9609

Merged
merged 16 commits into from
Feb 8, 2024

Conversation

gleichda
Copy link
Contributor

@gleichda gleichda commented Dec 8, 2023

Add client, permissions, monitoring and mfa configs to google_identity_platform_config

For client this also fixes the diff that was introduced in #9417 and reverted in #9447

Fixes hashicorp/terraform-provider-google/issues/14192
Fixes hashicorp/terraform-provider-google/issues/15712
Fixes hashicorp/terraform-provider-google/issues/13326
Fixes hashicorp/terraform-provider-google/issues/17063
Relates to: hashicorp/terraform-provider-google/issues/14194

Release Note Template for Downstream PRs (will be copied)

identityplatform: added `client`, `permissions`, `monitoring` and `mfa` fields to `google_identity_platform_config`

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@zli82016, 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.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 588 insertions(+), 7 deletions(-))
Terraform Beta: Diff ( 3 files changed, 588 insertions(+), 7 deletions(-))
TF Conversion: Diff ( 1 file changed, 269 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_identity_platform_config (4 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_identity_platform_config" "primary" {
  client {
    api_key            = # value needed
    firebase_subdomain = # value needed
  }
  mfa {
    enabled_providers = # value needed
    provider_configs {
      state = # value needed
      totp_provider_config {
        adjacent_intervals = # value needed
      }
    }
    state = # value needed
  }
  monitoring {
    request_logging {
      enabled = # value needed
    }
  }
  multi_tenant {
    allow_tenants           = # value needed
    default_tenant_location = # value needed
  }
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3275
Passed tests 2941
Skipped tests: 334
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@zli82016
Copy link
Member

zli82016 commented Dec 8, 2023

Also, can you please add the new fields to the update test to make sure updating the new fields is working? Thanks.

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/services/identityplatform/resource_identity_platform_config_test.go

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 609 insertions(+), 7 deletions(-))
Terraform Beta: Diff ( 3 files changed, 609 insertions(+), 7 deletions(-))
TF Conversion: Diff ( 1 file changed, 269 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_identity_platform_config (4 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_identity_platform_config" "primary" {
  client {
    api_key            = # value needed
    firebase_subdomain = # value needed
  }
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3290
Passed tests 2951
Skipped tests: 337
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy|TestAccIdentityPlatformConfig_identityPlatformConfigMinimalExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccIdentityPlatformConfig_identityPlatformConfigMinimalExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@gleichda gleichda marked this pull request as draft December 13, 2023 17:28
@gleichda gleichda marked this pull request as ready for review December 14, 2023 22:42
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 666 insertions(+), 13 deletions(-))
Terraform Beta: Diff ( 4 files changed, 666 insertions(+), 13 deletions(-))
TF Conversion: Diff ( 1 file changed, 269 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_identity_platform_config (4 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_identity_platform_config" "primary" {
  client {
    api_key            = # value needed
    firebase_subdomain = # value needed
  }
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3296
Passed tests 2958
Skipped tests: 337
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccIdentityPlatformConfig_identityPlatformConfigMinimalExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccIdentityPlatformConfig_identityPlatformConfigMinimalExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

name: 'client'
description: |
Options related to how clients making requests on behalf of a project should be configured.
ignore_read: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what is the reason to add ignore_read ?

Copy link
Contributor Author

@gleichda gleichda Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore read is added here because the API always returns the default values for multiple fields.

resource "google_identity_platform_config" "default" {
  project                    = "MYPROJECTID"
}

produces the following TF debug snippet:

2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: 2023/12/15 14:09:53 [DEBUG] Google API Request Details:
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: ---[ REQUEST ]---------------------------------------
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: GET /v2/projects/MYPROJECTID/config?alt=json HTTP/1.1
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: Host: identitytoolkit.googleapis.com
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: User-Agent: Terraform/1.6.1 (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/dev
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: Content-Type: application/json
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: X-Goog-User-Project: gleichda-xyz-demo
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: Accept-Encoding: gzip
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google
2023-12-15T14:09:53.082Z [DEBUG] provider.terraform-provider-google: -----------------------------------------------------
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: 2023/12/15 14:09:53 [DEBUG] Google API Response Details:
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: ---[ RESPONSE ]--------------------------------------
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: HTTP/2.0 200 OK
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Cache-Control: no-cache, no-store, max-age=0, must-revalidate
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Content-Type: application/json; charset=UTF-8
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Date: Fri, 15 Dec 2023 14:09:53 GMT
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Expires: Mon, 01 Jan 1990 00:00:00 GMT
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Pragma: no-cache
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Server: ESF
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Vary: Origin
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Vary: X-Origin
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: Vary: Referer
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: X-Content-Type-Options: nosniff
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: X-Frame-Options: SAMEORIGIN
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: X-Xss-Protection: 0
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "name": "projects/141423373225/config",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "signIn": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "email": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "enabled": true
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "phoneNumber": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "enabled": true,
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "testPhoneNumbers": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "+11231231234": "000000"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       }
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "anonymous": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "enabled": true
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "allowDuplicateEmails": true,
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "hashConfig": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "algorithm": "SCRYPT",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "signerKey": "ocDgh1Kx2zrbQrzKZjKeQ5J0Cl4RALqSBBPi2+ZheygcJ75wsWc9uBWvokCWz8GrSLnhtEXfN+4s4S1Zzgz09A==",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "saltSeparator": "Bw==",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "rounds": 8,
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "memoryCost": 14
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     }
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "notification": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "sendEmail": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "method": "DEFAULT",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "resetPasswordTemplate": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "senderLocalPart": "noreply",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "subject": "Reset your password for %APP_NAME%",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "body": "\u003cp\u003eHello,\u003c/p\u003e\n\u003cp\u003eFollow this link to reset your %APP_NAME% password for your %EMAIL% account.\u003c/p\u003e\n\u003cp\u003e\u003ca href='%LINK%'\u003e%LINK%\u003c/a\u003e\u003c/p\u003e\n\u003cp\u003eIf you didn’t ask to reset your password, you can ignore this email.\u003c/p\u003e\n\u003cp\u003eThanks,\u003c/p\u003e\n\u003cp\u003eYour %APP_NAME% team\u003c/p\u003e",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "bodyFormat": "HTML",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "replyTo": "noreply"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "verifyEmailTemplate": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "senderLocalPart": "noreply",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "subject": "Verify your email for %APP_NAME%",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "body": "\u003cp\u003eHello %DISPLAY_NAME%,\u003c/p\u003e\n\u003cp\u003eFollow this link to verify your email address.\u003c/p\u003e\n\u003cp\u003e\u003ca href='%LINK%'\u003e%LINK%\u003c/a\u003e\u003c/p\u003e\n\u003cp\u003eIf you didn’t ask to verify this address, you can ignore this email.\u003c/p\u003e\n\u003cp\u003eThanks,\u003c/p\u003e\n\u003cp\u003eYour %APP_NAME% team\u003c/p\u003e",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "bodyFormat": "HTML",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "replyTo": "noreply"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "changeEmailTemplate": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "senderLocalPart": "noreply",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "subject": "Your sign-in email was changed for %APP_NAME%",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "body": "\u003cp\u003eHello %DISPLAY_NAME%,\u003c/p\u003e\n\u003cp\u003eYour sign-in email for %APP_NAME% was changed to %NEW_EMAIL%.\u003c/p\u003e\n\u003cp\u003eIf you didn’t ask to change your email, follow this link to reset your sign-in email.\u003c/p\u003e\n\u003cp\u003e\u003ca href='%LINK%'\u003e%LINK%\u003c/a\u003e\u003c/p\u003e\n\u003cp\u003eThanks,\u003c/p\u003e\n\u003cp\u003eYour %APP_NAME% team\u003c/p\u003e",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "bodyFormat": "HTML",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "replyTo": "noreply"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "callbackUri": "https://MYPROJECTID.firebaseapp.com/__/auth/action",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "dnsInfo": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "customDomainState": "NOT_STARTED",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "domainVerificationRequestTime": "1970-01-01T00:00:00Z"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "revertSecondFactorAdditionTemplate": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "senderLocalPart": "noreply",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "subject": "You've added 2 step verification to your %APP_NAME% account.",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "body": "\u003cp\u003eHello %DISPLAY_NAME%,\u003c/p\u003e\n\u003cp\u003eYour account in %APP_NAME% has been updated with %SECOND_FACTOR% for 2-step verification.\u003c/p\u003e\n\u003cp\u003eIf you didn't add this 2-step verification, click the link below to remove it.\u003c/p\u003e\n\u003cp\u003e\u003ca href='%LINK%'\u003e%LINK%\u003c/a\u003e\u003c/p\u003e\n\u003cp\u003eThanks,\u003c/p\u003e\n\u003cp\u003eYour %APP_NAME% team\u003c/p\u003e",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "bodyFormat": "HTML",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "replyTo": "noreply"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       }
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "sendSms": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       "smsTemplate": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:         "content": "%LOGIN_CODE% is your verification code for %APP_NAME%."
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:       }
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "defaultLocale": "en"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "quota": {},
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "monitoring": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "requestLogging": {}
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "multiTenant": {},
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "authorizedDomains": [
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "localhost",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "MYPROJECTID.firebaseapp.com",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: "MYPROJECTID.web.app"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   ],
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "subtype": "IDENTITY_PLATFORM",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "client": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "apiKey": "REDACTED",
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "permissions": {},
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "firebaseSubdomain": "MYPROJECTID"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "mfa": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "state": "DISABLED"
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   },
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "blockingFunctions": {},
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "smsRegionConfig": {},
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "autodeleteAnonymousUsers": true,
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   "emailPrivacyConfig": {
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:     "enableImprovedEmailPrivacy": true
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google:   }
2023-12-15T14:09:53.312Z [DEBUG] provider.terraform-provider-google: }
2023-12-15T14:09:53.313Z [DEBUG] provider.terraform-provider-google

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the information. Then default_from_api should be used here instead of ignore_read

name: 'mfa'
description: |
Options related to how clients making requests on behalf of a project should be configured.
ignore_read: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what is the reason to add ignore_read ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as in #9609 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default_from_api should be used here instead of ignore_read

name: 'monitoring'
description: |
Configuration related to monitoring project activity.
ignore_read: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what is the reason to add ignore_read ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as in #9609 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default_from_api should be used here instead of ignore_read

@zli82016 zli82016 requested review from a team and slevenick and removed request for zli82016 and a team January 24, 2024 17:43
@zli82016
Copy link
Member

zli82016 commented Jan 24, 2024

Passing this PR to @slevenick , as I am going to be OOO. Thanks, @slevenick .

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 829 insertions(+), 10 deletions(-))
Terraform Beta: Diff ( 4 files changed, 829 insertions(+), 10 deletions(-))
TF Conversion: Diff ( 1 file changed, 272 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 16
Passed tests 14
Skipped tests: 2
Affected tests: 0

Click here to see the affected service packages
  • identityplatform

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@gleichda
Copy link
Contributor Author

gleichda commented Feb 7, 2024

@slevenick Finally made it to finish the PR PTAL
(tagging you explicitly as I can't request a review)

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small questions on the import state verify stuff.

Tests pass, so we should be good to go

ResourceName: "google_identity_platform_config.basic",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"client", "mfa", "monitoring"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need ignore state verify here? If these are default_from_api it should just accept the server's value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed/or at least reduced to the outputs.
Without the outputs the test fail:

=== NAME  TestAccIdentityPlatformConfig_identityPlatformConfigBasicExample
    vcr_utils.go:152: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) (len=1) {
         (string) (len=27) "client.0.firebase_subdomain": (string) (len=28) "tf-test-my-projectswbrzvojnv"
        }


        (map[string]string) (len=1) {
         (string) (len=27) "client.0.firebase_subdomain": (string) ""
        }

I'm not sure where this comes from but the actual expected is returned from the API therefor excluding this was my solution but happy to learn how to properly fix this

@@ -49,6 +49,9 @@ examples:
test_vars_overrides:
# Set quota start time for the following day.
quota_start_time: 'time.Now().AddDate(0, 0, 1).Format(time.RFC3339)'
ignore_read_extra:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need ignore_read_extra for output-only values I think?

Copy link
Contributor Author

@gleichda gleichda Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above:
without the ingore read for the outputs the test fails: (This also feels a little bit flaky not happening every time but regularly)

=== NAME  TestAccIdentityPlatformConfig_identityPlatformConfigBasicExample
    vcr_utils.go:152: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) (len=1) {
         (string) (len=27) "client.0.firebase_subdomain": (string) (len=28) "tf-test-my-projectswbrzvojnv"
        }


        (map[string]string) (len=1) {
         (string) (len=27) "client.0.firebase_subdomain": (string) ""
        }
        ```

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 833 insertions(+), 13 deletions(-))
Terraform Beta: Diff ( 4 files changed, 833 insertions(+), 13 deletions(-))
TF Conversion: Diff ( 1 file changed, 272 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 16
Passed tests 14
Skipped tests: 2
Affected tests: 0

Click here to see the affected service packages
  • identityplatform

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@slevenick slevenick merged commit 6029bee into GoogleCloudPlatform:main Feb 8, 2024
13 checks passed
@gleichda gleichda deleted the gcip branch February 14, 2024 13:48
tdbhacks pushed a commit to tdbhacks/magic-modules that referenced this pull request Feb 23, 2024
…y_platform_config (GoogleCloudPlatform#9609)

* Reapply "Add client config and permissions to google_identity_platform_config (GoogleCloudPlatform#9417)"

This reverts commit 1c1f4d2.

* Fix diff from API for GCIP client

* Add mfa config to GCIP

* Add multiTenant to google_identity_platform_config

* Add monitoring and request Logging to google_identity_platform_config

* Add test cases

* Extend update test to the new attributes

* Findings from review

* First set of review findings from zli82016

* Fix monitoring permadiff via custom flatten

* Fix client.permissions permadiff via custom flatten

* Fix mfa.state permadiff

* Adapt tests to latest changes

* Ignore only outputs for the import verify
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request Apr 19, 2024
…y_platform_config (GoogleCloudPlatform#9609)

* Reapply "Add client config and permissions to google_identity_platform_config (GoogleCloudPlatform#9417)"

This reverts commit 1c1f4d2.

* Fix diff from API for GCIP client

* Add mfa config to GCIP

* Add multiTenant to google_identity_platform_config

* Add monitoring and request Logging to google_identity_platform_config

* Add test cases

* Extend update test to the new attributes

* Findings from review

* First set of review findings from zli82016

* Fix monitoring permadiff via custom flatten

* Fix client.permissions permadiff via custom flatten

* Fix mfa.state permadiff

* Adapt tests to latest changes

* Ignore only outputs for the import verify
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
…y_platform_config (GoogleCloudPlatform#9609)

* Reapply "Add client config and permissions to google_identity_platform_config (GoogleCloudPlatform#9417)"

This reverts commit 1c1f4d2.

* Fix diff from API for GCIP client

* Add mfa config to GCIP

* Add multiTenant to google_identity_platform_config

* Add monitoring and request Logging to google_identity_platform_config

* Add test cases

* Extend update test to the new attributes

* Findings from review

* First set of review findings from zli82016

* Fix monitoring permadiff via custom flatten

* Fix client.permissions permadiff via custom flatten

* Fix mfa.state permadiff

* Adapt tests to latest changes

* Ignore only outputs for the import verify
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
…y_platform_config (GoogleCloudPlatform#9609)

* Reapply "Add client config and permissions to google_identity_platform_config (GoogleCloudPlatform#9417)"

This reverts commit 1c1f4d2.

* Fix diff from API for GCIP client

* Add mfa config to GCIP

* Add multiTenant to google_identity_platform_config

* Add monitoring and request Logging to google_identity_platform_config

* Add test cases

* Extend update test to the new attributes

* Findings from review

* First set of review findings from zli82016

* Fix monitoring permadiff via custom flatten

* Fix client.permissions permadiff via custom flatten

* Fix mfa.state permadiff

* Adapt tests to latest changes

* Ignore only outputs for the import verify
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants