-
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): add parameter region
to google_vertex_ai_featurestore_entitytype_feature
#6992
feat(vertexai): add parameter region
to google_vertex_ai_featurestore_entitytype_feature
#6992
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 ( 1 file changed, 33 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccLoggingBucketConfigProject_cmekSettings |
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 made some comments here to make sure that this solution is correct, but I also left a comment on the issue that it is resolving. I'd like to confirm that this is not a breaking change, and does not have other implications.
|
||
entitytype := d.Get("entitytype").(string) | ||
|
||
re := regexp.MustCompile("^projects/(.+)/locations/(.+)/featurestores/(.+)$") |
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.
Would we want to validate that the entity type also includes entityTypes/
, or am I misunderstanding this logic?
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 purpose of the regex is just to extract the <location>
from projects/<project name>/locations/<location>/featurestores/<featurestore name>/entityTypes/<entitytype name>
.
I didn't intend to validate the id of entity type here, but adding the validation sounds good to me. So I'll update the regex. 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.
I've updated it in 047a83b.
@@ -0,0 +1,8 @@ | |||
if v, ok := d.GetOk("entitytype"); ok { | |||
re := regexp.MustCompile("projects/(.+)/locations/(.+)/featurestores/(.+)$") |
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.
Same question as above, do we need entityTypes
?
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.
mmv1/templates/terraform/extra_schema_entry/vertex_ai_featurestore_entitytype_feature.go.erb
Outdated
Show resolved
Hide resolved
I face a similar issue using Error: Cannot determine region: set in this resource, or set provider-level 'region' or 'zone'. This is the version using: Terraform v1.3.7
What's the remedy? |
@shotarok lightly pinging this one. The PR has been open for quite a while now; could we try to move it forward or close it? |
Thanks for the fix! We used it & cleaned up our configs really nicely!
…On Tue, May 23, 2023 at 3:50 PM Shotaro Kohama ***@***.***> wrote:
@roaks3 <https://github.com/roaks3> (CC: @mahvar
<https://github.com/mahvar>) Sorry for keeping this PR open for a long
time. I'll check out this PR and the original issue too
—
Reply to this email directly, view it on GitHub
<#6992 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH77W2I6Z5NSOYX6LCWLKYDXHUPJPANCNFSM6AAAAAATCZYPVM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Fereshteh Mahvar | Staff Medical Device Software Engineer - Google Health |
***@***.*** | +1 650-448-9653
|
I haven't fixed the problem of the featurestore_entitytype_feature yet, but that's good to know the similar issue of google_vertex_ai_endpoint was resolved! |
@roaks3 I've updated the code and replied to the comments. Could you take a look when you have time?
I believe this PR is a bug fix, not a breaking change as the issue labels indicate (hashicorp/terraform-provider-google#13217). Please let me know if there are any concerns. [UPDATED] I pushed two additional commits because I found unintentional changes |
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 ( 1 file changed, 33 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 testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
…ore_entitytype_feature` (GoogleCloudPlatform#6992) * feat: add parameter region to google_vertex_ai_featurestore_entitytype_feature * feat: update code to use tpgresource and transport_tpg * fix: update a description of the field * refactor: update regex to validate id of an entity type * fix: remove an no longer used yaml file * fix: fix the files to update the regex
…ore_entitytype_feature` (GoogleCloudPlatform#6992) * feat: add parameter region to google_vertex_ai_featurestore_entitytype_feature * feat: update code to use tpgresource and transport_tpg * fix: update a description of the field * refactor: update regex to validate id of an entity type * fix: remove an no longer used yaml file * fix: fix the files to update the regex
…ore_entitytype_feature` (GoogleCloudPlatform#6992) * feat: add parameter region to google_vertex_ai_featurestore_entitytype_feature * feat: update code to use tpgresource and transport_tpg * fix: update a description of the field * refactor: update regex to validate id of an entity type * fix: remove an no longer used yaml file * fix: fix the files to update the regex
fixes hashicorp/terraform-provider-google#13217.
This PR will make
google_vertex_ai_featurestore_entitytype_feature
always use regional endpoint corresponding to parent's region. This will prevent the error: "Cannot determine region: set in this resource, or set provider-level 'region' or 'zone'" from happening.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)