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 resources azurerm_mssql_virtual_machine_group and azurerm_mssql_virtual_machine_availability_group_listener #22808

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

myc2h6o
Copy link
Contributor

@myc2h6o myc2h6o commented Aug 3, 2023

Add support for SQL Virtual Machine Group and SQL Virtual Machine Availability Group Listener. This adds the ability to configure Always-On SQL Availability Group for SQL VMs.
Document
Swagger

Copy link
Member

@stephybun stephybun 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 this PR @myc2h6o. I've left some comments in-line that need to be fixed up but once that's done we can take another look through.

}, false),
},

"wsfc_domain_profile": helper.WsfcDomainProfileSchemaMsSqlVirtualMachineAvailabilityGroup(),
Copy link
Member

Choose a reason for hiding this comment

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

I only see this referenced once and the schema is flat so can we just in-line this here instead of defining it as a func elsewhere?

website/docs/r/mssql_virtual_machine_group.html.markdown Outdated Show resolved Hide resolved
website/docs/r/mssql_virtual_machine_group.html.markdown Outdated Show resolved Hide resolved
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Aug 10, 2023

@stephybun thanks for reviewing the pr! I have updated it according to your comments, please take a look.

Copy link
Member

@stephybun stephybun 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 addressing those comments @myc2h6o. Once comment was missed it seems and there's one typo in the docs. I've pointed them out in-line. Once they're resolved this should be good to go!

Comment on lines 117 to 139
"private_ip_address": {
Type: pluginsdk.TypeList,
Required: true,
ForceNew: true,
MaxItems: 1,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"ip_address": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validation.IsIPAddress,
},

"subnet_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: networkValidate.SubnetID,
},
},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Since we can only supply one of these and they're required we can flatten these.

Suggested change
"private_ip_address": {
Type: pluginsdk.TypeList,
Required: true,
ForceNew: true,
MaxItems: 1,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"ip_address": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validation.IsIPAddress,
},
"subnet_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: networkValidate.SubnetID,
},
},
},
},
"private_ip_address": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validation.IsIPAddress,
},
"subnet_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: networkValidate.SubnetID,
},

Comment on lines 175 to 197
"private_ip_address": {
Type: pluginsdk.TypeList,
Required: true,
ForceNew: true,
MaxItems: 1,
Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"ip_address": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validation.IsIPAddress,
},

"subnet_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: networkValidate.SubnetID,
},
},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Same here


A `replica` block supports the following:

* `commit` - (Required) T replica commit mode for the availability group. Possible values are `Synchronous_Commit` and `Asynchronous_Commit`. Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `commit` - (Required) T replica commit mode for the availability group. Possible values are `Synchronous_Commit` and `Asynchronous_Commit`. Changing this forces a new resource to be created.
* `commit` - (Required) The replica commit mode for the availability group. Possible values are `Synchronous_Commit` and `Asynchronous_Commit`. Changing this forces a new resource to be created.

@myc2h6o
Copy link
Contributor Author

myc2h6o commented Aug 14, 2023

@stephybun thanks, I've further flattened the private_ip_address and fixed the doc according to your comments

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @myc2h6o LGTM 👍

@stephybun stephybun merged commit 5b57b45 into hashicorp:main Aug 14, 2023
@github-actions github-actions bot added this to the v3.70.0 milestone Aug 14, 2023
@myc2h6o myc2h6o deleted the sqlvmha_update branch August 15, 2023 01:43
stephybun added a commit that referenced this pull request Aug 15, 2023
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2024
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.

3 participants