-
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
Added support for google_vertex_ai_tensorboard resource #6759
Added support for google_vertex_ai_tensorboard resource #6759
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @melinath, 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 ( 4 files changed, 718 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|TestAccVertexAITensorboard_vertexAiTensorboardExample |
LGTM |
@melinath Could you please review 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.
This looks pretty good; however, it needs to have:
- a "full" test that exercises as many settable fields as possible - in particular it should definitely set the encrypionSpec field, which probably means it will need to be handwritten.
- an "update" test that exercises the update behavior; this will need to be handwritten.
I also have one question below.
…odules into vertexai-add-tensorboard-resource
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, 795 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 testsTestAccVertexAITensorboard_Full |
Tests failed during RECORDING mode: Please fix these 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 ( 5 files changed, 907 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 testsTestAccVertexAITensorboard_Update|TestAccVertexAITensorboard_Full |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
It looks like the "timeout" error is a misleading error; the reason there's a timeout is:
|
/gcbrun |
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, 907 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|TestAccVertexAITensorboard_Full |
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.
this is looking really good - I realized there's a better way to handle the kms crypto key
name = "keyring-%s" | ||
location = "us-central1" | ||
} | ||
resource "google_kms_crypto_key" "example-key" { |
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.
Please switch this test to use BootstrapKMSKeyInLocation - it looks like this might even let you switch to generating the test using an example. For example:
- !ruby/object:Provider::Terraform::Examples |
Using the bootstrapped crypto keys lets us limit the number of crypto keys created, which is important since they can't be deleted.
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 the test to use BootstrapKMSKeyInLocation.
…odules into vertexai-add-tensorboard-resource
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, 921 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 testsTestAccVertexAITensorboard_vertexAiTensorboardFullExample |
display_name: "terraform" | ||
- !ruby/object:Provider::Terraform::Examples | ||
name: "vertex_ai_tensorboard_full" | ||
skip_import_test: 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.
Why do these tests use skip_import_test
?
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's because we need to use ImportStateVerifyIgnore: []string{"region", "project"}
instead of ImportStateVerifyIgnore: []string{"region"}
. There is a difference in the name
field (projects/{project}/locations/{location}/tensorboards/{tensorboard}
) of imported resources where instead of project_id we are getting project_number. Hence, I've used skip_import_test
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 that should be fine, since name
is an output-only field.
You can use ignore_read_extra
here & above instead of skip_import_test
to just ignore the one additional field:
skip_import_test: true | |
ignore_read_extra: | |
- project |
If the API returns project numbers - will users get a permadiff if they try to set the project
field using a project id?
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.
No, User won't face state drift if the project
field is set to project id. It's just that import is forming the name
field using the replaceVars
function which takes the project_id and this gets changed when the resource is successfully imported.
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 examples to include ignore_read_extra
for the tests.
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, 933 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 testsTestAccComputeForwardingRule_update|TestAccVertexAITensorboard_vertexAiTensorboardExample|TestAccVertexAITensorboard_vertexAiTensorboardFullExample|TestAccFirebaserulesRelease_BasicRelease |
Tests passed during RECORDING mode: All tests passed |
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
…latform#6759) * Added support for google_vertex_ai_tensorboard resource * Added full test case for google_vertex_ai_tensorboard resource * Added import support for google_vertex_ai_tensorboard * Added Full and Update test cases for google_vertex_ai_tensorboard * Updated the full test of google_vertex_ai_tensorboard * Updated examples to include import test for vertex_ai_tensorboard
…latform#6759) * Added support for google_vertex_ai_tensorboard resource * Added full test case for google_vertex_ai_tensorboard resource * Added import support for google_vertex_ai_tensorboard * Added Full and Update test cases for google_vertex_ai_tensorboard * Updated the full test of google_vertex_ai_tensorboard * Updated examples to include import test for vertex_ai_tensorboard
…latform#6759) * Added support for google_vertex_ai_tensorboard resource * Added full test case for google_vertex_ai_tensorboard resource * Added import support for google_vertex_ai_tensorboard * Added Full and Update test cases for google_vertex_ai_tensorboard * Updated the full test of google_vertex_ai_tensorboard * Updated examples to include import test for vertex_ai_tensorboard
…latform#6759) * Added support for google_vertex_ai_tensorboard resource * Added full test case for google_vertex_ai_tensorboard resource * Added import support for google_vertex_ai_tensorboard * Added Full and Update test cases for google_vertex_ai_tensorboard * Updated the full test of google_vertex_ai_tensorboard * Updated examples to include import test for vertex_ai_tensorboard
Added support for google_vertex_ai_tensorboard resource.
fixes hashicorp/terraform-provider-google#12711
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)