-
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
feat(vertexai): Make it possible to use Private Service Connect in Vertex AI Index Endpoint #8851
feat(vertexai): Make it possible to use Private Service Connect in Vertex AI Index Endpoint #8851
Conversation
Hello! I am a robot. It looks like you are a: Community Contributor @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 Hello, when you have time, could you approve the cloud builder to run the acceptance tests? 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 ( 3 files changed, 180 insertions(+)) |
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 testsTestAccVertexAIIndexEndpoint_vertexAiIndexEndpointWithPscExample |
Rerun these tests in REPLAYING mode to catch issues
|
- !ruby/object:Api::Type::Boolean | ||
name: enablePrivateServiceConnect | ||
description: If set to true, the IndexEndpoint is created without private service access. | ||
- !ruby/object:Api::Type::Array | ||
name: projectAllowlist | ||
description: A list of Projects from which the forwarding rule will target the service attachment. | ||
item_type: Api::Type::String |
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.
Are these two subfields also immutable? If so, I think we need to add immutable as well. If not, it's be nice that we add an update test on it.
Plus, can we make at least one of the subfields required. We 'd try to avoid users from sending empty object.
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.
The subfields were immutable although I tested a PATCH request. Based on your advice, I added immutable:
and required:
in debd2d1.
test_vars_overrides: | ||
network_name: 'acctest.BootstrapSharedTestNetwork(t, "vertex-ai-index-endpoint")' |
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.
It doesn't look like that network_name
is being used anywhere in the test. Feel free to remove it if it's not needed
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.
I confirmed an acceptance test succeeded after removing it. Therefore, I've deleted it in 1a0f1ea.
@shuyama1 Thank you for your feedback! I've updated the code. Please approve the cloud build job again. Thanks |
I merged main to resolve a conflict because another PR for IndexEndpoint was merged |
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, 183 insertions(+)) |
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.
Overall looks good. Only small comment though
@@ -65,7 +68,6 @@ parameters: | |||
url_param_only: true | |||
immutable: true | |||
properties: | |||
# Intentionally deployedIndexes[] is not included because it's an output-only field and another terraform resource will manage a deployed index. |
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.
Any reason we're removing this comment?
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.
Ah, I initially defined deployedIndexes
in this PR, but I reverted it because of #8851 (comment). At that time, I forgot to revert the comment. I'll restore it. Thank you for the catch!
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.
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 testsTestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
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, 183 insertions(+)) |
@shuyama1 I updated the code and replied to the comments. If the changes look good, please approve the CI to run the acceptance tests. Thanks! |
@@ -48,11 +48,20 @@ examples: | |||
- !ruby/object:Provider::Terraform::Examples | |||
name: "vertex_ai_index_endpoint" | |||
primary_resource_id: "index_endpoint" | |||
skip_docs: true |
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.
we'd prefer to keep example of this config in the test but skip vertex_ai_index_endpoint_with_false_psc
instead, since vertex_ai_index_endpoint
is a standard basic config for the resource
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.
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, 232 insertions(+), 21 deletions(-)) |
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, 226 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerAwsCluster_BetaBasicHandWritten|TestAccContainerAwsCluster_BetaBasicEnumHandWritten|TestAccDataprocClusterIamPolicy|TestAccLoggingProjectSink_updatePreservesCustomWriter|TestAccSpannerDatabaseIamPolicy|TestAccVertexAIIndexEndpoint_vertexAiIndexEndpointWithFalsePscExample |
Rerun these tests in REPLAYING mode to catch issues
|
I think the failed tests/checks should resolve themselves in a rerun |
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, 226 insertions(+)) |
Tests analyticsTotal tests:
|
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.
Thank you!
@shuyama1 Thank you so much for your support and reviews! I'm excited to be able to use this new function through terraform! |
…rtex AI Index Endpoint (GoogleCloudPlatform#8851)
…rtex AI Index Endpoint (GoogleCloudPlatform#8851)
…rtex AI Index Endpoint (GoogleCloudPlatform#8851)
part of hashicorp/terraform-provider-google#12818
Release Note Template for Downstream PRs (will be copied)