-
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
Datasource - google_sql_database_instances #6980
Conversation
Sync Forked Repo
… user from setting "password" or "host" for CLOUD_IAM_USER and CLOUD_IAM_SERVICE_ACCOUNT user types.
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @SarahFrench, 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 ( 4 files changed, 564 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 testsTestAccDataSourceSqlDatabaseInstanceList_regionFilter|TestAccDataSourceSqlDatabaseInstanceList_databaseVersionFilter|TestAccDataSourceSqlDatabaseInstanceList_basic|TestAccLoggingBucketConfigProject_cmekSettings|TestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccFirebaserulesRelease_BasicRelease |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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 ( 4 files changed, 636 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 testsTestAccDataSourceSqlDatabaseInstanceList_tierFilter|TestAccDataSourceSqlDatabaseInstanceList_regionFilter|TestAccLoggingBucketConfigProject_cmekSettings|TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withInvalidGatewayApiConfigChannel |
Tests passed during RECORDING mode: All tests passed |
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.
Hi @ravisiddhu 👋 Thanks for your PR!
Could you please add some documentation to accompany the new data source? When a new handwritten resource/data source is added the documentation needs to be handwritten too. This is easy to miss because we haven't added guidance about this to our contribution docs yet!
Also, I've left some comments asking for some changes in the code related to naming and the sql_user resource.
- Sarah
Here's some guidance to help with writing the docs:
- You'd need to create a file called
sql_database_instances.html.markdown
in the mmv1/third_party/terraform/website/docs/d/ folder. The file's location is important here; d == data sources. - You can copy the existing documentation for the sql_database_instance data source but change the text to be for a plural data source. Note that you don't need to reproduce all the info about arguments - just link off to the resource that corresponds to your data source.
- This tool lets you copy/paste in your handwritten documentation markdown and see how it would be rendered in the Registry website : Doc Preview Tool
mmv1/third_party/terraform/data_sources/data_source_sql_database_instance_list.go
Outdated
Show resolved
Hide resolved
I have added the documentation for the new data source, and I have also reverted the changes in the google_sql_user resource. |
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 ( 4 files changed, 652 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccContainerCluster_withInvalidGatewayApiConfigChannel|TestAccComputeForwardingRule_update|TestAccLoggingBucketConfigProject_cmekSettings|TestAccDataSourceSqlDatabaseInstances_regionFilter|TestAccDataSourceSqlDatabaseInstances_basic|TestAccDataSourceSqlDatabaseInstances_databaseVersionFilter|TestAccDataSourceSqlDatabaseInstances_tierFilter|TestAccFirebaserulesRelease_BasicRelease |
Tests passed during RECORDING mode: All tests passed |
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 ( 4 files changed, 652 insertions(+)) |
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|TestAccContainerCluster_withInvalidGatewayApiConfigChannel |
mmv1/third_party/terraform/data_sources/data_source_sql_database_instances.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/d/sql_database_instances.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/d/sql_database_instances.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/d/sql_database_instances.html.markdown
Outdated
Show resolved
Hide resolved
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.
Thanks for adding the documentation - I've made some suggested changes for a few pluralisation-related changes, but besides that I think this PR is ready to merge!
Also, I'll hold off on approval until the other reviewer's comments are addressed/resolved
I have made the suggested changes. |
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 ( 4 files changed, 650 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccContainerCluster_withInvalidGatewayApiConfigChannel |
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.
Thanks for the changes, approved!
@SarahFrench Could you please merge this PR? |
* Added validation for "type" in cloud_sql_user_resource for preventing user from setting "password" or "host" for CLOUD_IAM_USER and CLOUD_IAM_SERVICE_ACCOUNT user types. * Added new data_source_sql_database_instance_list data source and test file * Added new data_source_sql_database_instance_list data source and test file * Reverting validation of usert types, this would be added as a seperate PR * Added documentation for the new google_sql_database_instances data source * mend * mend * mend * mend * mend
This PR is to create a new data source as request in the issue mentioned in hashicorp/terraform-provider-google#8164
Buganizer reference : (b/259913353): EXT: Datasource google_sql_database_instances
I have created a new data source called "google_sql_database_instance_list" which would fetch all the database instances in a project, and u can apply filters on top of it {project, database_version, region, zone, tier and state}.
I have tested this new data source with unit tests.