cloudamqp / terraform-provider-cloudamqp

Terraform Provider for CloudAMQP
https://registry.terraform.io/providers/cloudamqp/cloudamqp
Mozilla Public License 2.0
35 stars 39 forks source link

fix: added handling of notice alarm #265

Closed arthmoeros closed 7 months ago

arthmoeros commented 7 months ago

Given this issue as context:

https://github.com/cloudamqp/terraform-provider-cloudamqp/issues/219

I'm proposing with this PR that notice alarm "auto-imports" into state on resource "creation", and also apply any changes to the existing notice alarm (that the API allows).

dentarg commented 7 months ago

@tbroden84 I think this could work? You see any problems with it?

tbroden84 commented 7 months ago

Looks good, will try it out a bit later today.

tbroden84 commented 7 months ago

Would also suggest adding the notice type check to resourceAlarmDelete too. Since our API backend don't allow notice alarm to be deleted, it's a mandatory alarm. We can skip sending this request but still remove it from the state file. Something like:

if d.Get("type") == "notice" {
  return nil
}
return api.DeleteAlarm(d.Get("instance_id").(int), params)

Otherwise like the solution! Hadn't thought about this approach at all.

arthmoeros commented 7 months ago

Would also suggest adding the notice type check to resourceAlarmDelete too. Since our API backend don't allow notice alarm to be deleted, it's a mandatory alarm. We can skip sending this request but still remove it from the state file. Something like:

if d.Get("type") == "notice" {
  return nil
}
return api.DeleteAlarm(d.Get("instance_id").(int), params)

Otherwise like the solution! Hadn't thought about this approach at all.

Yeah, I was thinking about it yesterday, will add that change also so destroy only removes it from the state file

arthmoeros commented 7 months ago

There @tbroden84 , added handling of delete on notice alarm, thanks for the feedback