-
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
datacatalog - bump Taxonomy and PolicyTag to ga #6989
datacatalog - bump Taxonomy and PolicyTag to ga #6989
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 ( 15 files changed, 2059 insertions(+), 26 deletions(-)) |
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 ( 15 files changed, 2049 insertions(+), 32 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease|TestAccDataCatalogTaxonomy_dataCatalogTaxonomyBasicExample|TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccDataCatalogPolicyTag_dataCatalogTaxonomiesPolicyTagChildPoliciesExample|TestAccDataCatalogPolicyTag_dataCatalogTaxonomiesPolicyTagBasicExample|TestAccDataCatalogTaxonomyIamPolicyGenerated|TestAccDataCatalogTaxonomyIamMemberGenerated|TestAccDataCatalogPolicyTagIamPolicyGenerated|TestAccDataCatalogTaxonomyIamBindingGenerated|TestAccDataCatalogPolicyTagIamMemberGenerated|TestAccDataCatalogPolicyTagIamBindingGenerated |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
f780950
to
5f459aa
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 ( 15 files changed, 2049 insertions(+), 32 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccDataCatalogTaxonomyIamPolicyGenerated|TestAccDataCatalogPolicyTagIamBindingGenerated|TestAccDataCatalogPolicyTagIamMemberGenerated|TestAccDataCatalogTaxonomyIamMemberGenerated|TestAccDataCatalogTaxonomyIamBindingGenerated|TestAccDataCatalogPolicyTagIamPolicyGenerated |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
I looked into this a bit and found that the failing tests are all due to import issues; these test steps previously weren't being generated in beta due to the code described here: hashicorp/terraform-provider-google#12610 Hopefully we will be able to address that at some point. I still need to look closer in terms of fixing the import code. |
@DrFaust92 I just merged #7137 which should fix the import for datacatalog taxonomy. Could you rebase off the latest main branch to see if that fixes things? |
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 above
Sure, will try
…On Tue, 31 Jan 2023 at 0:03 Stephen Lewis (Burrows) < ***@***.***> wrote:
***@***.**** requested changes on this pull request.
see above
—
Reply to this email directly, view it on GitHub
<#6989 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIA2W7Q44JW3RYTHH4UYCHTWVA3EVANCNFSM6AAAAAATCT3ZGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Oh, it looks like policytag is likely still going to fail, actually. That may need a separate fix. Would you want to split taxonomy into a separate PR for easier review? |
@melinath how would splitting help? missing something here taxonomy also fails same as before. the way the test is generated cant work here. the get operation needs the uuid that created after create. so the |
Yep, you're absolutely right, I got too excited. :-/ |
It looks like you could work around this by switching these IAM resources to be handwritten (since we don't have a way to only skip generating the tests at the moment :-/) and then doing something similar to this: test that pulls value out of test, function that does the pulling cc @SarahFrench who suggested this on a PR related to running import tests for beta resources, which ran into this same issue. If you want to take a stab at this I think that would be fine; otherwise this will be blocked until she's done either skipping the tests or fixing them (which will hopefully be soon :-) ) |
I think the 'pulling generated values out of a test' approach would still require the tests to be handwritten (as I don't think custom code can be added into generated IAM tests). But at least then the handwritten tests would be able to include an import step! |
Update: I've implemented skipping of IAM import steps in this PR's commit 5b7760b . In it's current implementation it's a blanket exclusion of IAM import steps for any IAM tests for a given parent resource. |
Update: I've merged my PR where I have added the ability to skip import steps in acceptance tests for IAM resources. I that PR I made these change to mmv1/products/datacatalog/api.yaml to skip imports in tests for Data Catalog's If you rebase/merge those changes in here then you don't need to worry about the tests that rely on uuid's that are made by the API during resource creation |
5f459aa
to
2bb0e91
Compare
Thanks SarahFrench ive rebased and will test |
--- PASS: TestAccDataCatalogTaxonomyIamBindingGenerated (60.48s) |
/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 ( 15 files changed, 1994 insertions(+), 32 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|TestAccComputeForwardingRule_update|TestAccDataCatalogTaxonomyIamPolicyGenerated|TestAccDataCatalogPolicyTagIamPolicyGenerated|TestAccDataCatalogPolicyTagIamMemberGenerated|TestAccDataCatalogPolicyTagIamBindingGenerated|TestAccDataCatalogTaxonomyIamMemberGenerated|TestAccDataCatalogTaxonomyIamBindingGenerated|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample |
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.
lgtm - failure is unrelated
should've done this prior to merge, but out of paranoia, running tests for GA https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloud_ProviderGoogleCloudMmUpstream/378717 |
update: all passed 🎉 |
…6989) * update Taxonomy and PolicyTag to ga * update Taxonomy and PolicyTag to ga * refactor * refactor
* main: (41 commits) update the test cases to resolve resourcename not found error Adds `grpc` field to `liveness_probe` and `startup_probe` to `google_cloud_run_v2_service` resource (GoogleCloudPlatform#6987) Upgrade DCL to v1.34 (GoogleCloudPlatform#7276) Add max_distance field to group placement policy (GoogleCloudPlatform#7354) Add stateful_ips to region_per_instance_config and per_instance_config (GoogleCloudPlatform#7316) Added support for workload-vulnerability-scanning and workload-config-audit (GoogleCloudPlatform#7310) datacatalog - bump Taxonomy and PolicyTag to ga (GoogleCloudPlatform#6989) Added best practices documentation for ForceNew fields (GoogleCloudPlatform#7127) Split resources in "B" products (GoogleCloudPlatform#7350) force recreate on master_config.num_instances (GoogleCloudPlatform#7349) Fix DataFusion instance versions used in tests (GoogleCloudPlatform#7343) remove duplicate word in Cluster.yaml (GoogleCloudPlatform#7347) Move more billing tests that require permissions beyond Billing User to master billing account (GoogleCloudPlatform#7344) Remove artifact repository beta URL, fixup handwritten tests (GoogleCloudPlatform#7345) Cloud Workstations - Workstation Config (GoogleCloudPlatform#7017) Add missing `type` argument to data source docs (GoogleCloudPlatform#7341) Fix caps in spanner resource schema accesses (GoogleCloudPlatform#7346) Downgrade Go to 1.18, modify comments (GoogleCloudPlatform#7339) feat: Add support for deletion_policy on shared vpc service project (GoogleCloudPlatform#7283) fixed virtual field update issues (GoogleCloudPlatform#7318) ...
…6989) * update Taxonomy and PolicyTag to ga * update Taxonomy and PolicyTag to ga * refactor * refactor
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)