-
Notifications
You must be signed in to change notification settings - Fork 4.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
New Data Source: azurerm_mssql_managed_database #27026
New Data Source: azurerm_mssql_managed_database #27026
Conversation
d45958c
to
1a41f12
Compare
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 this PR @theonlyames! This is off to a good start, I left some comments in-line, if you could take a look then we can get this moving and hopefully into the provider soon.
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source_test.go
Outdated
Show resolved
Hide resolved
Thanks for the feedback. I'll start on making those changes! |
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.
Apologies for taking a while to circle back to this @theonlyames. I took another look through and there are a few things that would be great to have fixed up. Once that's done I can kick off the tests and we should be able to get this merged.
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source.go
Outdated
Show resolved
Hide resolved
I believe I have made all the changes you suggested. Thanks for the review. |
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.
@theonlyames I think the CI will be fixed by performing a rebase. If you can do that and get those checks green then this should be good to go!
Removed Stray Comments Replaced Parent Manged Instance Resource arguments with managed_instance_id Cleanup Computed Args Formatted Code Updated Attributes
Co-authored-by: stephybun <[email protected]>
Co-authored-by: stephybun [email protected]
11c8d4d
to
930e328
Compare
I have rebased my branch off of the main branch. |
I believe I may have messed things up in the rebase. I will rework things |
Should hopefully be all set now. |
internal/services/mssqlmanagedinstance/mssql_managed_database_data_source_test.go
Outdated
Show resolved
Hide resolved
…data_source_test.go That makes sense. Thank you Co-authored-by: stephybun <[email protected]>
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 @theonlyames LGTM 🌸
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
This PR adds a Data Source for the azurerm_mssql_managed_database resource.
My organization is prohibited from creating or deleting Azure Managed Instance resources through Terraform, but it still is useful for use to access the SQL MI databases as a Data Source as part of our multi-layer Terraform infrastructure.
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Testing
I lack a personal Azure subscription that I can use to perform acceptance tests.
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_mssql_managed_database
[New Data Source: azurerm_mssql_managed_database #27026]This is a (please select all that apply):
Related Issue(s)
N/A
Note
If this PR changes meaningfully during the course of review please update the title and description as required.