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

Promote Gemini Data Sharing With Google Setting Binding from google-beta to google #13121

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

PerlMonker303
Copy link
Contributor

@PerlMonker303 PerlMonker303 commented Feb 19, 2025

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

Promote resources for Gemini Admin Control

`google_gemini_data_sharing_with_google_setting_binding` (GA)
gemini: fixed permadiff on `product` field in `google_gemini_data_sharing_with_google_setting_binding` resource (Beta)

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

google provider: Diff ( 5 files changed, 692 insertions(+), 6 deletions(-))
google-beta provider: Diff ( 2 files changed, 1 insertion(+), 11 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 17
Passed tests: 17
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • gemini
#### Non-exercised tests

🔴 Tests were added that are GA-only additions and require manual runs:

  • TestAccGeminiDataSharingWithGoogleSettingBinding_update
    🟢 All tests passed!

View the build log

@PerlMonker303 PerlMonker303 marked this pull request as ready for review February 19, 2025 10:44
@github-actions github-actions bot requested a review from shuyama1 February 19, 2025 10:45
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

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

@shuyama1
Copy link
Member

shuyama1 commented Feb 20, 2025

Running GA tests on TC. I'll update test result once it's done

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

Test failed due to error:

resource_gemini_data_sharing_with_google_setting_binding_test.go:23: Step 1/4 error: After applying this test step, the plan was not empty.
        stdout:
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        Terraform will perform the following actions:
          # google_gemini_data_sharing_with_google_setting_binding.basic_binding will be updated in-place
          ~ resource "google_gemini_data_sharing_with_google_setting_binding" "basic_binding" {
                id                                  = "projects/ci-test-project-188019/locations/global/dataSharingWithGoogleSettings/tf-test-ls-anbpja9s28/settingBindings/tf-test-lsb-zpgyv39zxx"
                name                                = "projects/ci-test-project-188019/locations/global/dataSharingWithGoogleSettings/tf-test-ls-anbpja9s28/settingBindings/tf-test-lsb-zpgyv39zxx"
              - product                             = "GEMINI_CLOUD_ASSIST" -> null
                # (9 unchanged attributes hidden)
            }
        Plan: 0 to add, 1 to change, 0 to destroy.

I've checked our nightly tests and it seems the test has failed with the same error in our beta environment. Let's fix that in this PR.

The diff is likely due to the API returns default value for unset field product. https://googlecloudplatform.github.io/magic-modules/develop/diffs/#fix-diffs

@github-actions github-actions bot requested a review from shuyama1 February 20, 2025 18:24
@PerlMonker303
Copy link
Contributor Author

I have encountered the error before, but it is weird how similar PRs did not have this error and they have identical setups - see #13051 and #13050.

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

google provider: Diff ( 5 files changed, 692 insertions(+), 6 deletions(-))
google-beta provider: Diff ( 2 files changed, 2 insertions(+), 12 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 17
Passed tests: 16
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • gemini
#### Non-exercised tests

🔴 Tests were added that are GA-only additions and require manual runs:

  • TestAccGeminiDataSharingWithGoogleSettingBinding_update

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
  • TestAccGeminiDataSharingWithGoogleSettingBinding_update

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🔴 Tests failed during RECORDING mode:
TestAccGeminiDataSharingWithGoogleSettingBinding_update [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

I have encountered the error before, but it is weird how similar PRs did not have this error and they have identical setups - see #13051 and #13050.

I think the issue occurs in google_gemini_data_sharing_with_google_setting_binding specifically. Modifying the test config would not fix the issue. This diff happens when the field is not explicitly specified but the API returns value. You'll need to add default_value: DEFAULT_VALUE or default_from_api: true to the field depends on if it has a fixed default value. See details at https://googlecloudplatform.github.io/magic-modules/develop/diffs/#default

@github-actions github-actions bot requested a review from shuyama1 February 20, 2025 21:26
@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.

google provider: Diff ( 5 files changed, 693 insertions(+), 6 deletions(-))
google-beta provider: Diff ( 3 files changed, 2 insertions(+), 11 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 17
Passed tests: 17
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • gemini
#### Non-exercised tests

🔴 Tests were added that are GA-only additions and require manual runs:

  • TestAccGeminiDataSharingWithGoogleSettingBinding_update
    🟢 All tests passed!

View the build log

@PerlMonker303
Copy link
Contributor Author

Added a fix after testing locally. This should work.

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

google provider: Diff ( 5 files changed, 693 insertions(+), 6 deletions(-))
google-beta provider: Diff ( 3 files changed, 3 insertions(+), 12 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 17
Passed tests: 16
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • gemini
#### Non-exercised tests

🔴 Tests were added that are GA-only additions and require manual runs:

  • TestAccGeminiDataSharingWithGoogleSettingBinding_update

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
  • TestAccGeminiDataSharingWithGoogleSettingBinding_update

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccGeminiDataSharingWithGoogleSettingBinding_update [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🟢 All tests passed!

View the build log or the debug log for each test

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

Added a fix after testing locally. This should work.

Thank you for updating the tests. I'd prefer we have the update test set as before - product field unset for creation and change it to a non-default value for update.
Running the previous test in GA, we got the following error:

=== RUN   TestAccGeminiDataSharingWithGoogleSettingBinding_update
=== PAUSE TestAccGeminiDataSharingWithGoogleSettingBinding_update
=== CONT  TestAccGeminiDataSharingWithGoogleSettingBinding_update
    resource_gemini_data_sharing_with_google_setting_binding_test.go:23: Step 3/4 error: Error running apply: exit status 1
        Error: Error waiting for Updating DataSharingWithGoogleSettingBinding: Error code 3, message: product "GEMINI_CODE_ASSIST" is not supported by the setting type "dataSharingWithGoogleSettings", allowed product is "GEMINI_CLOUD_ASSIST": invalid argument
          with google_gemini_data_sharing_with_google_setting_binding.basic_binding,
          on terraform_plugin_test.tf line 12, in resource "google_gemini_data_sharing_with_google_setting_binding" "basic_binding":
          12: resource "google_gemini_data_sharing_with_google_setting_binding" "basic_binding" {
--- FAIL: TestAccGeminiDataSharingWithGoogleSettingBinding_update (39.27s)

Is the GEMINI_CLOUD_ASSIST currently no supported? or is the value only supported in beta?

@github-actions github-actions bot requested a review from shuyama1 February 21, 2025 22:18
@PerlMonker303
Copy link
Contributor Author

"GEMINI_CLOUD_ASSIST" should be supported. I have changed the tests now, let me know if it passes.

@shuyama1
Copy link
Member

shuyama1 commented Feb 21, 2025

GEMINI_CODE_ASSIST

Sorry, I mean "GEMINI_CODE_ASSIST" (the non-default value). Is that supported? as we set it before in the test and the GA failure indicated that it was not supported. If not, we may want to remove it from possible value for ga resource.

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

google provider: Diff ( 5 files changed, 694 insertions(+), 6 deletions(-))
google-beta provider: Diff ( 3 files changed, 4 insertions(+), 12 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 18
Passed tests: 18
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • gemini
#### Non-exercised tests

🔴 Tests were added that are GA-only additions and require manual runs:

  • TestAccGeminiDataSharingWithGoogleSettingBinding_update
    🟢 All tests passed!

View the build log

@github-actions github-actions bot requested a review from shuyama1 February 21, 2025 22:46
@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.

google provider: Diff ( 5 files changed, 693 insertions(+), 6 deletions(-))
google-beta provider: Diff ( 3 files changed, 3 insertions(+), 12 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 18
Passed tests: 17
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages
  • gemini
#### Non-exercised tests

🔴 Tests were added that are GA-only additions and require manual runs:

  • TestAccGeminiDataSharingWithGoogleSettingBinding_update

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
  • TestAccGeminiDataSharingWithGoogleSettingBinding_update

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccGeminiDataSharingWithGoogleSettingBinding_update [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🟢 All tests passed!

View the build log or the debug log for each test

@PerlMonker303
Copy link
Contributor Author

Sorry for the confusion, you are right. GEMINI_CODE_ASSIST is not supported for the product field for this resource. I removed it from possible values. I think things should be good to go now.

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

google provider: Diff ( 5 files changed, 694 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 3 files changed, 6 insertions(+), 15 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 18
Passed tests: 18
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • gemini
#### Non-exercised tests

🔴 Tests were added that are GA-only additions and require manual runs:

  • TestAccGeminiDataSharingWithGoogleSettingBinding_update
    🟢 All tests passed!

View the build log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants