-
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
Add Dataplex Asset resource #6310
Add Dataplex Asset resource #6310
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @megan07, 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! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 1350 insertions(+), 1 deletion(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1351 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 testsTestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccDataplexAsset_BasicAsset|TestAccComputeInstance_soleTenantNodeAffinities|TestAccSqlUser_mysqlDisabled |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1351 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 testsTestAccFirebaserulesRelease_BasicRelease|TestAccDataplexAsset_BasicAsset|TestAccComputeInstance_soleTenantNodeAffinities|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccSqlUser_mysqlDisabled |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
It looks like I'll have to manually create a GCS bucket for the Dataplex asset to attach to. I'll mark this review in draft until I complete that step. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1356 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 testsTestAccDataplexAsset_BasicAssetHandWritten|TestAccComputeInstance_soleTenantNodeAffinities|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1350 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 testsTestAccDataplexAsset_BasicAssetHandWritten|TestAccComputeInstance_soleTenantNodeAffinities|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1365 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 testsTestAccFirebaserulesRelease_BasicRelease|TestAccDataplexAsset_BasicAssetHandWritten|TestAccComputeInstance_soleTenantNodeAffinities|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1365 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 testsTestAccDataplexAsset_BasicAssetHandWritten|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1367 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 testsTestAccFirebaserulesRelease_BasicRelease|TestAccDataplexAsset_BasicAssetHandWritten|TestAccComputeInstance_soleTenantNodeAffinities|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
I've added an ignore_read for Asset.resource_spec.name. This is an immutable field, and the Dataplex API accepts input in the format "projects/{projectID}/buckets/{bucketID}", however it returns it as "{bucketID}". The API behavior is because of the need to maintain backwards compatibility but return results in line with other GCP APIs. Since it's an immutable field, it's safe to skip the verify import-state-step for this field. I've discussed the above with @trodge, and all the tests are passing now. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 6 files changed, 1367 insertions(+), 1 deletion(-)) |
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.
Nevermind, it's just really long! No issue
Couple notes: resource_type should not document that it allows TYPE_UNSPECIFIED, as it is not a valid value to send to the API. The discovery_spec.enabled field's documentation does not match its behavior
|
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|TestAccComputeInstance_soleTenantNodeAffinities|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
/gcbrun to fix build |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 9 files changed, 1356 insertions(+), 7 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|TestAccContainerCluster_withMeshCertificatesConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccActiveDirectoryDomain_update|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
/gcbrun there was a CI hiccup |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 9 files changed, 1356 insertions(+), 7 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|TestAccComputeInstance_soleTenantNodeAffinities|TestAccActiveDirectoryDomain_update|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample |
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!
One note but not urgent, you can fix as you have time:
This line mentions what happens when discovery_spec is unset, but the field has been marked required so it can never be unset: https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-6310-old..auto-pr-6310#diff-5d6d84399bf306af362c41c7ee924744379a41d52bb6d4754206961bd89c681aR100
* Initial commit, verify asset.yaml * Use an existing public bucket * Add Dataplex Asset into provider.go * Use a sample bucket instead of a hardcoded one * Fix asset YAML to have asset field * Fix asset YAML to have bucket field * Try again with a hardcoded bucket * minor syntax fix * Using handwritten tests since GCS buckets are not in DCL * Fix bucket name reference in Dataplex Asset * Ignore updates to bucket after creation * Change dataplex terraform resource name to primary for the tests * Add override for resource_spec.name. This is an immutable field and is normalized by the API. * upgrade dcl to 1.16 * Update test to use dataplexZone instead of zone * Add back comment accidentally removed in merge
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.