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

Update azurerm_linux|windows_virtual_machine_scale_set - change extension block from List to Set #11425

Merged
merged 6 commits into from
May 14, 2021

Conversation

ArcturusZhang
Copy link
Contributor

@ArcturusZhang ArcturusZhang commented Apr 22, 2021

fixes #11367

I am only adding the note comment here since changing this type would be a provider breaking change.
Please let me know if we could make the actual changes in this PR.

@ghost ghost added the size/XS label Apr 22, 2021
@ArcturusZhang ArcturusZhang added the service/vmss Virtual Machine Scale Sets label Apr 22, 2021
@hbuckle
Copy link
Contributor

hbuckle commented Apr 22, 2021

Just to note I feel like the current behaviour is a bug, as changing the order of the extensions doesn't change it on the Azure side, so you get a perpetual diff. So it would be great if it could be fixed here, otherwise I will have to keep using the deprecated scale set resource, which has its own bugs to deal with.

@tombuildsstuff
Copy link
Contributor

@ArcturusZhang @hbuckle

Whilst this would technically be a breaking change - tbh I'd think this is probably fine to change prior to 3.0, since it's not a field which users are likely to interpolate from.

That said, switching this from a List to a Set will make this considerably harder to read in the plan, so ultimately this probably needs the new "Dictionary" type rather than being a Set outright - so in practice this'd likely need to change twice, once to a Set and then again to the new Dictionary type once it's available. In either case both of those are going to require migration testing, since the trade-off here is the Plan which will be unclear (particularly if new fields are added to this block over time) - but I don't think there's much we can do there but test this and see how the UX works

WDYT?

@hbuckle
Copy link
Contributor

hbuckle commented Apr 22, 2021

Sounds good to me, the new dictionary type sounds interesting as well.
For our use case we aren't going to be adding and removing extensions very often, we just have lots of existing scale sets where the extensions are stored in a different order on Azure's side, so having a set will allow us to upgrade to the new resource

@ArcturusZhang
Copy link
Contributor Author

Hi @tombuildsstuff if we all agree to migrate it now, I will then make some more commit to make the changes in this PR.

@ArcturusZhang ArcturusZhang changed the title Update azurerm_linux|windows_virtual_machine_scale_set - adding a note that we should change extension block from List to Set Update azurerm_linux|windows_virtual_machine_scale_set - change extension block from List to Set Apr 23, 2021
@katbyte
Copy link
Collaborator

katbyte commented Apr 27, 2021

@ArcturusZhang - i think we're good to migrate now, and then again in the future to unblock people. just need to make sure we test old -> new versions of the resource

@ArcturusZhang
Copy link
Contributor Author

@ArcturusZhang - i think we're good to migrate now, and then again in the future to unblock people. just need to make sure we test old -> new versions of the resource

I will let you know when I have done the testing on this

@@ -1342,7 +1342,7 @@ func FlattenVirtualMachineScaleSetAutomaticRepairsPolicy(input *compute.Automati

func VirtualMachineScaleSetExtensionsSchema() *schema.Schema {
return &schema.Schema{
Type: schema.TypeList,
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

This'll also need the expand functions changed from .([]interface{}) to .(*schema.Set).List()?

@ArcturusZhang
Copy link
Contributor Author

Hi @tombuildsstuff I had some updates, please take a look, thanks

@katbyte katbyte added this to the v2.60.0 milestone May 14, 2021
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.

LGTM 👍

@katbyte katbyte merged commit db1eba5 into hashicorp:master May 14, 2021
katbyte added a commit that referenced this pull request May 14, 2021
@ArcturusZhang ArcturusZhang deleted the vmss-extensions-should-be-set branch May 18, 2021 04:55
@ghost
Copy link

ghost commented May 21, 2021

This has been released in version 2.60.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.60.0"
}
# ... other configuration ...

@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 Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question service/vmss Virtual Machine Scale Sets size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_windows_virtual_machine_scale_set inline extensions should be a set
4 participants