-
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: add IAM resources for Vertex AI Endpoints #7965
feat: add IAM resources for Vertex AI Endpoints #7965
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @slevenick, 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. |
a752585
to
66a1763
Compare
hi @slevenick! I have now run the unit tests for |
@slevenick would you be able to take a look at this? |
Sorry I must have missed this originally. Can you rebase this from current main? Our CI is having trouble running it as it's too old |
66a1763
to
d97e370
Compare
done! |
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, 219 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_vertex_ai_endpoint_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
endpoint = # value needed
location = # value needed
members = # value needed
role = # value needed
}
Resource: resource "google_vertex_ai_endpoint_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
endpoint = # value needed
location = # value needed
member = # value needed
role = # value needed
}
Resource: resource "google_vertex_ai_endpoint_iam_policy" "primary" {
endpoint = # value needed
location = # value needed
policy_data = # value needed
}
|
Tests seem not to be generating. Looks like it's because we skip the only test in the vertex AI endpoints resource. Can we unmark the test as skip_test to see if that will generate the IAM resource tests? |
Tests analyticsTotal tests:
|
@slevenick done! |
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, 367 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_vertex_ai_endpoint_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
location = # value needed
}
Resource: resource "google_vertex_ai_endpoint_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
location = # value needed
}
Resource: resource "google_vertex_ai_endpoint_iam_policy" "primary" {
location = # value needed
}
|
Tests analyticsTotal tests:
|
/gcbrun I'll try kicking it off again |
Oh no, I see it: google/resource_vertex_ai_endpoint_test.go:150:6: testAccCheckVertexAIEndpointDestroyProducer redeclared in this block We may need to go back to skipping the test and hand-writing a test for the IAM resources specifically. You can use the generated test in the previous diff as a starting place for that, because it should do all of what we need |
@slevenick is this what you had in mind for the handwritten test? (I am still pretty new to magic modules!) |
@slevenick is anything more needed for this 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.
Sorry, there were some folder changes recently, you may need to rebase and move the handwritten file.
CONFLICT (file location): mmv1/third_party/terraform/tests/iam_vertex_endpoint_test.go.erb added in head/feat/vertex_endpoint_iam inside a directory that was renamed in HEAD, suggesting it should perhaps be moved to mmv1/third_party/terraform/services/compute/iam_vertex_endpoint_test.go.erb.
compute is the wrong suggestion though.
818fc2a
to
8a21b96
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 ( 4 files changed, 232 insertions(+), 2 deletions(-)) |
@@ -0,0 +1,407 @@ | |||
<% autogen_exception -%> | |||
package google |
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 think the package is different now to match vertexai
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 ( 4 files changed, 232 insertions(+), 2 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_vertex_ai_endpoint_iam_binding" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
location = # value needed
}
Resource: resource "google_vertex_ai_endpoint_iam_member" "primary" {
condition {
description = # value needed
expression = # value needed
title = # value needed
}
location = # value needed
}
Resource: resource "google_vertex_ai_endpoint_iam_policy" "primary" {
location = # value needed
}
|
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 testsTestAccDataSourceGoogleServiceAccountIdToken_impersonation |
Rerun these tests in REPLAYING mode to catch issues
|
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.
Compilation failure at:
google-beta/services/vertexai/iam_vertex_endpoint_test.go:32:24: fmt.Sprintf format %s reads arg #3, but call has 2 args
@ghost, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 14 days. This notification can be disabled with the |
Fixes hashicorp/terraform-provider-google#14630
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)