Skip to content
This repository has been archived by the owner on Feb 14, 2024. It is now read-only.

fix!: add app insights worker mode (#146) #175

Merged
merged 3 commits into from
Oct 5, 2022

Conversation

jblaszc
Copy link
Contributor

@jblaszc jblaszc commented Aug 5, 2022

Description

Adding support for Application Insights workspace-based mode and setting it as the default.

BREAKING CHANGE: default behaviour as well as variables used to define Application Insights of the vmseries and vmss module will change.

Motivation and Context

For the moment the vmseries module support creation of classic AI only. There are regions where only workspace mode for AI is supported. This causes the module to crash during AI provisioning.

Azure recommends to use a workspace-based Application Insights mode now. All new Azure regions introduced after Feb 2021 do not support creating classic AI resources. For the older regions, support for classic AI mode will end on Feb 29th 2024.

This fix sets the workspace-based as the default mode if Application Insights is required. In case the Classic mode is needed, it can be forced by setting app_insights_settings.workspace_mode variable to false.

This PR:

How Has This Been Tested?

A vmseries (standalone and scale set) examples were built with AI mode set to workspace-based and Classic.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

@jblaszc jblaszc requested review from pimielowski and FoSix August 5, 2022 07:14
@jblaszc jblaszc added bug Something isn't working azure labels Aug 5, 2022
@jblaszc jblaszc linked an issue Aug 5, 2022 that may be closed by this pull request
Copy link
Contributor

@pimielowski pimielowski left a comment

Choose a reason for hiding this comment

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

Looks good :)

modules/vmseries/main.tf Outdated Show resolved Hide resolved
modules/vmseries/README.md Outdated Show resolved Hide resolved
modules/vmseries/variables.tf Outdated Show resolved Hide resolved
modules/vmss/main.tf Outdated Show resolved Hide resolved
@jblaszc jblaszc force-pushed the issue146_app-insights_workspace branch 2 times, most recently from 6c18442 to 72eb2f7 Compare August 12, 2022 14:49
Copy link
Contributor

@FoSix FoSix left a comment

Choose a reason for hiding this comment

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

👍🏻

modules/vmseries/variables.tf Outdated Show resolved Hide resolved
modules/vmseries/variables.tf Outdated Show resolved Hide resolved
modules/vmseries/variables.tf Outdated Show resolved Hide resolved
modules/vmseries/main.tf Outdated Show resolved Hide resolved
modules/vmseries/variables.tf Outdated Show resolved Hide resolved
@jblaszc jblaszc force-pushed the issue146_app-insights_workspace branch 2 times, most recently from f512c6c to fe983f6 Compare August 30, 2022 15:18
@jblaszc jblaszc changed the title fix: add app insights worker mode (#146) bug: add app insights worker mode (#146) Sep 7, 2022
    * fix(modules/vmseries): add 'azurerm_log_analytics_workspace' resource
                             update 'azurerm_application_insights' so it can use workspaces
                             add 'app_insights_settings' map variable that holds all app_insights related parameters
                             remove 'name_application_insights' variable, moved to 'app_insights_settings' var
                             remove 'metrics_retention_in_days' variable, moved to 'app_insights_settings' var

    * fix(modules/vmss):     add 'azurerm_log_analytics_workspace' resource
                             update 'autoscale_metrics' defaults values to empty map
                             update 'azurerm_application_insights' so it can use workspaces
                             add 'app_insights_settings' map variable that holds all app_insights related parameters

    * fix(examples/vmss):    update example, so the new 'app_insights_settings' variable is used

    * docs(module/vmseries): add 'app_insights_settings' variable description

    * docs(module/vmss):     add 'app_insights_settings' variable description
@jblaszc jblaszc force-pushed the issue146_app-insights_workspace branch from 3bf565f to d9df242 Compare September 7, 2022 15:07
@FoSix FoSix marked this pull request as draft September 27, 2022 07:32
@FoSix FoSix changed the title bug: add app insights worker mode (#146) fix: add app insights worker mode (#146) Sep 27, 2022
@FoSix FoSix changed the title fix: add app insights worker mode (#146) fix!: add app insights worker mode (#146) Sep 30, 2022
@FoSix FoSix marked this pull request as ready for review September 30, 2022 11:04
@FoSix FoSix requested a review from a team as a code owner September 30, 2022 11:04
Copy link
Contributor

@pimielowski pimielowski left a comment

Choose a reason for hiding this comment

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

Looks good 👍 :)

modules/vmseries/variables.tf Show resolved Hide resolved
Copy link
Contributor

@FoSix FoSix left a comment

Choose a reason for hiding this comment

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

👍🏻

@FoSix FoSix merged commit e22e513 into develop Oct 5, 2022
@FoSix FoSix deleted the issue146_app-insights_workspace branch October 5, 2022 10:47
@welcome-to-palo-alto-networks
Copy link

🎉 Congrats on getting your first pull request merged! We here at Palo Alto Networks are so grateful! ❤️

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

🎉 This PR is included in version 0.5.0 🎉

The release is available on Terraform Registry and GitHub release

Posted by semantic-release bot

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
azure bug Something isn't working released
Projects
None yet
4 participants