-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add type as computed attributes to service & business_service data sources + resources #364
Conversation
@jjm Super cool idea, as we discussed in the roundtable meeting. Remind me how the fact that the Business Service already having a |
@stmcallister It was the following terraform-provider-pagerduty/pagerduty/resource_pagerduty_business_service.go Lines 43 to 50 in 13bacf4
As I had added a I think maybe the best option is to add a |
d94ec7d
to
73373d5
Compare
ed2d7d0
to
f9a4ada
Compare
…sources docs: update the docs for the addition of dependency_type feat: Use dependency_type instead of type due to a clash on resourcePagerDutyBusinessServiceRead docs: Update clone instructions in README.md
f9a4ada
to
4cd226b
Compare
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 |
@stmcallister Ready for review now and the workflow needs to be approved as this is my first contribution. |
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 With all of that said, I think we should change For the
What are your thoughts as a user? |
@stmcallister Thanks, I think 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 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. |
…ice (+ make type on pagerduty_business_service as Deprecated)
ab0207a
to
c16f50a
Compare
@stmcallister I've made those changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a few tweaks to the docs and we'll be good. Thanks @jjm !!
fc0a45e
to
ccd1246
Compare
Thanks @stmcallister, I've addressed your review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🌮 👍 Awesome! Thank you @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: