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

azurerm_proximity_placement_group - fix update when vm is attached #20131

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

myc2h6o
Copy link
Contributor

@myc2h6o myc2h6o commented Jan 20, 2023

Fix #20056

  • Loosen the validation of allowed_vm_sizes, as VM sizes could be added ahead of Spec update, and it is validated at service side
  • In update scenario, when updating allowed_vm_sizes to empty, only explicitly set it to empty when existing property value is not empty. When existing property value is empty, explicitly setting it to empty will cause failure when there is a VM attached to it.

@myc2h6o
Copy link
Contributor Author

myc2h6o commented Jan 20, 2023

Test result
image

@@ -57,7 +56,7 @@ func resourceProximityPlacementGroup() *pluginsdk.Resource {
MinItems: 1,
Elem: &pluginsdk.Schema{
Type: pluginsdk.TypeString,
ValidateFunc: validation.StringInSlice(virtualmachines.PossibleValuesForVirtualMachineSizeTypes(), false),
ValidateFunc: validation.StringIsNotEmpty,
Copy link
Member

Choose a reason for hiding this comment

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

We can't really do this here, whilst it may be lagging behind the swagger updates, not validating the values invites a lot of other problems. Is there a reason there's a lag between the swagger updates and the service updates?

Copy link
Contributor Author

@myc2h6o myc2h6o Feb 8, 2023

Choose a reason for hiding this comment

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

This string slice only populates value for HardwareProfile.vmSize but not the full list of VM Sizes, thus some of the possible values are not contained there. It is currently not applicable to get a static full list, so updating it to the same validation as in VM

Copy link
Collaborator

Choose a reason for hiding this comment

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

does that not indicate that the swagger is incomplete and needs these missing values added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the service is moving away from the static enum list, so the list is incomplete any more. It is said in the swagger that "The enum data type is currently deprecated and will be removed by December 23rd 2023.".

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahh then that would explain why it is not maintained. there isn't much we can do about it then. thanks @myc2h6o

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 once you fix conflicts 🦀

@myc2h6o
Copy link
Contributor Author

myc2h6o commented Feb 28, 2023

Hi @katbyte thanks for reviewing! I've resolved the conflicts

@katbyte
Copy link
Collaborator

katbyte commented Mar 2, 2023

still have conflicts @myc2h6o

@myc2h6o
Copy link
Contributor Author

myc2h6o commented Mar 2, 2023

@katbyte have resolved the second conflicts

@katbyte katbyte merged commit 73bee76 into hashicorp:main Mar 3, 2023
katbyte added a commit that referenced this pull request Mar 3, 2023
@github-actions github-actions bot added this to the v3.46.0 milestone Mar 3, 2023
@myc2h6o myc2h6o deleted the ppg_fix branch March 3, 2023 01:42
@github-actions
Copy link

github-actions bot commented Mar 6, 2023

This functionality has been released in v3.46.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

github-actions bot commented Apr 6, 2023

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 6, 2023
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.

azurerm_proximity_placement_group resource cannot be modified due allowed_vm_sizes
4 participants