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

Discovery Engine Data Store #9682

Merged
merged 47 commits into from
Jan 12, 2024

Conversation

5fff
Copy link
Contributor

@5fff 5fff commented Dec 20, 2023

This is a draft PR for collaboration/debugging for Vertex AI Search & Conversation

google_discovery_engine_data_store

jialei-chen and others added 17 commits November 29, 2023 23:45
…enerated resource name would be google_discoveryengine_data_store
@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.

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

@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 ( 17 files changed, 1755 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 17 files changed, 1755 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 4 files changed, 492 insertions(+))
TF OiCS: Diff ( 4 files changed, 108 insertions(+))

Missing test report

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

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

resource "google_discoveryengine_data_store" "primary" {
  collection_id  = # value needed
  data_store_id  = # value needed
  solution_types = # value needed
}

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

resource "google_discoveryengine_data_store_iam_binding" "primary" {
  collection_id = # value needed
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
  data_store_id = # value needed
  location      = # value needed
  members       = # value needed
  role          = # value needed
}

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

resource "google_discoveryengine_data_store_iam_member" "primary" {
  collection_id = # value needed
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
  data_store_id = # value needed
  location      = # value needed
  member        = # value needed
  role          = # value needed
}

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

resource "google_discoveryengine_data_store_iam_policy" "primary" {
  collection_id = # value needed
  data_store_id = # value needed
  location      = # value needed
  policy_data   = # value needed
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3314
Passed tests 2974
Skipped tests: 336
Affected tests: 4

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDiscoveryengineDataStoreIamMemberGenerated|TestAccDiscoveryengineDataStore_discoveryengineDatastoreBasicExample|TestAccDiscoveryengineDataStoreIamPolicyGenerated|TestAccDiscoveryengineDataStoreIamBindingGenerated

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccDiscoveryengineDataStoreIamMemberGenerated[Error message] [Debug log]
TestAccDiscoveryengineDataStore_discoveryengineDatastoreBasicExample[Error message] [Debug log]
TestAccDiscoveryengineDataStoreIamPolicyGenerated[Error message] [Debug log]
TestAccDiscoveryengineDataStoreIamBindingGenerated[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

@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 ( 17 files changed, 1727 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 17 files changed, 1727 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 4 files changed, 474 insertions(+))
TF OiCS: Diff ( 4 files changed, 109 insertions(+))

Missing test report

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

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

resource "google_discoveryengine_data_store" "primary" {
  solution_types = # value needed
}

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

resource "google_discoveryengine_data_store_iam_binding" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

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

resource "google_discoveryengine_data_store_iam_member" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3314
Passed tests 2972
Skipped tests: 336
Affected tests: 6

Action taken

Found 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccBigqueryReservationBiReservation_update|TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample|TestAccDiscoveryengineDataStoreIamPolicyGenerated|TestAccDiscoveryengineDataStoreIamMemberGenerated|TestAccDiscoveryengineDataStoreIamBindingGenerated|TestAccDiscoveryengineDataStore_discoveryengineDatastoreBasicExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigqueryReservationBiReservation_update[Debug log]
TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample[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:}}$
TestAccDiscoveryengineDataStoreIamPolicyGenerated[Error message] [Debug log]
TestAccDiscoveryengineDataStoreIamMemberGenerated[Error message] [Debug log]
TestAccDiscoveryengineDataStoreIamBindingGenerated[Error message] [Debug log]
TestAccDiscoveryengineDataStore_discoveryengineDatastoreBasicExample[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

Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

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

Two minor comments, but otherwise LGTM

Also, we'll need to add a release note to the PR description before merging (I can override if needed), see https://googlecloudplatform.github.io/magic-modules/contribute/release-notes/.

@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 ( 14 files changed, 1102 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 14 files changed, 1102 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 99 insertions(+))
TF OiCS: Diff ( 4 files changed, 110 insertions(+))

@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 ( 14 files changed, 1092 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 14 files changed, 1092 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 99 insertions(+))
TF OiCS: Diff ( 4 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3348
Passed tests 3009
Skipped tests: 337
Affected tests: 2

Click here to see the affected service packages
all service packages are affected

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
TestAccDataprocClusterIamPolicy|TestAccDiscoveryEngineDataStore_discoveryengineDatastoreBasicExample_update

Get to know how VCR tests work

Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

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

Will merge after confirming tests pass

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]
TestAccDiscoveryEngineDataStore_discoveryengineDatastoreBasicExample_update[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

@roaks3 roaks3 merged commit 03f7549 into GoogleCloudPlatform:main Jan 12, 2024
13 checks passed
bskaplan pushed a commit to bskaplan/magic-modules that referenced this pull request Jan 17, 2024
* Create a configuration file product.yaml for product discoveryengine

* Create resource DataStore configuration file template DataStore.yaml

* Modify resource DataStore configuration file template to match the API resource's documented behavior

* Add each API resource field to configuration file properties attribute

* Add IAM support in Terraform to match DataStore API resource IAM support

* Polish field contents in product configuration file.

* Modify DataStore.yaml for LRO handling, parameters matching and readability improvement.

* Add a DataStore create test.

* Modify product name from DiscoveryEngine to Discoveryengine so that generated resource name would be google_discoveryengine_data_store

* change to GA in doc (GoogleCloudPlatform#9491)

Co-authored-by: Edward Sun <[email protected]>

* Add value of  to template into the generated tests to fix error 'fmt.Sprintf format %s reads arg GoogleCloudPlatform#3, but call has 2 args'.

* Remove Api::Product::ApiReference to match the change GoogleCloudPlatform#9536

* Add var collection_id to unit test examples in resource config.

* Fix argument missing error in unit tests.

* Update resource create unit test case fields to contain display_name, industry_vertical, content_config.

* Fix errors in acceptance tests: now acc test fails at invalid parent field of CreateDataStoreRequest.

* Set collection_id as default_collection.

* Remove uncessary variables under unit test example.

* Fix acceptance tests issues by (1) removing iam_policy and (2) adding import_format.

* Clean up configs: (1) update api version for tpg-beta to v1alpha; (2) remove fields for std Update method; (3) rename structured datastore example.

* Add examples and tests for all types of DataStore.

* Remove redundant field project from create test examples.

* Fix lint-yaml check failure.

* Add field solution_types to acceptance tests to satisfy coverage requirements.

* Clean up industry_vertical enum values.

* Add a new query parameter createAdvancedSiteSearch to create_url.

* Add a create test for advanced site search data store.

* Add IAM roles/discoveryengine.admin.

* Remove role config from .ci/infra/terraform/main.tf

* Remove beta version in product.yaml because both ga and beta versions have the same base_url.

* Rename product from Discoveryengine to DiscoveryEngine.

* Remove unnecessary configs and improve description readability.

* Mark property solution_types as immutable.

* Add update related settings and update test.

* Add delete_url for resource's Delete method.

* Remove all but one test example.

* Rename test example to make it more generic.

* Modify first template in update test to omit all non-required fields.

* Fix the update test: update display_name not data_store_id.

* Modify property description: remove brackets.

---------

Co-authored-by: Jialei Chen <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: jialei-chen <[email protected]>
@shuyama1
Copy link
Member

shuyama1 commented Jan 18, 2024

@roaks3 Coming from a PR I'm reviewing #9822, which adds a resource in the product discovery engine. I realize the product is implemented using the alpha API. So I want to double check if google and google-beta providers are the right places for this product.

@jialei-chen
Copy link
Member

@roaks3 Coming from a PR I'm reviewing #9822, which adds a resource in the product discovery engine. I realize the product is implemented using the alpha API. So I want to double check if google and google-beta providers are the right places for this product.

@shuyama1 @roaks3 Hi Shuya and Ryan, FYI
We brought the v1alpha API to v1 at cl/599283438. After the rollout, we may modify the implementation in this PR to use v1 API. As it takes some time for the rollout, I will confirm within the team about whether to update the API version in this iteration (< Jan 26, 2024) or move to the next one.

@roaks3
Copy link
Contributor

roaks3 commented Jan 18, 2024

I caught up with @shuyama1 over chat, and we've determined that this should be reverted with the v1alpha API. It was missed during initial review, and because alpha APIs typically carry the expectation that breaking changes are allowed, we do not support them in the public providers.

Then plan moving forward would be to revert this change (I will create the PR), and then once the v1 API is available, recreate this PR with the v1 API instead. Since this already went through review, it should be approved quickly, but the end result will likely be that this gets released a week or 2 later while waiting for the API to be deployed.

@jialei-chen If there are concerns about this timing, please let us know, and we can talk through alternatives.

@jialei-chen
Copy link
Member

@roaks3 Can we keep this change and update the version from v1alpha to v1 later, instead of directly rolling back? Because the product.yaml file is defined here, and the changes in #9822 and #9839 need to depend on it.

@roaks3
Copy link
Contributor

roaks3 commented Jan 18, 2024

We followed up over chat, and agreed to move forward with the revert. We understand that changing to v1 later could be safe in this case, but it still exposes our users to the risk of a breaking change, and we would like to avoid any sort of escalations that could come from that.

I will follow up on the other PRs to make sure they are not blocked by the revert.

kylase pushed a commit to yuanchuankee/magic-modules that referenced this pull request Jan 21, 2024
* Create a configuration file product.yaml for product discoveryengine

* Create resource DataStore configuration file template DataStore.yaml

* Modify resource DataStore configuration file template to match the API resource's documented behavior

* Add each API resource field to configuration file properties attribute

* Add IAM support in Terraform to match DataStore API resource IAM support

* Polish field contents in product configuration file.

* Modify DataStore.yaml for LRO handling, parameters matching and readability improvement.

* Add a DataStore create test.

* Modify product name from DiscoveryEngine to Discoveryengine so that generated resource name would be google_discoveryengine_data_store

* change to GA in doc (GoogleCloudPlatform#9491)

Co-authored-by: Edward Sun <[email protected]>

* Add value of  to template into the generated tests to fix error 'fmt.Sprintf format %s reads arg GoogleCloudPlatform#3, but call has 2 args'.

* Remove Api::Product::ApiReference to match the change GoogleCloudPlatform#9536

* Add var collection_id to unit test examples in resource config.

* Fix argument missing error in unit tests.

* Update resource create unit test case fields to contain display_name, industry_vertical, content_config.

* Fix errors in acceptance tests: now acc test fails at invalid parent field of CreateDataStoreRequest.

* Set collection_id as default_collection.

* Remove uncessary variables under unit test example.

* Fix acceptance tests issues by (1) removing iam_policy and (2) adding import_format.

* Clean up configs: (1) update api version for tpg-beta to v1alpha; (2) remove fields for std Update method; (3) rename structured datastore example.

* Add examples and tests for all types of DataStore.

* Remove redundant field project from create test examples.

* Fix lint-yaml check failure.

* Add field solution_types to acceptance tests to satisfy coverage requirements.

* Clean up industry_vertical enum values.

* Add a new query parameter createAdvancedSiteSearch to create_url.

* Add a create test for advanced site search data store.

* Add IAM roles/discoveryengine.admin.

* Remove role config from .ci/infra/terraform/main.tf

* Remove beta version in product.yaml because both ga and beta versions have the same base_url.

* Rename product from Discoveryengine to DiscoveryEngine.

* Remove unnecessary configs and improve description readability.

* Mark property solution_types as immutable.

* Add update related settings and update test.

* Add delete_url for resource's Delete method.

* Remove all but one test example.

* Rename test example to make it more generic.

* Modify first template in update test to omit all non-required fields.

* Fix the update test: update display_name not data_store_id.

* Modify property description: remove brackets.

---------

Co-authored-by: Jialei Chen <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: jialei-chen <[email protected]>
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
* Create a configuration file product.yaml for product discoveryengine

* Create resource DataStore configuration file template DataStore.yaml

* Modify resource DataStore configuration file template to match the API resource's documented behavior

* Add each API resource field to configuration file properties attribute

* Add IAM support in Terraform to match DataStore API resource IAM support

* Polish field contents in product configuration file.

* Modify DataStore.yaml for LRO handling, parameters matching and readability improvement.

* Add a DataStore create test.

* Modify product name from DiscoveryEngine to Discoveryengine so that generated resource name would be google_discoveryengine_data_store

* change to GA in doc (GoogleCloudPlatform#9491)

Co-authored-by: Edward Sun <[email protected]>

* Add value of  to template into the generated tests to fix error 'fmt.Sprintf format %s reads arg GoogleCloudPlatform#3, but call has 2 args'.

* Remove Api::Product::ApiReference to match the change GoogleCloudPlatform#9536

* Add var collection_id to unit test examples in resource config.

* Fix argument missing error in unit tests.

* Update resource create unit test case fields to contain display_name, industry_vertical, content_config.

* Fix errors in acceptance tests: now acc test fails at invalid parent field of CreateDataStoreRequest.

* Set collection_id as default_collection.

* Remove uncessary variables under unit test example.

* Fix acceptance tests issues by (1) removing iam_policy and (2) adding import_format.

* Clean up configs: (1) update api version for tpg-beta to v1alpha; (2) remove fields for std Update method; (3) rename structured datastore example.

* Add examples and tests for all types of DataStore.

* Remove redundant field project from create test examples.

* Fix lint-yaml check failure.

* Add field solution_types to acceptance tests to satisfy coverage requirements.

* Clean up industry_vertical enum values.

* Add a new query parameter createAdvancedSiteSearch to create_url.

* Add a create test for advanced site search data store.

* Add IAM roles/discoveryengine.admin.

* Remove role config from .ci/infra/terraform/main.tf

* Remove beta version in product.yaml because both ga and beta versions have the same base_url.

* Rename product from Discoveryengine to DiscoveryEngine.

* Remove unnecessary configs and improve description readability.

* Mark property solution_types as immutable.

* Add update related settings and update test.

* Add delete_url for resource's Delete method.

* Remove all but one test example.

* Rename test example to make it more generic.

* Modify first template in update test to omit all non-required fields.

* Fix the update test: update display_name not data_store_id.

* Modify property description: remove brackets.

---------

Co-authored-by: Jialei Chen <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: jialei-chen <[email protected]>
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
* Create a configuration file product.yaml for product discoveryengine

* Create resource DataStore configuration file template DataStore.yaml

* Modify resource DataStore configuration file template to match the API resource's documented behavior

* Add each API resource field to configuration file properties attribute

* Add IAM support in Terraform to match DataStore API resource IAM support

* Polish field contents in product configuration file.

* Modify DataStore.yaml for LRO handling, parameters matching and readability improvement.

* Add a DataStore create test.

* Modify product name from DiscoveryEngine to Discoveryengine so that generated resource name would be google_discoveryengine_data_store

* change to GA in doc (GoogleCloudPlatform#9491)

Co-authored-by: Edward Sun <[email protected]>

* Add value of  to template into the generated tests to fix error 'fmt.Sprintf format %s reads arg GoogleCloudPlatform#3, but call has 2 args'.

* Remove Api::Product::ApiReference to match the change GoogleCloudPlatform#9536

* Add var collection_id to unit test examples in resource config.

* Fix argument missing error in unit tests.

* Update resource create unit test case fields to contain display_name, industry_vertical, content_config.

* Fix errors in acceptance tests: now acc test fails at invalid parent field of CreateDataStoreRequest.

* Set collection_id as default_collection.

* Remove uncessary variables under unit test example.

* Fix acceptance tests issues by (1) removing iam_policy and (2) adding import_format.

* Clean up configs: (1) update api version for tpg-beta to v1alpha; (2) remove fields for std Update method; (3) rename structured datastore example.

* Add examples and tests for all types of DataStore.

* Remove redundant field project from create test examples.

* Fix lint-yaml check failure.

* Add field solution_types to acceptance tests to satisfy coverage requirements.

* Clean up industry_vertical enum values.

* Add a new query parameter createAdvancedSiteSearch to create_url.

* Add a create test for advanced site search data store.

* Add IAM roles/discoveryengine.admin.

* Remove role config from .ci/infra/terraform/main.tf

* Remove beta version in product.yaml because both ga and beta versions have the same base_url.

* Rename product from Discoveryengine to DiscoveryEngine.

* Remove unnecessary configs and improve description readability.

* Mark property solution_types as immutable.

* Add update related settings and update test.

* Add delete_url for resource's Delete method.

* Remove all but one test example.

* Rename test example to make it more generic.

* Modify first template in update test to omit all non-required fields.

* Fix the update test: update display_name not data_store_id.

* Modify property description: remove brackets.

---------

Co-authored-by: Jialei Chen <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: jialei-chen <[email protected]>
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.

8 participants