Skip to content
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

fix: improve team_membership deletion logic #918

Merged
merged 1 commit into from
Aug 9, 2024
Merged

fix: improve team_membership deletion logic #918

merged 1 commit into from
Aug 9, 2024

Conversation

mdb
Copy link
Contributor

@mdb mdb commented Jul 28, 2024

This seeks to fix issue #913 by only diassociating/re-associating the team-specific escalation policies for which the
user-whose-membership-is-to-be-deleted is a rule target (rather than attempting to diassociate/re-associate all the user-associated escalation policies to the team, as those escalation policies may be taken by other teams).

In other words...

Previously, team membership deletion attempted to diassociate all escalation policies associated to the user from the team, then re-associate those policies back to the team. This could run into the following error if/when that user was a member of multiple teams (see issue #913 for details) and associated to escalation policies associated to other teams:

pagerduty_team_membership.redacted: Still destroying...
[id=REDACTED:REDACTED, 2m0s elapsed]

  | │ Error: PUT API call to
https://api.pagerduty.com/teams/team1/escalation_policies/escalation_policy2
failed 400 Bad Request. Code: 2001, Errors: <nil>, Message: Escalation
Policy has already been taken; Error while trying to associate back team
"team1" to Escalation Policy "escalation_policy2". Resource succesfully
deleted, but some team association couldn't be completed, so you need to
run "terraform plan -refresh-only" and again "terraform apply/destroy"
in order to remediate the drift

This seeks to fix that :)

Result of new acceptance tests introduced...

TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -count=1 -run TestAccPagerDutyTeamMembership -timeout 120m
?       github.com/PagerDuty/terraform-provider-pagerduty       [no test files]
?       github.com/PagerDuty/terraform-provider-pagerduty/util/apiutil  [no test files]

=== RUN   TestAccPagerDutyTeamMembership_import
--- PASS: TestAccPagerDutyTeamMembership_import (13.99s)
=== RUN   TestAccPagerDutyTeamMembership_importWithRole
--- PASS: TestAccPagerDutyTeamMembership_importWithRole (14.20s)
=== RUN   TestAccPagerDutyTeamMembership_Basic
--- PASS: TestAccPagerDutyTeamMembership_Basic (13.61s)
=== RUN   TestAccPagerDutyTeamMembership_WithRole
--- PASS: TestAccPagerDutyTeamMembership_WithRole (13.05s)
=== RUN   TestAccPagerDutyTeamMembership_WithRoleConsistentlyAssigned
--- PASS: TestAccPagerDutyTeamMembership_WithRoleConsistentlyAssigned (21.90s)
=== RUN   TestAccPagerDutyTeamMembership_DestroyWithEscalationPolicyDependant
--- PASS: TestAccPagerDutyTeamMembership_DestroyWithEscalationPolicyDependant (21.98s)
=== RUN   TestAccPagerDutyTeamMembership_DestroyWithEscalationPolicyDependantAndMultipleTeams # 👈 New acceptance test 
--- PASS: TestAccPagerDutyTeamMembership_DestroyWithEscalationPolicyDependantAndMultipleTeams (24.00s)
PASS
ok      github.com/PagerDuty/terraform-provider-pagerduty/pagerduty     123.264s
testing: warning: no tests to run
PASS
ok      github.com/PagerDuty/terraform-provider-pagerduty/pagerdutyplugin       1.128s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/PagerDuty/terraform-provider-pagerduty/util  0.711s [no tests to run]

This seeks to fix issue #913 by only diassociating/re-associating the
_team_-specific escalation policies for which the
user-whose-membership-is-to-be-deleted is a rule target (rather than
attempting to diassociate/re-associate _all_ the user-associated
escalation policies to the team, as those escalation policies may be
taken by other teams).

In other words...

Previously, team membership deletion attempted to diassociate _all_ escalation
policies associated to the user from the team, then re-associate those policies
back to the team. This could run into the following error if/when that
user was a member of multiple teams (see issue #913 for details) and
associated to escalation policies associated to _other_ teams:

```
pagerduty_team_membership.redacted: Still destroying...
[id=REDACTED:REDACTED, 2m0s elapsed]

  | │ Error: PUT API call to
https://api.pagerduty.com/teams/team1/escalation_policies/escalation_policy2
failed 400 Bad Request. Code: 2001, Errors: <nil>, Message: Escalation
Policy has already been taken; Error while trying to associate back team
"team1" to Escalation Policy "escalation_policy2". Resource succesfully
deleted, but some team association couldn't be completed, so you need to
run "terraform plan -refresh-only" and again "terraform apply/destroy"
in order to remediate the drift
```

This seeks to fix that :)
@mdb
Copy link
Contributor Author

mdb commented Jul 28, 2024

Full transparency: I don't have a reliable way to run the acceptance tests nor am I intimately familiar with the quirks of the PagerDuty API. So, there could be bugs or mistaken assumptions associated with this logic change, but -- at face value -- I believe this is an improvement over the original logic. Am I mistaken?

@Surmi64
Copy link

Surmi64 commented Aug 7, 2024

Are there any chance that this will be merged in the near future? We ran into this problem in the past weeks and it causes a lot of trouble when we would modify any detail of a team with EP.

@imjaroiswebdev imjaroiswebdev self-requested a review August 9, 2024 19:16
Copy link
Contributor

@imjaroiswebdev imjaroiswebdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for taking the time to fix this @mdb, it's a very welcome contribution 💪🏽 🥇

@imjaroiswebdev imjaroiswebdev merged commit 9a364cc into PagerDuty:master Aug 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants