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 data source for google_compute_region_disk #9421

Merged
merged 14 commits into from
Dec 7, 2023

Conversation

estebanbouza
Copy link
Contributor

@estebanbouza estebanbouza commented Nov 7, 2023

This change adds support for a data source for google_compute_region_disk .

Fixes hashicorp/terraform-provider-google#16462

Release Note Template for Downstream PRs (will be copied)

`google_compute_region_disk`

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will require approval to run.

@shuyama1, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Nov 7, 2023
@shuyama1
Copy link
Member

shuyama1 commented Nov 7, 2023

@estebanbouza Thanks for working on this! Would you mind checking the internal contribution page to get your GH account registered so that the tests will automatically run for your changes in the future. Thanks!

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Nov 7, 2023
@shuyama1
Copy link
Member

shuyama1 commented Nov 7, 2023

/gcbrun

@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, 205 insertions(+))
Terraform Beta: Diff ( 4 files changed, 205 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3209
Passed tests 2883
Skipped tests: 325
Affected tests: 1

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
TestAccDataSourceGoogleComputeRegionDisk_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccDataSourceGoogleComputeRegionDisk_basic[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

@shuyama1
Copy link
Member

shuyama1 commented Nov 7, 2023

Haven't looked closely into the change, only checked the failed tests. It's due to a missing required field replica_zones in the config of google_compute_region_disk in the test

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

Overall looks good. Some comments regarding tests and docs

Copy link
Contributor Author

@estebanbouza estebanbouza left a comment

Choose a reason for hiding this comment

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

The comments have been addressed and VCR tests passing locally.

@estebanbouza
Copy link
Contributor Author

@shuyama1 thanks for the detailed review. All comments should be addressed now. All tests passing locally. There's a merge conflict but I believe that's part of the auto-generated code? Let me know if you require any further changes. Thanks!

@shuyama1
Copy link
Member

shuyama1 commented Dec 4, 2023

@shuyama1 thanks for the detailed review. All comments should be addressed now. All tests passing locally. There's a merge conflict but I believe that's part of the auto-generated code? Let me know if you require any further changes. Thanks!

@estebanbouza Thanks for the update! We've moved out the resource/datasource map from mmv1/third_party/terraform/provider/provider.go.erb file to a separate file mmv1/third_party/terraform/provider/provider_mmv1_resources.go.erb in a recent change #9433. Since this datasource is handwritten, you'll need to move the datasource entry from the provider.go.erb to the new file to resolve the merge conflict. Sorry for the extra work.

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

LGTM overall. Only one small nit-pick and test should be triggered after the merge conflict is resolved.

…_compute_region_disk_test.go

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>
@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, 152 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 4 files changed, 152 insertions(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3261
Passed tests 2928
Skipped tests: 332
Affected tests: 1

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
TestAccDataSourceGoogleComputeRegionDisk_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleComputeRegionDisk_basic[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

@estebanbouza
Copy link
Contributor Author

@shuyama1 changes applied, merging done, tests passing. Thanks!

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

Only one nitpick. Should be good to go after. Thanks!

"google_compute_region_instance_group": compute.DataSourceGoogleComputeRegionInstanceGroup(),
"google_compute_region_instance_template": compute.DataSourceGoogleComputeRegionInstanceTemplate(),
"google_compute_region_disk": compute.DataSourceGoogleComputeRegionDisk(),
Copy link
Member

Choose a reason for hiding this comment

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

only nit - let's move google_compute_region_disk before google_compute_region_instance_group and revert the change of google_compute_router to follow the alphabetical order. Sorry for the nit-picks

@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, 151 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 5 files changed, 156 insertions(+), 1 deletion(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3267
Passed tests 2932
Skipped tests: 334
Affected tests: 1

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
TestAccGKEHub2Fleet_gkehubFleetBasicExample_update

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccGKEHub2Fleet_gkehubFleetBasicExample_update[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

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

Thank you!

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.

Add data source for google_compute_region_disk
3 participants