Merge pull request #918 from mdb/mdb/issue-913

fix: improve team_membership deletion logic

pagerduty/resource_pagerduty_team_membership.go

@@ -203,13 +203,14 @@ func resourcePagerDutyTeamMembershipDelete(d *schema.ResourceData, meta interfac
 
  log.Printf("[DEBUG] Removing user: %s from team: %s", userID, teamID)
 
- // Extracting Escalation Policies ids where this team referenced
- epsAssociatedToUser, err := extractEPsAssociatedToUser(client, userID)
+ // Extract Escalation Policies associated to the team for which the userID is
+ // a rule target.
+ epsAssociatedToTeamAndUser, err := extractEPsAssociatedToTeamAndUser(client, teamID, userID)
  if err != nil {
  return err
  }
 
- epsDissociatedFromTeam, err := dissociateEPsFromTeam(client, teamID, epsAssociatedToUser)
+ epsDissociatedFromTeam, err := dissociateEPsFromTeam(client, teamID, epsAssociatedToTeamAndUser)
  if err != nil {
  return err
  }
@@ -240,20 +241,21 @@ func resourcePagerDutyTeamMembershipDelete(d *schema.ResourceData, meta interfac
  return nil
 }
 
-func buildEPsIdsList(l []*pagerduty.OnCall) []string {
+func buildEPsIdsList(l []*pagerduty.EscalationPolicy) []string {
  eps := []string{}
  for _, o := range l {
- if o.EscalationPolicy != nil {
- eps = append(eps, o.EscalationPolicy.ID)
- }
+ eps = append(eps, o.ID)
  }
  return unique(eps)
 }
 
-func extractEPsAssociatedToUser(c *pagerduty.Client, userID string) ([]string, error) {
- var oncalls []*pagerduty.OnCall
+// extractEPsAssociatedToTeamAndUser returns the IDs of escalation policies
+// associated to the specified team and for which the specified user is a rule
+// target.
+func extractEPsAssociatedToTeamAndUser(c *pagerduty.Client, teamID, userID string) ([]string, error) {
+ var eps []*pagerduty.EscalationPolicy
  retryErr := retry.Retry(2*time.Minute, func() *retry.RetryError {
- resp, _, err := c.OnCall.List(&pagerduty.ListOnCallOptions{UserIds: []string{userID}})
+ resp, _, err := c.EscalationPolicies.List(&pagerduty.ListEscalationPoliciesOptions{TeamIDs: []string{teamID}})
  if err != nil {
  if isErrCode(err, http.StatusBadRequest) {
  return retry.NonRetryableError(err)
@@ -262,14 +264,28 @@ func extractEPsAssociatedToUser(c *pagerduty.Client, userID string) ([]string, e
  time.Sleep(2 * time.Second)
  return retry.RetryableError(err)
  }
- oncalls = resp.Oncalls
+ eps = resp.EscalationPolicies
  return nil
  })
  if retryErr != nil {
  return nil, retryErr
  }
- epsAssociatedToUser := buildEPsIdsList(oncalls)
- return epsAssociatedToUser, nil
+
+ // filter all team escalation policies to only those for which the specified
+ // user is a target.
+ userEPs := []*pagerduty.EscalationPolicy{}
+ for _, ep := range eps {
+ for _, rule := range ep.EscalationRules {
+ for _, target := range rule.Targets {
+ if target.ID == userID {
+ userEPs = append(userEPs, ep)
+ }
+ }
+ }
+ }
+
+ epsAssociatedToTeamAndUser := buildEPsIdsList(userEPs)
+ return epsAssociatedToTeamAndUser, nil
 }
 
 func dissociateEPsFromTeam(c *pagerduty.Client, teamID string, eps []string) ([]string, error) {

pagerduty/resource_pagerduty_team_membership_test.go

@@ -107,6 +107,35 @@ func TestAccPagerDutyTeamMembership_DestroyWithEscalationPolicyDependant(t *test
  })
 }
 
+func TestAccPagerDutyTeamMembership_DestroyWithEscalationPolicyDependantAndMultipleTeams(t *testing.T) {
+ userOne := fmt.Sprintf("tf-%s", acctest.RandString(5))
+ teamOne := fmt.Sprintf("tf-%s", acctest.RandString(5))
+ teamTwo := fmt.Sprintf("tf-%s", acctest.RandString(5))
+ role := "manager"
+ escalationPolicyOne := fmt.Sprintf("tf-%s", acctest.RandString(5))
+ escalationPolicyTwo := fmt.Sprintf("tf-%s", acctest.RandString(5))
+
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckPagerDutyTeamMembershipDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccCheckPagerDutyTeamMembershipDestroyWithEscalationPolicyDependantAndMultipleTeams(userOne, teamOne, teamTwo, role, escalationPolicyOne, escalationPolicyTwo),
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckPagerDutyTeamMembershipExists("pagerduty_team_membership.one"),
+ ),
+ },
+ {
+ Config: testAccCheckPagerDutyTeamMembershipDestroyWithEscalationPolicyDependantAndMultipleTeamsUpdated(userOne, teamOne, teamTwo, role, escalationPolicyOne, escalationPolicyTwo),
+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckPagerDutyTeamMembershipNoExists("pagerduty_team_membership.one"),
+ ),
+ },
+ },
+ })
+}
+
 func testAccCheckPagerDutyTeamMembershipDestroy(s *terraform.State) error {
  client, _ := testAccProvider.Meta().(*Config).Client()
  for _, r := range s.RootModule().Resources {
@@ -297,3 +326,115 @@ resource "pagerduty_escalation_policy" "foo" {
 }
 `, user, team, role, escalationPolicy)
 }
+
+func testAccCheckPagerDutyTeamMembershipDestroyWithEscalationPolicyDependantAndMultipleTeams(user, teamOne, teamTwo, role, escalationPolicyOne, escalationPolicyTwo string) string {
+ return fmt.Sprintf(`
+resource "pagerduty_user" "one" {
+ name = "%[1]v"
+ email = "%[1]v@foo.test"
+}
+
+resource "pagerduty_team" "one" {
+ name = "%[2]v"
+ description = "team_one"
+}
+
+resource "pagerduty_team" "two" {
+ name = "%[3]v"
+ description = "team_two"
+}
+
+resource "pagerduty_team_membership" "one" {
+ user_id = pagerduty_user.one.id
+ team_id = pagerduty_team.one.id
+ role = "%[4]v"
+}
+
+resource "pagerduty_team_membership" "two" {
+ user_id = pagerduty_user.one.id
+ team_id = pagerduty_team.two.id
+ role = "%[4]v"
+}
+
+resource "pagerduty_escalation_policy" "one" {
+ name = "%s"
+ num_loops = 2
+ teams = [pagerduty_team.one.id]
+
+ rule {
+ escalation_delay_in_minutes = 10
+ target {
+ type = "user_reference"
+ id = pagerduty_user.one.id
+ }
+ }
+}
+
+resource "pagerduty_escalation_policy" "two" {
+ name = "%s"
+ num_loops = 2
+ teams = [pagerduty_team.two.id]
+
+ rule {
+ escalation_delay_in_minutes = 10
+ target {
+ type = "user_reference"
+ id = pagerduty_user.one.id
+ }
+ }
+}
+`, user, teamOne, teamTwo, role, escalationPolicyOne, escalationPolicyTwo)
+}
+
+func testAccCheckPagerDutyTeamMembershipDestroyWithEscalationPolicyDependantAndMultipleTeamsUpdated(user, teamOne, teamTwo, role, escalationPolicyOne, escalationPolicyTwo string) string {
+ return fmt.Sprintf(`
+resource "pagerduty_user" "one" {
+ name = "%[1]v"
+ email = "%[1]v@foo.test"
+}
+
+resource "pagerduty_team" "one" {
+ name = "%[2]v"
+ description = "team_one"
+}
+
+resource "pagerduty_team" "two" {
+ name = "%[3]v"
+ description = "team_two"
+}
+
+resource "pagerduty_team_membership" "two" {
+ user_id = pagerduty_user.one.id
+ team_id = pagerduty_team.two.id
+ role = "%[4]v"
+}
+
+resource "pagerduty_escalation_policy" "one" {
+ name = "%s"
+ num_loops = 2
+ teams = [pagerduty_team.one.id]
+
+ rule {
+ escalation_delay_in_minutes = 10
+ target {
+ type = "user_reference"
+ id = pagerduty_user.one.id
+ }
+ }
+}
+
+resource "pagerduty_escalation_policy" "two" {
+ name = "%s"
+ num_loops = 2
+ teams = [pagerduty_team.two.id]
+
+ rule {
+ escalation_delay_in_minutes = 10
+ target {
+ type = "user_reference"
+ id = pagerduty_user.one.id
+ }
+ }
+}
+`, user, teamOne, teamTwo, role, escalationPolicyOne, escalationPolicyTwo)
+}