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

New Data Source: azurerm_sql_database #4210

Merged
merged 11 commits into from
Sep 3, 2019

Conversation

leewilson86
Copy link
Contributor

A new data source to be able to query Azure SQL Databases.

@ghost ghost added size/XL and removed size/L labels Sep 2, 2019
Copy link
Collaborator

@katbyte katbyte left a 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!

Computed: true,
},

"name": {
Copy link
Collaborator

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.

Computed: true,
},

"location": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Followed by this

Computed: true,
},

"resource_group_name": azure.SchemaResourceGroupNameForDataSource(),
Copy link
Collaborator

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.


d.Set("collation", resp.Collation)

if dsLocation := resp.ElasticPoolName; dsLocation != nil {
Copy link
Collaborator

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

d.Set("edition", string(resp.Edition))

if ep := resp.ElasticPoolName; ep != nil {
d.Set("elastic_pool_name", ep)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here:

Suggested change
d.Set("elastic_pool_name", ep)
d.Set("elastic_pool_name", props.ElasticPoolName)

d.Set("elastic_pool_name", ep)
}

if fogID := resp.FailoverGroupID; fogID != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here

if resp.ReadScale == sql.ReadScaleEnabled {
readScale = true
}
d.Set("read_scale", readScale)
Copy link
Collaborator

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:

Suggested change
d.Set("read_scale", readScale)
d.Set("read_scale", props.ReadScale == sql.ReadScaleEnabled)

d.SetId(*id)
}

d.Set("collation", resp.Collation)
Copy link
Collaborator

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)

@leewilson86
Copy link
Contributor Author

@katbyte thanks for the feedback. I have addressed your comments.

@ghost ghost removed the waiting-response label Sep 3, 2019
Copy link
Collaborator

@katbyte katbyte left a 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 🚀

@MattMencel
Copy link
Contributor

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? azurerm_sql_databases ?

@katbyte katbyte merged commit 507c885 into hashicorp:master Sep 3, 2019
@leewilson86
Copy link
Contributor Author

leewilson86 commented Sep 3, 2019

@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 azurerm_sql_server data source like this: data.azurerm_sql_server.server[0].databases.

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.

@katbyte
Copy link
Collaborator

katbyte commented Sep 3, 2019

@MattMencel, @leewilson86 is right in that the best place for that is the azurerm_sql_server datasource as `azurerm_sql_server.databases.

katbyte added a commit that referenced this pull request Sep 3, 2019
@leewilson86 leewilson86 deleted the DataSource/SqlDatabase branch September 3, 2019 21:40
@ghost
Copy link

ghost commented Sep 18, 2019

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 ...

@ghost
Copy link

ghost commented Oct 4, 2019

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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants