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

r\linux_virtual_machine_scale_set r\windows_virtual_machine_scale_set: Rename terminate_notification to termination_notification #15570

Merged
merged 6 commits into from
Apr 29, 2022

Conversation

myc2h6o
Copy link
Contributor

@myc2h6o myc2h6o commented Feb 23, 2022

Updated per comment
--------
Updating the property name to the correct word and match the one in r\orchestrated_virtual_machine_scale_set.

"termination_notification": OrchestratedVirtualMachineScaleSetTerminateNotificationSchema(),

This property is Optional + Computed, so in the Update function, by simply checking the HasChange for each of them, it can handle the migration from old property name to new one (If property name changes with value remaining same, no changes are detected).

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.

@myc2h6o - could we instead rename terminate to termination as from the docs this is a "termination" notification and that is the correct word, and is more explicit on what this does.

thanks

…set`: Rename `terminate_notification` to `termination_notification`
@myc2h6o myc2h6o force-pushed the termination_notification_rename branch from 56c66ef to 1fcf5bf Compare February 28, 2022 07:39
@myc2h6o myc2h6o changed the title r\orchestrated_virtual_machine_scale_set: Rename termination_notification to terminate_notification r\linux_virtual_machine_scale_set r\windows_virtual_machine_scale_set: Rename terminate_notification to termination_notification Feb 28, 2022
@myc2h6o myc2h6o changed the title r\linux_virtual_machine_scale_set r\windows_virtual_machine_scale_set: Rename terminate_notification to termination_notification r\linux_virtual_machine_scale_set r\windows_virtual_machine_scale_set: Rename terminate_notification to termination_notification Feb 28, 2022
@github-actions github-actions bot added size/L and removed size/XL labels Feb 28, 2022
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Feb 28, 2022

Hi @katbyte Thanks for the comment, yes termination_notification seems more reasonable. I've updated the pr, please take a look

@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 modified the milestones: v3.1.0, v3.2.0 Apr 7, 2022
@katbyte katbyte modified the milestones: v3.2.0, v3.3.0 Apr 14, 2022
@katbyte katbyte modified the milestones: v3.3.0, v3.4.0 Apr 22, 2022
@github-actions
Copy link

This functionality has been released in v3.4.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 github-actions bot added size/XL and removed size/L labels Apr 26, 2022
@myc2h6o myc2h6o force-pushed the termination_notification_rename branch from d038282 to dd5af47 Compare April 26, 2022 15:42
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Apr 27, 2022

Updated test result
image

}

if !features.FourPointOhBeta() {
out["terminate_notification"] = VirtualMachineScaleSetTerminateNotificationSchema()
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be adding a deprecated message to this property directing users to the new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's added in VirtualMachineScaleSetTerminateNotificationSchema in virtual_machine_scale_set.go:

Deprecated: "`terminate_notification` has been renamed to `termination_notification` and will be removed in 4.0.",

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah perfect! thanks :)

Optional: true,
Computed: true,
MaxItems: 1,
Deprecated: "`terminate_notification` has been renamed to `termination_notification` and will be removed in 4.0.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecation notice is put here

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 🚀

}

if !features.FourPointOhBeta() {
out["terminate_notification"] = VirtualMachineScaleSetTerminateNotificationSchema()
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah perfect! thanks :)

@katbyte katbyte added the service/vmss Virtual Machine Scale Sets label Apr 29, 2022
@katbyte katbyte merged commit 1952a4e into hashicorp:main Apr 29, 2022
katbyte added a commit that referenced this pull request Apr 29, 2022
@myc2h6o myc2h6o deleted the termination_notification_rename branch April 29, 2022 02:31
@github-actions
Copy link

This functionality has been released in v3.4.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 May 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants