Skip to content
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 google_dns_managed_zones data source and acceptance test #9742

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

SarahFrench
Copy link
Collaborator

@SarahFrench SarahFrench commented Jan 3, 2024

Closes hashicorp/terraform-provider-google#15737

Release Note Template for Downstream PRs (will be copied)

`google_dns_managed_zones`

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 323 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 4 files changed, 323 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3324
Passed tests 2986
Skipped tests: 337
Affected tests: 1

Click here to see the affected service packages
all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@SarahFrench
Copy link
Collaborator Author

👆 I removed the skip as I believe the issue in hashicorp/terraform-provider-google#14158 isn't due to the plugin framework necessarily.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 321 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 4 files changed, 321 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3324
Passed tests 2987
Skipped tests: 336
Affected tests: 1

Click here to see the affected service packages
all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataSourceDnsManagedZones_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceDnsManagedZones_basic[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{red}{\textsf{Tests failed when rerunning REPLAYING mode:}}$
TestAccDataSourceDnsManagedZones_basic[Error message] [Debug log]

Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made.

Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.


$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$
View the build log or the debug log for each test

@SarahFrench
Copy link
Collaborator Author

Requested interaction not found

Dammit, seems like there is a problem related to the migrated data sources 🤦

@SarahFrench
Copy link
Collaborator Author

Hi @roaks3 , I'm working on a customer request to implement this plural data source and decided to implement it using the plugin-framework to match other dns data sources. I requested you as reviewer because you noticed the VCR issues with data sources migrated to the plugin-framework that also affect this PR.

What are you opinions on leaving this as using the PF versus implementing it using the SDK? I'm torn between wanting to keep the dns package's data sources consistent vs avoiding knowingly contributing to a problem with tests.

@SarahFrench
Copy link
Collaborator Author

I brought up this in the HashiCorp/Google sync yesterday, and @rileykarson said that implementing this data source using the plugin framework was ok given that it's a customer request and that the issue with VCR tests & the plugin framework is investigated soon. I'll advocate for that work to be included in my Q1 plans (starting in Feb).

Because of that I'll skip this PR's new test in VCR.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 323 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 4 files changed, 323 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3334
Passed tests 2995
Skipped tests: 338
Affected tests: 1

Click here to see the affected service packages
all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataprocJobIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocJobIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@roaks3
Copy link
Contributor

roaks3 commented Jan 9, 2024

Sorry for the delay on this. I think that makes sense to stick with PF to get this implemented more quickly, and so +1 on skipping the tests here.

Will give this a review within the next hour or so.

Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming you were able to run things locally. Just one minor question regarding the name field.

@SarahFrench
Copy link
Collaborator Author

Thanks for spotting that problem with the schema when I copied it from singular => plural data source! In future I hope to identify how we can write data source in the plugin-framework and re-use schemas with minor edits, similar to what we do for SDK resources and data sources.

I've pushed a change to the test to make it interrogate the managed zone elements more closely, to ensure the fields for those elements are being populated as expected. The tests passes locally. Previously the less-strict version of the test passed on the PR in recording mode (the pass seen here) but failed in replay mode due to the plugin framework related problem.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3334
Passed tests 2996
Skipped tests: 337
Affected tests: 1

Click here to see the affected service packages
all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

bskaplan pushed a commit to bskaplan/magic-modules that referenced this pull request Jan 17, 2024
…eCloudPlatform#9742)

* Add `google_dns_managed_zones` data source and acceptance test

* Stop skipping `TestAccDataSourceDnsManagedZones_basic` in VCR

* Skip acceptance test affected by PF/VCR incompatibility

* Edit schema to make sense for a plural data source

* Update test to explicitly check for elements' fields being set
kylase pushed a commit to yuanchuankee/magic-modules that referenced this pull request Jan 21, 2024
…eCloudPlatform#9742)

* Add `google_dns_managed_zones` data source and acceptance test

* Stop skipping `TestAccDataSourceDnsManagedZones_basic` in VCR

* Skip acceptance test affected by PF/VCR incompatibility

* Edit schema to make sense for a plural data source

* Update test to explicitly check for elements' fields being set
@SarahFrench SarahFrench deleted the add-dns-managed-zones-plural-datasource branch February 6, 2024 11:01
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
…eCloudPlatform#9742)

* Add `google_dns_managed_zones` data source and acceptance test

* Stop skipping `TestAccDataSourceDnsManagedZones_basic` in VCR

* Skip acceptance test affected by PF/VCR incompatibility

* Edit schema to make sense for a plural data source

* Update test to explicitly check for elements' fields being set
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
…eCloudPlatform#9742)

* Add `google_dns_managed_zones` data source and acceptance test

* Stop skipping `TestAccDataSourceDnsManagedZones_basic` in VCR

* Skip acceptance test affected by PF/VCR incompatibility

* Edit schema to make sense for a plural data source

* Update test to explicitly check for elements' fields being set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a plural data source for DNS Managed Zones
3 participants