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_application_insights - support for the daily_data_cap_in_gb & daily_data_cap_notifications_disabled properties #5480

Merged
merged 5 commits into from
Jan 31, 2020

Conversation

wasfree
Copy link
Contributor

@wasfree wasfree commented Jan 22, 2020

This PR should fix #584, so we can set now a daily cap for Applications Insight in GB. Beside setting a daily cap for Applications Insight it's also possible now to disable notifications if daily cap is hit.

Compared to other resources this configuration is separated to it's own billing feature api. Which requires to update billing feature api once Application Insight was created.

@wasfree wasfree requested a review from katbyte January 24, 2020 09:05
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 the PR @NickMetz! overall this looks great, but i have some questions i've left inline

@@ -89,6 +89,19 @@ func resourceArmApplicationInsights() *schema.Resource {

"tags": tags.Schema(),

"daily_cap": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be better as

Suggested change
"daily_cap": {
"daily_traffic_cap_in_gb": {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This option is related to data which is actually stored in applications insight. So my recommendation would be daily_data_cap_in_gb. What you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

works for me!

ValidateFunc: validation.FloatBetween(0, 1000),
},

"stop_send_notification_when_hit_cap": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This property is awfully odd, could we invert and reword it to

Suggested change
"stop_send_notification_when_hit_cap": {
"daily_traffic_cap_notifications_enabled": {

where true -> sends false, false -> send true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @katbyte,
agree with you awfully odd. But i think we should name it the other way. Because in general people will only touch this option if they want to stop notifications if cap is hit e.g. disabled = true.

daily_data_cap_notifications_disabled
true -> don't send messages
false -> send messages
default is false

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with daily_data_cap_notifications_disabled, thanks!

Nick Metz added 2 commits January 25, 2020 22:16
…llingClient, remove defaults switch to computed, rename billing feature options
@wasfree wasfree requested a review from katbyte January 26, 2020 10:39
@wasfree
Copy link
Contributor Author

wasfree commented Jan 30, 2020

@katbyte can you please check my two open questions. If my recommendation is ok, this PR is ready to merge.

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 those changes @NickMetz! LGTM now 👍

@katbyte katbyte merged commit 0446b69 into hashicorp:master Jan 31, 2020
@katbyte katbyte changed the title Fix issue #584 add option to set AI daily cap and hit daily cap stop notify azurerm_application_insights - support for the daily_data_cap_in_gb & daily_data_cap_notifications_disabled properties Jan 31, 2020
katbyte added a commit that referenced this pull request Jan 31, 2020
@ghost
Copy link

ghost commented Feb 4, 2020

This has been released in version 1.43.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 = "~> 1.43.0"
}
# ... other configuration ...

@ghost ghost removed the waiting-response label Feb 4, 2020
@ghost
Copy link

ghost commented Mar 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
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.

Feature Request: Support for Application Insights billing & pricing
2 participants