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_log_analytics_workspace - Allow field value updates while workspace is linked to a cluster #17069

Merged
merged 5 commits into from
Jun 3, 2022

Conversation

WodansSon
Copy link
Collaborator

The block on SKU change behavior was implemented incorrectly, this fixes the old implementation for the correct behavior. This is currently blocking customers.

@mozts2005
Copy link
Contributor

mozts2005 commented Jun 2, 2022

@WodansSon Why just update to use the new API version then add the missing properties and enum options.

https://github.com/Azure/azure-sdk-for-go/blob/main/services/operationalinsights/mgmt/2020-10-01/operationalinsights/enums.go

@tombuildsstuff
Copy link
Contributor

@mozts2005 that would require config changes post-apply - the "right" way to fix this is by splitting the Create and Update methods here to allow users to use ignore_changes in the config, but that'd also require config changes at this point so would be a major version change at this point.

@mozts2005
Copy link
Contributor

mozts2005 commented Jun 2, 2022

Why can't you just handle the config changes the way they are done for other services like when AKS creates a new version of the API?

example: AKS addon_profile being removed

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

One comment around a TODO - and we should add an acctest for this scenario - but this should otherwise be fine 👍

Comment on lines 78 to 80
Optional: true,
ForceNew: true,
Default: string(operationalinsights.WorkspaceSkuNameEnumPerGB2018),
Computed: true,
ValidateFunc: validation.StringInSlice([]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll need to add a TODO here to add the new SKU in 4.x, remove computed, split the Create/Update and tell users to use ignore_changes here fwiw - since that's how this should be updated longer-term

Copy link
Contributor

Choose a reason for hiding this comment

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

just handle it the same way it was handled for AKS

Copy link
Contributor

Choose a reason for hiding this comment

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

@mozts2005 unfortunately that'd be a series of breaking changes which we can't ship until the next major version, so that's not possible in a minor release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why they would be braking changes you could just remap the old values to the new values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

also, your changes do not allow of creating a new LAW with the cluster settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

my need to is to make so I can use azurerm_log_analytics_workspace and azurerm_log_analytics_cluster together in an Idempotent fashion from creation and in the case of an update.

Choose a reason for hiding this comment

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

This is by design, new LAWs can not be created with the cluster settings.

@WodansSon
Copy link
Collaborator Author

The only test that fails is due to the billing configuration of the subscription:
image

@WodansSon
Copy link
Collaborator Author

image

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 ♻️

@WodansSon WodansSon changed the title azurerm_log_analytics_workspace - Update SKU behavior azurerm_log_analytics_workspace - Allow field value updates while workspace is linked to a cluster Jun 3, 2022
@WodansSon WodansSon merged commit 179160a into main Jun 3, 2022
@WodansSon WodansSon deleted the b_law_sku branch June 3, 2022 00:05
WodansSon added a commit that referenced this pull request Jun 3, 2022
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

This functionality has been released in v3.9.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 Jul 4, 2022

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 Jul 4, 2022
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.

5 participants