-
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
Add Dataplex Zone resource #6075
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. |
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|TestAccDataplexZone_BasicZone|TestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Reassigning to someone who knows more about the DCL than I do since it's doing weird things. A couple things to note: Error:
HTTP request to create the resource contains: "discoverySpec": {
"enabled": false,
"excludePatterns": [],
"includePatterns": []
} Response with the created resource contains: "discoverySpec": {
"csvOptions": {},
"jsonOptions": {},
"schedule": ""
} So it looks like the API isn't returning a value for enabled (which was sent) and is returning a value for schedule (which wasn't sent). This feels like it could cause issues? I think there may also be issues telling the difference between a struct that has all its values set to the "unset" value and a struct that is not set at all - but I'm not sure? |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 1099 insertions(+), 2 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.
You'll need to rebase and remove serialization.go, it shouldn't be needed anymore due to a recent change
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 1099 insertions(+), 2 deletions(-)) |
1 similar comment
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 1099 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|TestAccDataplexZone_BasicZone|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your 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.
Issues came up during manual testing when attempting to remove the discovery_spec block. A permadiff appears as the API does not seem to remove the full object, and continues to return a somewhat empty discoverySpec object.
This will need a fix either on the API-level or in the DCL
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 1096 insertions(+), 1 deletion(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
Looks pretty good! Just a couple of notes: resource_spec should be marked as immutable in the DCL YAML file that describes it, otherwise we don't correctly mark the field as requiring recreate to change. |
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.
See my above comment
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.
Looks like you've got some conflicts, can you rebase from main and resolve them?
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 1094 insertions(+), 9 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerAzureNodePool_BetaBasicHandWritten|TestAccContainerAzureNodePool_BasicHandWritten|TestAccDataplexZone_BasicZone|TestAccComputeInstance_soleTenantNodeAffinities |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your 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.
Looks good, thanks!
* Add Dataplex Zone resource * Fix zone name * Update DCL to 1.10.2 * run make upgrade-dcl * Update DCL to v1.15.1 * run make upgrade
part of hashicorp/terraform-provider-google#11648
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.