terraform-provider-pagerduty icon indicating copy to clipboard operation
terraform-provider-pagerduty copied to clipboard

resource pagerduty_service ignoring lifecycle rule for last_incident_timestamp

Open davekaufman opened this issue 3 years ago • 10 comments

Terraform Version

  • Terraform v1.0.0

pagerduty provider version:

  • Installed pagerduty/pagerduty v1.9.9 (signed by a HashiCorp partner, key ID 027C6DD1F0707B45)

Affected Resource(s)

# module.external_services_plaid.pagerduty_service.this has been changed
~ resource "pagerduty_service" "this" {
        id                      = "PG2RQLF"
      ~ last_incident_timestamp = "2021-09-05T16:16:07-07:00" -> "2021-09-09T17:30:16-07:00"
        name                    = "External services Plaid (Service)"
        # (8 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Terraform Configuration Files

resource "pagerduty_service" "this" {
  name                    = "${var.service_name} (Service)"
  escalation_policy       = var.escalation_policy.id
  alert_creation          = "create_alerts_and_incidents"
  acknowledgement_timeout = "null"
  auto_resolve_timeout    = "null"

  lifecycle {
    ignore_changes = [last_incident_timestamp, status]
  }
}

Debug Output

Panic Output

Expected Behavior

per the lifecycle block on the resource, changes to last_incident_timestamp should have been ignored.

Actual Behavior

changes to last_incident_timestamp result in copious irrelevant plan output

Steps to Reproduce

  1. terraform plan

Important Factoids

pretty bog-standard

References

search for last_incident_timestamp in previous issues returned no results.

davekaufman avatar Sep 14 '21 21:09 davekaufman

Seeing this well.

bradyclifford avatar Nov 01 '21 22:11 bradyclifford

Seeing this too, very annoying with the unwanted plan changes

davidferdinand avatar Nov 29 '21 13:11 davidferdinand

Similarly we also see a outside of Terraform change of status in the pagerduty_service rule.

 # module.platform-service.pagerduty_service.service has been changed
  ~ resource "pagerduty_service" "service" {
        id                      = "PAQCMT4"
      + last_incident_timestamp = "2021-12-02T10:18:57+01:00"
        name                    = "Platform"
      ~ status                  = "active" -> "critical"
        # (8 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

idsvandermolen avatar Dec 06 '21 12:12 idsvandermolen

Any update on this?

awilson1801 avatar Jan 26 '22 21:01 awilson1801

Similarly we also see a outside of Terraform change of status in the pagerduty_service rule.

We see the same.

cep21 avatar Mar 03 '22 23:03 cep21

We faced the exactly same problem.

resource "pagerduty_service" "service_hosting" {
  name                    = "Hosting"
  alert_creation          = "create_alerts_and_incidents"
  escalation_policy       = pagerduty_escalation_policy.default.id
  acknowledgement_timeout = "null"
  auto_resolve_timeout    = "null"

  incident_urgency_rule {
    type    = "constant"
    urgency = "high"
  }
  lifecycle {
    ignore_changes = [last_incident_timestamp,]
  }
}
  # pagerduty_service.service_hosting has changed
  ~ resource "pagerduty_service" "service_hosting" {
        id                      = "P2L0PDG"
      ~ last_incident_timestamp = "2023-02-28T17:11:57-05:00" -> "2023-03-03T14:56:30-05:00"
        name                    = "Hosting"
        # (9 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

franklinRibe avatar Mar 03 '23 22:03 franklinRibe

What's the point of tracking these attributes in TF state at all?

bschaeffer avatar Mar 28 '23 15:03 bschaeffer

Proposal:

If people are relying on this attribute they should be using a data resource. Terraform is not meant to manage the state of things that are constantly changing behind the scenes. We should update this provider to no longer track this attribute in state at the resource level.

Is that something the team would be down with?

bschaeffer avatar Apr 26 '23 18:04 bschaeffer

I don't have any stake in this but figured I'd clarify the Terraform behavior a bit and what's proposed in #690.

What Terraform is raising here is drift, not planned changes as some of the comments here suggest. ignore_changes suppresses planned changes, not the detection and reporting of drift during refresh. Because this attribute is Computed and not also Optional, there can never be planned changes.

There has been extensive discussion about confusion around drift reporting:

https://github.com/hashicorp/terraform/issues/28803

The Terraform core team worked on this to try to limit the drift detection reporting to increase the signal to noise:

https://github.com/hashicorp/terraform/pull/30486

The assumption made here is that extra refresh information, i.e "false positives", are a larger impediment to user's understanding of the plan than possibly hidden refresh information, which in most cases can be still inferred from the changes in the plan output. Taking this premise, we record only the specific attribute addresses that contributed to the plan, and filter the output down to those specific references.

No one's mentioned their Terraform version since the initial report, which predated this improvement.


Terraform is not meant to manage the state of things that are constantly changing behind the scenes.

This is not correct. Computed attributes exist so that Terraform can track but not manage certain attributes of a resource that are determined by the server. Including server-computed timestamps that might be valuable to the user is common/valid:

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/db_instance#latest_restorable_time

If people are relying on this attribute they should be using a data resource.

This is definitely not correct, at least so long as it implies only data.pagerduty_service.last_incident_timestamp should exist but not the equivalent resource attribute. Providers strive to keep the data source and resource for a given object of the same general shape for good reasons. One is that Terraform emphasizes the use of object types, e.g.:

variable "pagerduty_service" {
  type = object({
    id = string
    name = string
    last_incident_timestamp = string
  })
}

Whether last_incident_timestamp is useful or not in Terraform is certainly debatable. If you wanted logic that says "create some dependent resource if this service has had an incident in the last n months" you'd need it. Whether this is too contrived to be worthwhile isn't for me to say.

To the extent a data source is the answer, it's not diverging from the resource, it's a new one, data.pagerduty_service_status.


And then from #690:

To avoid a backwards incompatible change, would it make sense to keep these in the resource state (and add Deprecated to them) but with a constant value (e.g. unknown and 1970-01-01T00:00:00.000Z)?

This is not any less breaking than removing the attributes entirely. If users are depending on them, breaking their behavior but preserving the structure is still breaking.

bendrucker avatar Aug 04 '23 20:08 bendrucker

A computed attribute is generally the result of inputs though. If the inputs change that drive it, than the computed attribute should to. If computed attributes are changing constantly without any change to the inputs that's definitely an anti-pattern.

Exposing this as an attribute on the data object only and removing last_incident_timestamp from the resource in newer versions makes the most sense to me.

bschaeffer avatar Aug 14 '23 23:08 bschaeffer