Alerting Contact Points: Refer by name #1247
Conversation
|
In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically. |
Background: Currently, the contact point's ID is a collection of notifier UIDs. However, in Grafana (and in the resource's import ref), it's referred to by name This PR: Change the Terraform ID from semi-colon separated UIDs to the contact point name. This will be easier to test, and this will allow simplifying the code in the next major version (removing the code fetching notifiers by UID) Since the UID reading logic is moved to the read function, it also allows importing contact points in Crossplane where the import function is not used
julienduchesne
force-pushed
the
julienduchesne/contact-point-refer-by-name
branch
from
07a6c05
to
186930e
Compare
Depends on #1247 When the ID is the contact point's name, it makes it easy to write the test helper
Depends on #1247 When the ID is the contact point's name, it makes it easy to write the test helper
* Alerting Contact Points: Use OpenAPI for tests Depends on #1247 When the ID is the contact point's name, it makes it easy to write the test helper * Rename badly named func
|
Shouldn't this have been announced as a breaking change? As we created the resources with a version prior to this change, our TF state broke after upgrading the provider. As it tried to fetch the contact points based on the ID which was already in state, rather than the name. |
This was tested with IDs in the state. The read by name or ID should still work in the latest version also. As tested here: https://github.com/grafana/terraform-provider-grafana/blob/main/internal/resources/grafana/resource_alerting_contact_point_test.go#L93-L108. (An import test will set only the ID and try to read the resource from that) |
|
Not sure what happend then, because in our case (grafana cloud), we got a
fyi, we go this on updating to grafana provider from 2.7.0 --> 2.10.0 |
huh, interesting. |
|
I redacted the actual uid before posting it here. just like company.grafana.net was not the actual endpoint. |
|
Right 😄. The logic goes that if a contact point isn't found by name (404, or empty list), then it tries by ID. In your case, it's a 500, so that's going to crash the process right away. As if you had gotten a 500 while trying to fetch the contact points by ID. Not sure if the 500 is linked to the API call in particular, the |
|
I just checked, if I query any name, other than an existing one, I get a 500 error instead of 404 or []. For example, Maybe it has to do with how it is implemented in grafana-cloud. |
|
Interesting. I'll test it out and put out a fix |
|
Hey, the 500 error should be fixed soon in Grafana Cloud. It was a Grafana bug, and the fix has been merged to the main branch |
|
@julienduchesne awesome, thanks ! |
Background:
Currently, the contact point's ID is a collection of notifier UIDs. However, in Grafana (and in the resource's import ref), it's referred to by name
This PR:
Change the Terraform ID from semi-colon separated UIDs to the contact point name. This will be easier to test, and this will allow simplifying the code in the next major version (removing the code fetching notifiers by UID) Since the UID reading logic is moved to the read function, it also allows importing contact points in Crossplane where the import function is not used