Feature: azurem_application_insights add support for retention_in_days
#5457
Conversation
There was a problem hiding this comment.
Hey @neil-yechenwei, there is a breaking change here that I've documented below.
| "retention_in_days": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Default: 90, |
There was a problem hiding this comment.
Unfortunately, this is a breaking change since the API doesn't provide retention_in_days by default. This means that users migrating to this update will see a change like retention_in_days = 0 -> 90. We can't have this so this default should be removed.
There was a problem hiding this comment.
If the API returns a default we can probably just use computed instead
There was a problem hiding this comment.
API returns nil. So I removed the default value.
| @@ -35,4 +35,5 @@ output "application_insights_instrumentation_key" { | |||
| * `application_type` - The type of the component. | |||
| * `instrumentation_key` - The instrumentation key of the Application Insights component. | |||
| * `location` - The Azure location where the component exists. | |||
| * `retention_in_days` - The retention period in days. Possible values are `30`, `60`, `90`, `120`, `180`, `270`, `365`, `550` or `730`. | |||
There was a problem hiding this comment.
This is a datasource so can we word this as such
There was a problem hiding this comment.
LGTM! And tests are passing. Thanks for taking the time to get write this out @neil-yechenwei
mbfrahry
changed the title
azurem_application_insights add support for retention_in_days
|
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
locked and limited conversation to collaborators
Fix the issue of #5441