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.

Sign up for GitHub

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

Jump to bottom

r\linux_virtual_machine r\windows_virual_machine: Add support for termination_notification #14933

Merged
merged 1 commit into from
Mar 22, 2022
Merged

r\linux_virtual_machine r\windows_virual_machine: Add support for termination_notification #14933

merged 1 commit into from
Mar 22, 2022

Conversation

myc2h6o
Copy link
Contributor

@myc2h6o myc2h6o commented Jan 13, 2022 •
edited
Loading

Aligned to orchestrated VMSS implementation

terraform-provider-azurerm/internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

Line 193 in f232d0a

"termination_notification": OrchestratedVirtualMachineScaleSetTerminateNotificationSchema(),

@myc2h6o myc2h6o force-pushed the vm_terminate_notification branch 4 times, most recently from bfdac87 to 2787803 Compare January 13, 2022 05:14
katbyte
katbyte reviewed Jan 13, 2022
View reviewed changes
website/docs/r/windows_virtual_machine.html.markdown Outdated
@@ -287,6 +289,16 @@ A `secret` block supports the following:

---

A `terminate_notification` block supports the following:
Copy link
Collaborator

Choose a reason for hiding this comment

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

grammer, shouldn't this be

Suggested change
A `terminate_notification` block supports the following:
A `termination_notification` block supports the following:

and all documentation should probably use termination instead of terminate

Copy link
Contributor Author

@myc2h6o myc2h6o Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

linux_virtual_machine_scale_set_resource and windows_virtual_machine_scale_set_resource has it named terminate_..., however orchestrated_virtual_machine_scale_set_resource has it named termination_...

terraform-provider-azurerm/internal/services/compute/linux_virtual_machine_scale_set_resource.go

Line 282 in 5361e9c

"terminate_notification": VirtualMachineScaleSetTerminateNotificationSchema(),

terraform-provider-azurerm/internal/services/compute/windows_virtual_machine_scale_set_resource.go

Line 304 in 5361e9c

"terminate_notification": VirtualMachineScaleSetTerminateNotificationSchema(),

terraform-provider-azurerm/internal/services/compute/orchestrated_virtual_machine_scale_set_resource.go

Line 185 in 5361e9c

"termination_notification": OrchestratedVirtualMachineScaleSetTerminateNotificationSchema(),

Since the property name in API is called TerminateNotificationProfile, maybe it's better to use terminate_...? I'll start another pr to align the name in orchestrated_virtual_machine_scale_set_resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @katbyte I've created another change try renaming the property name #15570

Copy link
Contributor Author

@myc2h6o myc2h6o Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Update
--------
I've updated the property name from terminate_notification to termination_notification according to the suggestion

@myc2h6o myc2h6o force-pushed the vm_terminate_notification branch 2 times, most recently from 3d73b67 to 9076eda Compare February 28, 2022 08:12
@myc2h6o myc2h6o changed the title r\VirtualMachine: Add support for terminate_notification r\linux_virtual_machine r\windows_virual_machine: Add support for termination_notification Feb 28, 2022
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Feb 28, 2022

Test result after update:
image

@katbyte katbyte added this to the v3.1.0 milestone Mar 22, 2022
katbyte
katbyte approved these changes Mar 22, 2022
View reviewed changes
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 @myc2h6o - LGTM 🏗️

@katbyte katbyte modified the milestones: v3.1.0, v3.0.0 Mar 22, 2022
@katbyte katbyte merged commit 97b5128 into hashicorp:main Mar 22, 2022
katbyte added a commit that referenced this pull request Mar 22, 2022
@katbyte
CHANGELOG.md for #14933
58cbe4a
@myc2h6o myc2h6o deleted the vm_terminate_notification branch March 22, 2022 02:39
@github-actions
Copy link

This functionality has been released in v3.0.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
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 Apr 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@katbyte katbyte katbyte approved these changes

Assignees
No one assigned
Labels
documentation service/virtual-machine size/XL
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@myc2h6o @katbyte