Don't re-create services if support hours or scheduled actions change (…

…#68)

* Don't re-create services if support hours or scheduled actions change

* Add tests to verify services are not recreated when support hours or scheduled actions change

* Fix test data. Remove test case for scheduled actions as scheduled actions  can never be specified without support hours

* Update documentation

pagerduty/resource_pagerduty_service.go

@@ -121,7 +121,7 @@ func resourcePagerDutyService() *schema.Resource {
 				Optional: true,
 				MaxItems: 1,
 				MinItems: 1,
-				ForceNew: true,
+				ForceNew: false,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"type": {
@@ -152,7 +152,7 @@ func resourcePagerDutyService() *schema.Resource {
 			"scheduled_actions": &schema.Schema{
 				Type:     schema.TypeList,
 				Optional: true,
-				ForceNew: true,
+				ForceNew: false,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"type": {

pagerduty/resource_pagerduty_service_test.go

@@ -371,6 +371,75 @@ func TestAccPagerDutyService_FromBasicToCustomIncidentUrgencyRules(t *testing.T)
 	})
 }
 
+func TestAccPagerDutyService_SupportHoursChange(t *testing.T) {
+	username := fmt.Sprintf("tf-%s", acctest.RandString(5))
+	email := fmt.Sprintf("%s@foo.com", username)
+	escalationPolicy := fmt.Sprintf("tf-%s", acctest.RandString(5))
+	service := fmt.Sprintf("tf-%s", acctest.RandString(5))
+	service_id := ""
+	p_service_id := &service_id
+	updated_service_id := ""
+	p_updated_service_id := &updated_service_id
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckPagerDutyServiceDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccCheckPagerDutyServiceWithIncidentUrgencyRulesConfig(username, email, escalationPolicy, service),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckPagerDutyServiceExists("pagerduty_service.foo"),
+					resource.TestCheckResourceAttr(
+						"pagerduty_service.foo", "name", service),
+					testAccCheckPagerDutyServiceSaveServiceId(p_service_id, "pagerduty_service.foo"),
+				),
+			},
+			{
+				Config: testAccCheckPagerDutyServiceWithSupportHoursConfigUpdated(username, email, escalationPolicy, service),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckPagerDutyServiceExists("pagerduty_service.foo"),
+					resource.TestCheckResourceAttr(
+						"pagerduty_service.foo", "name", service),
+					testAccCheckPagerDutyServiceSaveServiceId(p_updated_service_id, "pagerduty_service.foo"),
+				),
+			},
+		},
+	})
+
+	if service_id != updated_service_id {
+		t.Error(fmt.Errorf("Expected service id to be %s, but found %s", service_id, updated_service_id))
+	}
+}
+
+func testAccCheckPagerDutyServiceSaveServiceId(p *string, n string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs, ok := s.RootModule().Resources[n]
+		if !ok {
+			return fmt.Errorf("Not found: %s", n)
+		}
+
+		if rs.Primary.ID == "" {
+			return fmt.Errorf("No Service ID is set")
+		}
+
+		client := testAccProvider.Meta().(*pagerduty.Client)
+
+		found, _, err := client.Services.Get(rs.Primary.ID, &pagerduty.GetServiceOptions{})
+		if err != nil {
+			return err
+		}
+
+		if found.ID != rs.Primary.ID {
+			return fmt.Errorf("Service not found: %v - %v", rs.Primary.ID, found)
+		}
+
+		*p = found.ID
+
+		return nil
+	}
+}
+
 func testAccCheckPagerDutyServiceDestroy(s *terraform.State) error {
 	client := testAccProvider.Meta().(*pagerduty.Client)
 	for _, r := range s.RootModule().Resources {
@@ -654,3 +723,44 @@ resource "pagerduty_service" "foo" {
 }
 `, username, email, escalationPolicy, service)
 }
+
+func testAccCheckPagerDutyServiceWithSupportHoursConfigUpdated(username, email, escalationPolicy, service string) string {
+	return fmt.Sprintf(`
+resource "pagerduty_user" "foo" {
+	name        = "%s"
+	email       = "%s"
+	color       = "green"
+	role        = "user"
+	job_title   = "foo"
+	description = "foo"
+}
+
+resource "pagerduty_escalation_policy" "foo" {
+	name        = "%s"
+	description = "bar"
+	num_loops   = 2
+
+	rule {
+		escalation_delay_in_minutes = 10
+		target {
+			type = "user_reference"
+			id   = "${pagerduty_user.foo.id}"
+		}
+	}
+}
+
+resource "pagerduty_service" "foo" {
+	name                    = "%s"
+	description             = "foo"
+	auto_resolve_timeout    = 1800
+	acknowledgement_timeout = 1800
+	escalation_policy       = "${pagerduty_escalation_policy.foo.id}"
+
+	incident_urgency_rule {
+		type = "constant"
+		urgency = "high"
+	}
+
+}
+`, username, email, escalationPolicy, service)
+}

website/docs/r/service.html.markdown

@@ -63,8 +63,7 @@ The block contains the following arguments.
   * `during_support_hours` - (Optional) Incidents' urgency during support hours.
   * `outside_support_hours` - (Optional) Incidents' urgency outside of support hours.
 
-When using `type = "use_support_hours"` in `incident_urgency_rule` you have to specify exactly one otherwise optional `support_hours` block.
-Changes to `support_hours` necessitate re-creating the service resource. Account must have the `service_support_hours` ability to assign support hours.
+When using `type = "use_support_hours"` in `incident_urgency_rule` you have to specify exactly one otherwise optional `support_hours` block. Account must have the `service_support_hours` ability to assign support hours.
 The block contains the following arguments.
 
   * `type` - The type of support hours. Can be `fixed_time_per_day`.
@@ -74,7 +73,7 @@ The block contains the following arguments.
   * `start_time` - The support hours' starting time of day.
   * `end_time` - The support hours' ending time of day.
 
-When using `type = "use_support_hours"` in the `incident_urgency_rule` block you have to also specify `scheduled_actions` for the service. Otherwise `scheduled_actions` is optional. Changes necessitate re-createing the service resource.
+When using `type = "use_support_hours"` in the `incident_urgency_rule` block you have to also specify `scheduled_actions` for the service. Otherwise `scheduled_actions` is optional.
 
   * `type` - The type of scheduled action. Currently, this must be set to `urgency_change`.
   * `at` - Represents when scheduled action will occur.