Add type as computed attributes to service & business_service data sources + resources

[jjm]

As was discussed on this weeks PagerDuty Terraform round table, raising a PR to add a dependency_type commuted attribute for the following:

• data pagerduty_service
• data pagerduty_business_service
• resource pagerduty_service
• pagerduty_business_service

Also a correct the repo name in the README, as the repo is now owned by PagerDuty.

ToDo:

• Update docs
• Update tests.

[stmcallister]

@jjm Super cool idea, as we discussed in the roundtable meeting. Remind me how the fact that the Business Service already having a type field is a blocker? 🤔

[jjm]

@stmcallister It was the following type in the schema for resourcePagerDutyBusinessService that I hit a block:

terraform-provider-pagerduty/pagerduty/resource_pagerduty_business_service.go

Lines 43 to 50 in 13bacf4

	"type": {
	Type: schema.TypeString,
	Optional: true,
	Default: "business_service",
	ValidateFunc: validateValueFunc([]string{
	"business_service",
	"business_service_reference",
	}),

As I had added a type to the 2 data sources and to resourcePagerDutyService.

I think maybe the best option is to add a dependancy_type to these resources & data sources instead, so that they don't have a clash with the existing schema.

[jjm]

I've successfully tested this change locally built, with the following config:

terraform {
  required_providers {
    pagerduty = {
      source  = "terraform.example.com/pagerduty/pagerduty"
      version = "2.3.0"
    }
  }
}

provider "pagerduty" {}

data "pagerduty_escalation_policy" "default" {
  name = "Default"
}

resource "pagerduty_business_service" "checkout_app" {
  name = "Checkout App"
}

resource "pagerduty_service" "web_app" {
  name                    = "Our Web App"
  auto_resolve_timeout    = 14400
  acknowledgement_timeout = 600

  escalation_policy = data.pagerduty_escalation_policy.default.id
}

resource "pagerduty_service" "shopping_cart" {
  name                    = "Shopping Cart"
  auto_resolve_timeout    = 14400
  acknowledgement_timeout = 600

  escalation_policy = data.pagerduty_escalation_policy.default.id
}

resource "pagerduty_service" "vault" {
  name                    = "Vault"
  auto_resolve_timeout    = 14400
  acknowledgement_timeout = 600

  escalation_policy = data.pagerduty_escalation_policy.default.id
}

locals {
  vault = [
    pagerduty_business_service.checkout_app,
    pagerduty_service.shopping_cart,
    pagerduty_service.web_app
  ]
}

resource "pagerduty_service_dependency" "vault" {
  for_each = { for k in local.vault : k.id => k.dependency_type }

  dependency {
    dependent_service {
      id   = each.key
      type = each.value
    }
    supporting_service {
      id   = pagerduty_service.vault.id
      type = pagerduty_service.vault.dependency_type
    }
  }
}

The for_each using the local was just what I was wanting to support. The services need created before the dependancies in this example due to Terraform not being able to determine the number of resources. However other use cases will be possible.

[jjm]

@stmcallister Ready for review now and the workflow needs to be approved as this is my first contribution.

[stmcallister]

Thanks @jjm! I love this idea in concept, but still not a fan of adding an additional field to the resources that doesn't exist on the PagerDuty API, if we don't have to.

On the pagerduty_business_service resource there is already a type field, as you mentioned, that is constrained to business_service or business_service_reference. Making this field editable in the provider was my mistake from the get go because the API ignores it on POST and PUT. And, if someone sets the value to business_service_reference in the provider they'll get stuck a permanent diff. However, there still is a possibility that some users are setting the value in their scripts.

With all of that said, I think we should change dependency_type to just type on the pagerduty_service resource.

For the pagerduty_business_service resource we have a couple of options I would love your input on. I would like to use the existing type field. To make sure it's going to provide a value that can be used we can either:

1. Make the type field computed, thus introducing a breaking change, but staying uniform with the API.
2. Leave the field "editable" to not break existing scripts but change the validation to only accept business_service as a value.

What are your thoughts as a user?

[jjm]

@stmcallister Thanks, I think type over dependency_type does makes sense.

I think going with the second option is best, but also mark this attribute as Deprecated following https://www.terraform.io/plugin/sdkv2/best-practices/deprecations#provider-attribute-removal and when there's a major version bump the Deprecated flag is changed to Commuted.

I'll make these changes later on today. I think causing a validation failure when there's a constant diff happening is not so much of an issue.

[jjm]

@stmcallister I've made those changes.

[stmcallister]

Looks great! Just a few tweaks to the docs and we'll be good. Thanks @jjm !!

[jjm]

Thanks @stmcallister, I've addressed your review comments.

[stmcallister]

🎉 🌮 👍 Awesome! Thank you @jjm !!!