-
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
add vertexai/featureonlinestore resource #9632
add vertexai/featureonlinestore resource #9632
Conversation
Hello! I am a robot. It looks like you are a: @SarahFrench, 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. |
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, 1170 insertions(+), 2 deletions(-)) |
6e85896
to
cdea0f5
Compare
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, 1168 insertions(+), 2 deletions(-)) |
cdea0f5
to
6588ee1
Compare
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, 1168 insertions(+), 2 deletions(-)) |
6588ee1
to
c8b1d34
Compare
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, 1168 insertions(+), 2 deletions(-)) |
c8b1d34
to
e3cd9e4
Compare
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, 1168 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccSpannerDatabaseIamPolicy|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBigtableExample|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBigtable_basic|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBetaFieldsOptimizedExample|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBetaFieldsBigtableExample |
Rerun these tests in REPLAYING mode to catch issues
|
e3cd9e4
to
c538695
Compare
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'm at the end of my day so can review properly tomorrow, but wanted to share how to fix the test failure affecting the Beta-only field tests:
vcr_utils.go:152: Step 1/2 error: Error running pre-apply refresh:
Error: Inconsistent dependency lock file
The following dependency selections recorded in the lock file are
inconsistent with the current configuration:
- provider registry.terraform.io/hashicorp/google: required by this configuration but no version is selected
To make the initial dependency selections that will initialize the dependency
lock file, run:
terraform init
When testing Beta features only the google-beta
provider is available for use, so all resources etc need to use it. The data sources are defaulting to looking for the GA google
provider but the test framework doesn't make it available.
.../templates/terraform/examples/vertex_ai_featureonlinestore_with_beta_fields_optimized.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/templates/terraform/examples/vertex_ai_featureonlinestore_with_beta_fields_bigtable.tf.erb
Outdated
Show resolved
Hide resolved
We had good timing then! 😄 Glad it's sorted |
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, 1172 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBetaFieldsOptimizedExample|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBetaFieldsBigtableExample |
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 ( 6 files changed, 1177 insertions(+), 2 deletions(-)) |
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 testsTestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBetaFieldsBigtableExample |
|
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 error shown in this message is :
| 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_vertex_ai_feature_online_store.featureonlinestore will be updated in-place
| ~ resource "google_vertex_ai_feature_online_store" "featureonlinestore" {
| id = "projects/PROJECT_ID/locations/us-central1/featureOnlineStores/terraform36zdh3a98t0"
| name = "terraform36zdh3a98t0"
| # (8 unchanged attributes hidden)
|
| ~ dedicated_serving_endpoint {
| # (1 unchanged attribute hidden)
|
| + private_service_connect_config {
| + enable_private_service_connect = true
| + project_allowlist = [
| + "1234567890",
| ]
| }
| }
|
| # (2 unchanged blocks hidden)
| }
|
| Plan: 0 to add, 1 to change, 0 to destroy.
This is because, despite the POST request to create that resource including:
"dedicatedServingEndpoint": {
"privateServiceConnectConfig": {
"enablePrivateServiceConnect": true,
"projectAllowlist": [
"1234567890"
]
}
},
the GET request to read the resource's information (after the create operation reported that it was done) includes this instead, and information from above is missing:
"dedicatedServingEndpoint": {
"publicEndpointDomainName": "523084585.us-central1-1051995038222.vdb.vertexai.goog"
},
Do you know if this is expected behaviour from the Beta API?
mmv1/third_party/terraform/services/vertexai/resource_vertex_ai_feature_online_store_test.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/vertexai/resource_vertex_ai_feature_online_store_test.go
Outdated
Show resolved
Hide resolved
5a9c58a
to
a01ceb2
Compare
3da0254
to
12c4de0
Compare
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, 241 insertions(+), 269 deletions(-)) |
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.
Some initial comments that are largely cleanup or related to the rebase. Once those are resolved, I can take another pass with more focus on the specific field additions, but I expect those are probably in a good state based on previous review and passing tests.
mmv1/templates/terraform/examples/vertex_ai_featureonlinestore_with_beta_fields_bigtable.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/templates/terraform/examples/vertex_ai_featureonlinestore_with_bigtable.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/templates/terraform/examples/vertex_ai_featureonlinestore_with_bigtable.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/vertexai/resource_vertex_ai_feature_online_store_test.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/vertexai/resource_vertex_ai_feature_online_store_test.go
Outdated
Show resolved
Hide resolved
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, 184 insertions(+), 211 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_vertex_ai_feature_online_store" "primary" {
force_destroy = # value needed
}
|
db7805f
to
4cb830c
Compare
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, 185 insertions(+), 211 deletions(-)) |
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.
A couple more small suggestions, but the main remaining question I have is how these tests will behave in our test environment. Namely, do we need to do anything special to keep these resources from accumulating, or will the test properly clean them up?
immutable: true | ||
required: true | ||
url_param_only: 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.
The precedence for vertexai is a bit fragmented unfortunately, with Endpoint being the primary exception to the pattern you mention. However, since most of the other resources do use url_param_only
and pattern
, I think it makes sense to keep them here.
However, are you sure we should remove required
? What happens if a name is not specified?
embedding_management { | ||
enabled = true | ||
} | ||
force_destroy = 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.
Should probably be removed, this will show up in our docs.
Follow-up question to confirm: when this config is run in our tests, I assume it will still be deleted without force_destroy
because it is not using any feature views or features. Is that correct, or will there be leftover resources?
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.
Follow
- Can we have force_destroy = true as set for one of the examples ? This would also mean that we'd be addressing the missing test report that is getting generated for
force_destroy
field and also an example of the usage of the field too. - Yes, It'll still be deleted. I've run the tests manually in my test env and the resources got deleted as expected at the end of the test in both force_destroy = true and false cases.
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.
Sure, if you feel it is better to include for this example I think that makes sense, and can always be adjusted later. Thanks for checking that resources get deleted!
mmv1/templates/terraform/examples/vertex_ai_featureonlinestore_with_bigtable.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/vertexai/resource_vertex_ai_feature_online_store_test.go
Outdated
Show resolved
Hide resolved
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccVertexAIFeatureOnlineStore_updated|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureOnlineStoreExample|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBetaFieldsBigtableExample|TestAccVertexAIFeatureOnlineStore_vertexAiFeatureonlinestoreWithBetaFieldsOptimizedExample |
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 ( 5 files changed, 176 insertions(+), 203 deletions(-)) |
b9c3210
to
c75e76e
Compare
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 pending test results
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, 176 insertions(+), 202 deletions(-)) |
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 testsTestAccVertexAIFeatureOnlineStore_updated |
Rerun these tests in REPLAYING mode to catch issues
|
Note: updated the release notes |
* rebase featureonlinestore based on merged changes * rebase based on merged featureonlinestore pr * fix pr suggestions featureonlinestore * fix documentation and pr comments featureonlinestore
* rebase featureonlinestore based on merged changes * rebase based on merged featureonlinestore pr * fix pr suggestions featureonlinestore * fix documentation and pr comments featureonlinestore
* rebase featureonlinestore based on merged changes * rebase based on merged featureonlinestore pr * fix pr suggestions featureonlinestore * fix documentation and pr comments featureonlinestore
This PR aims to add support to vertex AI FeatureOnlineStore resource which is recently launched. hashicorp/terraform-provider-google#16507