Fixing issue with AlertRuleGroupExport data model

[safaci2000]

Fixing issue with AlertRuleGroupExport data model

For more context: https://github.com/grafana/grafana/issues/94936

[dannylongeuay]

The same needs to be done for the AlertRuleExport.For field as well.

[safaci2000]

The same needs to be done for the AlertRuleExport.For field as well.

Updated. Thanks for calling that out.

[safaci2000]

Any thoughts on getting merged this in? objections or otherwise?

[safaci2000]

Any updates on this issue?

[safaci2000]

What's the use-case you're trying to fix exactly? I see there is a workaround in Grafana, but only covers hcl exports (here). I don't have much knowledge over this, but it feels like we should tackle it there instead of simply overriding the types here.

Other than that:

1. You'll probably want to handle KeepFiringFor as well (check the merge conflict)
2. It would be nice if you could validate this doesn't break grafana and the terraform provider, since they both make use of this

TLDR:

Basically the Alert API is broken and there is a mismatch in what the swagger definition is reporting and what the actual response is.

The current code is:

AlertRuleGroupExport
	// interval
	Interval Duration `json:"interval,omitempty"`

My change would change the data type ant accurately return the value as a string. Any attempt to use the API in the current state with any interval value would break.

1. Will do. I'll add that along. (Done)
2. I'm not as familiar with that code base. Wouldn't this come up when they update this library and be covered by their testing? What I'm getting at is that it should not be auto-updated, so someone is making a choice to upgrade and ideally test it?

I can take a stab at confirming this is safe , but I'm not very familiar wit TF yet.

There's another ticket with an example of this failure: https://github.com/grafana/grafana-openapi-client-go/issues/110.

So while I agree that the better fix would be to fix the actual schema, this isn't the first use case where this API in patching the grafana schema that is invalid. This workaround would allow users who want to be able to use the Alert API while we wait for a better solution from that group.

I mean to quote the script I was modifying:

# Playlist models are all messed up
# TODO: Upstream fix

@titolins

[titolins]

Hey @safaci2000 👋

Thanks for the answer.

My change would change the data type ant accurately return the value as a string.

Yep, that's clear. I'm just not sure patching it on the openapi client is the right way to go. I've raised this with the team now to check if anyone has any concerns about this and will update here 👍

Basically the Alert API is broken ... Any attempt to use the API in the current state with any interval value would break.

Just being nit-picky, but the API itself is not broken. The client is 👍

Wouldn't this come up when they update this library and be covered by their testing? What I'm getting at is that it should not be auto-updated, so someone is making a choice to upgrade and ideally test it?

As a general rule of thumb, you'd usually want the responsible for the changes to handle the updates. I don't think this is required here, but at least checking for breaking changes would be great 😄

[safaci2000]

Just being nit-picky, but the API itself is not broken. The client is 👍

Okay, fair enough but the client is still in an usable state. :)

Sounds good, over all.

If there's anything else I could do on my end for this update, please let me know. I would really like to get alerts into my code base without having to fork the official client.

[titolins]

Just for context:

• Dropped the cherry-pick and rebased off of latest upstream main instead
• Ran make generate-client to include all changes

cc @safaci2000

[safaci2000]

Sound good. Thank you for merging this in!