-
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
mssql_elasticpool - support license_type #6631
mssql_elasticpool - support license_type #6631
Conversation
Adding license_type for ElasticPool similar to Database - tests - datasource - website Might address hashicorp#6587
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 @pearcec! LGTM 👍
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 @pearcec thanks for this PR!
Overall this PR looks good, but I have some concerns on marking the license_type
as Computed
. Usually for optional attributes of enumerations, we will need to determine a default value for it, and service usually follows the same pattern by recognizing absence of the property as a default one. Would you please confirm whether this is valid for the license_type
?
In the meantime, we will have to mark it as Computed if the return value of an absence license_type
varies from different circumstances (for instance different sku with absence license_type
returns with different license_type
).
Thanks again for the great work on this!
azurerm/internal/services/mssql/resource_arm_mssql_elasticpool.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/mssql/tests/resource_arm_mssql_elasticpool_test.go
Show resolved
Hide resolved
azurerm/internal/services/mssql/tests/resource_arm_mssql_elasticpool_test.go
Show resolved
Hide resolved
azurerm/internal/services/mssql/tests/resource_arm_mssql_elasticpool_test.go
Outdated
Show resolved
Hide resolved
cast properties.LicenseType to string to avoid potential type casting issues. Co-Authored-By: Arcturus <[email protected]>
@ArcturusZhang thank you for the feedback, I am pushing some adjustments to the test, pulled in your change, and am looking for feedback on the Optional+Computed, vs Optional+Default. |
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 additional changes @pearcec! LGTM 2x 👍
This has been released in version 2.8.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 = "~> 2.8.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 👉 [email protected]. Thanks! |
Adding license_type for ElasticPool similar to Database
Might address #6587