-
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
feat(vertexai): add vertex_ai_index_endpoint for Vertex AI Matching Engine #6873
feat(vertexai): add vertex_ai_index_endpoint for Vertex AI Matching Engine #6873
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @roaks3, 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. |
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 ( 5 files changed, 869 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccVertexAIIndexEndpoint_vertexAiIndexEndpointExample|TestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease|TestAccDataSourceGoogleServiceAccountIdToken_impersonation |
Tests passed during RECORDING mode: All tests passed |
@roaks3 All tests passed. When you have time, could you please revive this PR? Thanks |
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 ( 5 files changed, 869 insertions(+), 2 deletions(-)) |
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|TestAccVertexAIIndex_updated|TestAccVertexAIIndex_vertexAiIndexStreamingExample|TestAccVertexAIIndex_vertexAiIndexExample|TestAccLoggingBucketConfigProject_cmekSettings |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
/gcbrun |
@roaks3 The failed test is not related to the changes in this PR. So could you review this PR? Apart from that, the failed test is what I added in the previous PR. So I'd appreciate it if I could know the error message to fix it in a separate PR 🙏 |
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.
Everything looks good, I just wanted to check if there was a reason for leaving deployedIndexes
out. Ideally we would probably want to include it within this changeset.
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.
LGTM, adding another reviewer for follow-up
@roaks3 Thank you for your reviews! @slevenick Could you review this PR when you have time? Thanks! |
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.
Can you add an update test for this resource? We write handwritten update tests to make sure the resource can be modified in-place correctly and that the fields marked as updatable (in this case it looks like network is updatable?) are actually updatable
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 ( 6 files changed, 1015 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
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 ( 6 files changed, 1015 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
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 ( 6 files changed, 1032 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccVertexAIIndexEndpoint_updated|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Awesome, this is ready to go! I checked the CLA issue, and it does appear you'll need to rebase to remove my
|
a9efb02
to
4d707ca
Compare
@roaks3 Yay, I completed the git-rebase. Could you please approve the workflow again? |
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 ( 6 files changed, 1032 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataLossPreventionDeidentifyTemplate_dlpDeidentifyTemplateBasicExample|TestAccDataLossPreventionDeidentifyTemplate_dlpDeidentifyTemplate_infoTypeTransformationsUpdate|TestAccDataLossPreventionDeidentifyTemplate_dlpDeidentifyTemplateSkipCharactersExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
@slevenick this is now blocked on your prior review where changes were requested. It looks good to me, so would you mind clearing your review so we can merge, or giving it another pass? |
Hello @roaks3, It looks like @slevenick unblocked this PR to merge by removing the past change request. Can we merge this PR with your approval to wrap up? |
Thanks for the ping! Yep, we can go ahead and merge. I'm going to wait just a bit as GitHub is having some issues, but once things look stable we can move forward. |
Thank you all for your kind support and feedback! |
…ngine (GoogleCloudPlatform#6873) * feat: create vertex_ai_index_endpoint * fix: remove a white line * feat: add a comment about deployedIndexes [ci skip] * feat: add an unit test to check update * fix: fix the handwritten test name to avoid a conflict * fix: fix unnecessary code from test * fix: use immutable instead of input * refactor: refactor test code with acctest and tpgresource * test: update network_name to use the unique names * test: fix an invalid label definition * feat: increase timeout from 20m to 1h regarding vertex ai index endpoint * test: increase the timeouts to 120m * Apply suggestions from code review * feat: update test and yaml to clarify the fields mutabilities
…ngine (GoogleCloudPlatform#6873) * feat: create vertex_ai_index_endpoint * fix: remove a white line * feat: add a comment about deployedIndexes [ci skip] * feat: add an unit test to check update * fix: fix the handwritten test name to avoid a conflict * fix: fix unnecessary code from test * fix: use immutable instead of input * refactor: refactor test code with acctest and tpgresource * test: update network_name to use the unique names * test: fix an invalid label definition * feat: increase timeout from 20m to 1h regarding vertex ai index endpoint * test: increase the timeouts to 120m * Apply suggestions from code review * feat: update test and yaml to clarify the fields mutabilities
…ngine (GoogleCloudPlatform#6873) * feat: create vertex_ai_index_endpoint * fix: remove a white line * feat: add a comment about deployedIndexes [ci skip] * feat: add an unit test to check update * fix: fix the handwritten test name to avoid a conflict * fix: fix unnecessary code from test * fix: use immutable instead of input * refactor: refactor test code with acctest and tpgresource * test: update network_name to use the unique names * test: fix an invalid label definition * feat: increase timeout from 20m to 1h regarding vertex ai index endpoint * test: increase the timeouts to 120m * Apply suggestions from code review * feat: update test and yaml to clarify the fields mutabilities
Part of hashicorp/terraform-provider-google#12818
Part of hashicorp/terraform-provider-google#9298
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)