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

Fix azurerm_application_insights_smart_detection_rule to support/configure all rules #12012

Closed
wants to merge 4 commits into from

Conversation

komal-SkyNET
Copy link

@komal-SkyNET komal-SkyNET commented May 31, 2021

Addresses the issue: #11999

In the smart detection resource, there is currently a validation(which has become a restriction) – that accepts only a set of rule names – that is hindering support for new rules as and when they become available. This also requires us to update our code every time a rule name is added/updated/deleted by microsoft.

To simplify this, I propose to remove this restriction so that users can reference microsoft docs and pass the internal rule name as required. This will also decouple our code from Microsoft's updates to old/new rules/names.

- this allows all smart detection rules to be controlled from the provider
…or internal rule name

-update unit tests for new rules
@ghost ghost added the size/M label May 31, 2021
@komal-SkyNET komal-SkyNET changed the title allow azurerm_application_insights_smart_detection_rule to configure all rules azurerm_application_insights_smart_detection_rule to support/configure all rules May 31, 2021
@komal-SkyNET komal-SkyNET changed the title azurerm_application_insights_smart_detection_rule to support/configure all rules azurerm_application_insights_smart_detection_rule needs to support/configure all rules May 31, 2021
@komal-SkyNET
Copy link
Author

Resolves #11999

@komal-SkyNET komal-SkyNET changed the title azurerm_application_insights_smart_detection_rule needs to support/configure all rules azurerm_application_insights_smart_detection_rule needs to support/configure all rules May 31, 2021
@komal-SkyNET komal-SkyNET changed the title azurerm_application_insights_smart_detection_rule needs to support/configure all rules Fix azurerm_application_insights_smart_detection_rule to support/configure all rules May 31, 2021
@ghost ghost added the documentation label May 31, 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.

Thanks for opening this pr, however we do not just remove validation from constants like this (and fwiw these should be added to the SDK) except in some exceptional circumstances.

instead could we update the list with the new constants? thanks

// The Smart Detection Rule name from the UI doesn't match what the API accepts.
// We'll have the user submit what the name looks like in the UI and trim it behind the scenes to match what the API accepts
name := strings.ToLower(strings.Join(strings.Split(d.Get("name").(string), " "), ""))
name := strings.Join(strings.Split(strings.ToLower(d.Get("name").(string)), " "), "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a breaking change so we can't change this behaviour till 3.0, could we add a comment to that affect?

Comment on lines -39 to -41
"Slow page load time",
"Slow server response time",
"Long dependency duration",
Copy link
Collaborator

Choose a reason for hiding this comment

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

and we'll need to keep these for people who are still using them

@katbyte
Copy link
Collaborator

katbyte commented Aug 5, 2021

@komal-SkyNET - i'm going to close this as #12857 has been opened which addresses my feedback - thanks

@komal-SkyNET
Copy link
Author

Thanks @katbyte! Sorry, didn't get a chance to look at this after our last conversation. Glad that it's been addressed. Do we know which release version it will be included in?

@github-actions
Copy link

github-actions bot commented Sep 6, 2021

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 Sep 6, 2021
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.

2 participants