Fix optional threshold fields handling in update function #400
Conversation
ghost
added
the
|
Hey, thanks for sending the PR! I think it looks good, but I'd like to ask you to extend the test suite of SLOs to also test for this. |
|
We're hitting this exact issue ATM, looking forward to a release with this fix! |
Ok no problem, I'll do that as soon as possible |
|
This should be good now, I added checks on the warning attribute, for the creation and the update |
|
@mzneos the tests don't actually pass. The reason is that you're checking for non-presence of attribute which actually is present in the state, but set to 0 (this is TF behavior for when such nested structures are used). The code patch that you created is actually correct as it ignores these during update when they were not explicitly set before. The only thing you need to adjust is remove the |
|
Ok I see. I was expecting the I removed those checks, everything should be good now. |
| @@ -136,6 +150,12 @@ func TestAccDatadogServiceLevelObjective_Basic(t *testing.T) { | |||
| "datadog_service_level_objective.foo", "thresholds.1.timeframe", "30d"), | |||
| resource.TestCheckResourceAttr( | |||
| "datadog_service_level_objective.foo", "thresholds.1.target", "98"), | |||
| resource.TestCheckResourceAttr( | |||
| "datadog_service_level_objective.foo", "thresholds.1.warning", "99.0"), | |||
There was a problem hiding this comment.
One last thing, this needs to be changed from 99.0 to 99 (because of how TF works internally). Otherwise this LGTM, so once you fix this, I can merge. Thanks @mzneos!
There was a problem hiding this comment.
Oh ok, my bad. I added this check as it was missing, and just copy-pasted the value without thinking further than that.
I should have run the acceptance tests by myself to make sure everything was working, but wasn't really sure of how to properly do it, sorry for that.
It is fixed now
Optional fields in the threshold object were always added to the update payload, even when they were not actually set in the state or the configuration.
This broke the update function as the
warningfield cannot be set to a lower value that the slo value (target), which triggers awarning must be greater than slo valueerror.If the warning field was not set, the
ResourceData.Get()function would return the default value for the field, which is0.0for a float, hence the error being triggered.This PR fixes this problem by using the
ResourceData.GetOk()to check if the field was actually set or not.The code was compiled and tested for two scenarios, both working as expected: