-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_sql_database #4210
Conversation
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 PR @leewilson86!
Overall this looks good with some minor comments i've left inline. however could we reorganize the properties in the schema to match the resource.go file? thanks!
azurerm/data_source_sql_database.go
Outdated
Computed: true, | ||
}, | ||
|
||
"name": { |
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.
This should be at the top & first.
azurerm/data_source_sql_database.go
Outdated
Computed: true, | ||
}, | ||
|
||
"location": { |
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.
Followed by this
azurerm/data_source_sql_database.go
Outdated
Computed: true, | ||
}, | ||
|
||
"resource_group_name": azure.SchemaResourceGroupNameForDataSource(), |
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.
And the rest should follow the ordering found in the resource file.
azurerm/data_source_sql_database.go
Outdated
|
||
d.Set("collation", resp.Collation) | ||
|
||
if dsLocation := resp.ElasticPoolName; dsLocation != nil { |
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.
We don't need this check as d.Set
will check for nilness
azurerm/data_source_sql_database.go
Outdated
d.Set("edition", string(resp.Edition)) | ||
|
||
if ep := resp.ElasticPoolName; ep != nil { | ||
d.Set("elastic_pool_name", ep) |
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.
Same here:
d.Set("elastic_pool_name", ep) | |
d.Set("elastic_pool_name", props.ElasticPoolName) |
azurerm/data_source_sql_database.go
Outdated
d.Set("elastic_pool_name", ep) | ||
} | ||
|
||
if fogID := resp.FailoverGroupID; fogID != nil { |
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.
and here
azurerm/data_source_sql_database.go
Outdated
if resp.ReadScale == sql.ReadScaleEnabled { | ||
readScale = true | ||
} | ||
d.Set("read_scale", readScale) |
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.
This can be replaced with:
d.Set("read_scale", readScale) | |
d.Set("read_scale", props.ReadScale == sql.ReadScaleEnabled) |
azurerm/data_source_sql_database.go
Outdated
d.SetId(*id) | ||
} | ||
|
||
d.Set("collation", resp.Collation) |
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.
Most of these properties actually reside in the anonymous property resp.DatabaseProperties
that could be nil. Thus we need to nil check it:
if props := respo.DatabaseProperties; props != nil {
d.Set("collation", props.Collation)
@katbyte thanks for the feedback. I have addressed your comments. |
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 revisions @leewilson86! LGTM now 🚀
If we wanted to add a way to list all the databases in a single azure_sql_server instance, would you want that in a separate data resource? |
@MattMencel that’s a good question and actually something I have been thinking about myself. I’m just not sure how best to return a list of databases, or if it’s a good idea, as there’ll be a lot of opinions on format etc. What format would we return the database? Would it be an array of database resource IDs or database names? A map? A list of maps? If this is a reasonable feature to add I think it probably best belongs with the existing However, would it be better to have databases defined as vars (possibly in .tfvars) and passed into the relevant resources for creation then those vars can be used to query for database details with this data source I’ve just created? This is pretty much how the Terraform config I’m working with operates. Your idea could work too as a dedicated resource for returning lists of databases on a server. Just not too sure on the best approach. Maybe @katbyte can advise? The data source I’ve added here is purely for the intention of querying a specific database on a given server. |
@MattMencel, @leewilson86 is right in that the best place for that is the |
This has been released in version 1.34.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 1.34.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
A new data source to be able to query Azure SQL Databases.