feat(slo): Update Terraform provider to include option to set min Failures #1660
Conversation
|
In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically. |
| if failures := tfAlerting["advanced_options"]; failures != nil { | ||
| lf := failures.([]interface{}) |
There was a problem hiding this comment.
| if failures := tfAlerting["advanced_options"]; failures != nil { | |
| lf := failures.([]interface{}) | |
| if advancedOptions, ok := tfAlerting["advanced_options"].([]interface{}); ok { |
it seems that the blocks above this all follow the same pattern with type assertion on the "if" line. Can we not follow that pattern here? Note: the suggestion is using []interface{} not []int (type depends on whether you take suggestion above)
| if failures := tfAlerting["advanced_options"]; failures != nil { | ||
| lf := failures.([]interface{}) | ||
| if len(lf) > 0 { | ||
| lf2 := lf[0].(map[string]interface{}) |
There was a problem hiding this comment.
issue: this will panic if the runtime value of lf[0] fails the type assertion.
Don't you need to use the lf2, ok := lf[0].(map[string]interface{}) version and check ok?
There was a problem hiding this comment.
Interesting point. Yeah I should fix that.
There was a problem hiding this comment.
We actually do this exact same thing for the alerting struct as a whole, the terraform can panic if it doesn't match the expected format. Think I should switch that too?
There was a problem hiding this comment.
yes, if it's easy. If we can handle more gracefully than panic, we should.
where else are we doing it though? The only uncheecked type assertions I see in this function are on line 574 and 576
There was a problem hiding this comment.
I actually misunderstood. the alerting struct handles it all gracefully. I've replicated it now here.
| @@ -150,6 +160,30 @@ func testAlertingExists(expectation bool, rn string, slo *slo.SloV00Slo) resourc | |||
| } | |||
| } | |||
|
|
|||
| func testAdvancedOptionsExists(expectation bool, rn string, slo *slo.SloV00Slo) resource.TestCheckFunc { | |||
There was a problem hiding this comment.
Heh, I guess this works but this could be tested without an additional test function. An import test would've tested that things are written correctly too (it wipes the state and checks that all attributes would be set the same on an import)
We recently have merged new functionality which has updated the SLO CRD. This update brings the terraform provider in line with the new capabilities.
-Doc Update
-New Test for advanced Options
Part of https://github.com/grafana/slo/issues/2070