newrelic / terraform-provider-newrelic

Terraform provider for New Relic
https://registry.terraform.io/providers/newrelic/newrelic/latest/docs
Mozilla Public License 2.0
202 stars 245 forks source link

fix(dashboards): fix to dashboard name update failure with filter_current_dashboard #2611

Closed pranav-new-relic closed 6 months ago

pranav-new-relic commented 6 months ago

Background

Via a recent support case, it was observed that if the name of a dashboard is updated, it wouldn't get updated (neither in the Terraform state, nor in the UI) while all other attributes would.

Upon further investigation (lots of trial and error), it was identified that this was happening because two API calls went out when Terraform would try updating configuration, which made obvious that the following section of the code was the culprit (as this comprises the second update call performed within the update function).

This also made clear that the failure of the dashboard's name getting updated was seen only upon using the attribute filter_current_dashboard as control flow would only then enter the following section of code as the number of widgets to filter would be > 0. In any way, the second update call was causing this issue, and the following section explains how.

https://github.com/newrelic/terraform-provider-newrelic/blob/429cdb953f81c6f141a5858bccf1f31429e5447c/newrelic/resource_newrelic_one_dashboard.go#L765-L780

The Problem

If no widgets to be filtered are specified (e.g. no filter_current_dashboard), control flow does not enter this block and the flow works as depicted in the following diagram. In summary, though the read call after the update API call gets the old (wrong) name of the dashboard, the last function called in the update function makes sure the name obtained in the response of the update API call is set to the state.

image

On the contrary, with widgets to filter, control flow enters the block of code specified above, and a second update call is made. Since this second update call makes use of expandDashboardInput which gets the name of the dashboard using d.Get("name") and prior to this function call, the read function would set the old dashboard's name already (because of late entity indexing), this old name is fetched and put into the body of the request used in the second update call, leading to the updated name being re-updated to the old name via the second update call. This continues to remain in the state (and also, obviously, does not reflect in the UI because we have "updated" it back to its old name).

image

The Solution

When control flow enters the block specified above, we need to make sure the old name of the dashboard is not used and the name fetched from the response of the first update call is used. This PR proposes changes along the lines of the same.

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 34.20%. Comparing base (2292003) to head (88b308e).

Files Patch % Lines
newrelic/structures_newrelic_one_dashboard.go 0.00% 7 Missing :warning:
newrelic/resource_newrelic_one_dashboard.go 0.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2611 +/- ## ========================================== + Coverage 32.99% 34.20% +1.21% ========================================== Files 98 98 Lines 26610 26613 +3 ========================================== + Hits 8780 9104 +324 + Misses 17672 17318 -354 - Partials 158 191 +33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Nandu-pns commented 6 months ago

Thanks for the detailed context, changes looks good to me.