Terraform Support for Dataplex Tasks.#8124
Conversation
|
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @NickElliot, 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. |
|
It looks like this failure might resolve itself with another /gcbrun - giving it a shot. |
|
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 ( 9 files changed, 3709 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataplexTask_dataplexTaskPrimaryExample|TestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccDataSourceAlloydbLocations_basic|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataplexTaskIamPolicyGenerated|TestAccDataplexTaskIamBindingGenerated|TestAccDataplexTaskIamMemberGenerated |
|
Note: the missing-test and breaking-change detector unit tests are failing for unrelated reasons. |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
There was a problem hiding this comment.
Added a few comments on the implementation of the resource itself.
Other than that, because it looks like this resource is mostly updateable in-place, could you add some handwritten update tests that flex that functionality?
Have a customer deadline so removing the update functionality for now . Will provide support for update in a later 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 ( 9 files changed, 3709 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
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 testsTestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccComputeFirewallPolicyRule_multipleRules|TestAccDataplexTaskIamMemberGenerated|TestAccDataplexTaskIamBindingGenerated|TestAccDataplexTaskIamPolicyGenerated |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
There was a problem hiding this comment.
As this resource is entirely standard, there is not a major reason to avoid launching with update support, and if it can be updated in place, it needs an update test.
It shouldn't need to be a particularly complicated update test as the point is mostly to flex that functionality period (that the resource is able to undergo applying a change, then having that changed state imported) rather than that every single field can be updated. Here is an example. If there is any questions I'll try to respond ASAP.
However regardless of the update support/not, we do still need full detailed test samples added that cover all the optional fields (at least two additional samples needed -- one for spark, one for notebook). 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 ( 10 files changed, 3888 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
NickElliot
left a comment
There was a problem hiding this comment.
Addressed questions and issues. All that is missing now is the expanded full test coverage :)
|
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 ( 10 files changed, 3882 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataplexTaskDataplexTask_update|TestAccDataplexTask_dataplexTaskBasicExample|TestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccComputeFirewallPolicyRule_multipleRules|TestAccDataplexTaskIamMemberGenerated|TestAccDataplexTaskIamBindingGenerated|TestAccDataplexTaskIamPolicyGenerated |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
NickElliot
left a comment
There was a problem hiding this comment.
You did not update the correct area and caused the build to fail. Please look at the existing comments I provided. Thanks!
Sorry by mistake had updated the wrong verb. |
|
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 ( 10 files changed, 4297 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Action takenFound 11 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataplexTask_dataplexTaskNotebookExample|TestAccDataplexTask_dataplexTaskSparkExample|TestAccDataplexTaskDataplexTask_update|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccVertexAIIndex_updated|TestAccVertexAIIndex_vertexAiIndexStreamingExample|TestAccVertexAIIndex_vertexAiIndexExample|TestAccComputeFirewallPolicyRule_multipleRules|TestAccDataplexTaskIamPolicyGenerated|TestAccDataplexTaskIamMemberGenerated|TestAccDataplexTaskIamBindingGenerated |
|
Rerun these tests in REPLAYING mode to catch issues
|
| create_url: 'projects/{{project}}/locations/{{location}}/lakes/{{lake}}/tasks?task_id={{task_id}}' | ||
| update_url: 'projects/{{project}}/locations/{{location}}/lakes/{{lake}}/tasks/{{task_id}}' | ||
| delete_url: 'projects/{{project}}/locations/{{location}}/lakes/{{lake}}/tasks/{{task_id}}' | ||
| update_verb: :POST |
There was a problem hiding this comment.
| update_verb: :POST | |
| update_verb: :PATCH |
When you changed this back you put the incorrect update verb.
|
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 ( 10 files changed, 4297 insertions(+), 3 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. Please add acceptance tests which include these fields. |
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 testsTestAccComputeFirewallPolicyRule_multipleRules|TestAccContainerAwsNodePool_BetaBasicHandWritten |
|
|
NickElliot
left a comment
There was a problem hiding this comment.
LGTM :)
To @trodge for final review
If this PR is for Terraform, I acknowledge that I have:
make testandmake lintin the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)