Bugfix SLO API: Update type for group_by to accept either string or array-of-strings

[gigerdo]

Starting with 8.14.0, the API can now return a list of strings instead of just a string. This causes a crash in the provider when reading from the cluster:

elasticstack % tf plan
elasticstack_kibana_slo.custom_kql: Refreshing state... [id=default/4712ef08-cb91-41ee-be7c-c31b2f02f0e1]

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: json: cannot unmarshal array into Go struct field SloResponse.groupBy of type string
│
│   with elasticstack_kibana_slo.custom_kql,
│   on main.tf line 27, in resource "elasticstack_kibana_slo" "custom_kql":
│   27: resource "elasticstack_kibana_slo" "custom_kql" {
│

To be able to handle both old and new APIs, the spec has been updated to accept either a string or a list of strings.
This update was done manually, as the official Kibana API has not yet been updated to reflect this change.

This change only ensures that the provider doesn't crash with the new API, there is no support yet to configure multiple group-by arguments in the schema.

This is also the reason why the change didn't really cause many code changes: We are not actually using the group-by when reading. It is only passed through when creating/updating.

[tobio]

Have we tested importing this resource from an SLO defined in the 8.14 UI?

The acceptance tests are passing against 8.14 currently so I expect that if groupBy is specified as a string during creation then it's returned as a string. I'm not sure we're properly exercising this code with the current tests.

We are not actually using the group-by when reading. It is only passed through when creating/updating.

This sounds like a bug, and looking at the code isn't one that's that hard to fix. I wonder if it's worth updating the schema for group_by to be a []string and fully fixing this attribute whilst we're here?

[gigerdo]

The acceptance tests are passing against 8.14 currently so I expect that if groupBy is specified as a string during creation then it's returned as a string. I'm not sure we're properly exercising this code with the current tests.

At least from the manual tests I did, the API always returns a list.

This sounds like a bug, and looking at the code isn't one that's that hard to fix. I wonder if it's worth updating the schema for group_by to be a []string and fully fixing this attribute whilst we're here?

I agree that it is another bug. The main effect is that TF doesn't see a diff in the groupBy if it is changed on the server side.

But wouldn't this break the config for all current users of the provider? Meaning they have to change the config after updating. Though at some point we have to switch over to a list anyway.

[tobio]

At least from the manual tests I did, the API always returns a list.

Can we double check that, at least one of the acceptance tests sets group by and I'd expect it to fail if this were the case right?

But wouldn't this break the config for all current users of the provider? Meaning they have to change the config after updating. Though at some point we have to switch over to a list anyway.

There's been a couple of breaking changes in the past, and this would be relatively simple to adjust to (just wrapping the current value in []). I think in theory we shouldn't shy away from making this change.

Another part of the problem is that the schema suggests group by could be a list, but for 8.13 or lower the provider will have to take only the first item and explicitly set it as a plain string.

[gigerdo]

Another part of the problem is that the schema suggests group by could be a list, but for 8.13 or lower the provider will have to take only the first item and explicitly set it as a plain string.

Yeah, I think I will make the change. I have to check what the error is if a list is sent when it isn't supported. Otherwise I guess we need to have a validation for <8.14 to only allow one item in the list.

When serializing I could probably make it work for every case by just always using the string if the list has exactly one element (as the new API supports both string and list-of-string)

Can we double check that, at least one of the acceptance tests sets group by and I'd expect it to fail if this were the case right?

Yeah it should be failing on main 🤔 I have to check why that isn't the case.

[gigerdo]

There's been a couple of breaking changes in the past, and this would be relatively simple to adjust to (just wrapping the current value in []). I think in theory we shouldn't shy away from making this change.

Seems like it isn't enough to just update the config. There also needs to be a state migration. It looks like this is possible in the sdk as well, although it is necessary to copy the whole schema definition.

[tobio]

There also needs to be a state migration

Yea, of course. I'm happy if you want to get this one merged/released and we can fix the rest fixed up in a follow up.

[gigerdo]

Yeah makes sense to split it into two parts. Then we can release a patch fix and maybe put the breaking change into a new minor version.