resource/pagerduty_team_membership: Support team member's role #151

dtan4 · 2019-09-04T12:37:52Z

WHAT

Add role field to pagerduty_team_membership resource to specify team member's role

resource "pagerduty_user" "foo" {
  name  = "foo"
  email = "foo@bar.com"
}

resource "pagerduty_team" "foo" {
  name        = "foo"
  description = "foo"
}

resource "pagerduty_team_membership" "foo" {
  user_id = "${pagerduty_user.foo.id}"
  team_id = "${pagerduty_team.foo.id}"
  role    = "manager"  # <<<<<<<<<<<<<
}

WHY

A few months ago, we can add users to team with "manager" role by this resource.

resource "pagerduty_team_membership" "foo" {
  user_id = "${pagerduty_user.foo.id}"
  team_id = "${pagerduty_team.foo.id}"
}

However, a new member is assigned with "observer" role recently. This behavior is unexpected for us. We'd like to give each team members permission to manage their on-call schedule by themselves, but "observer" role doesn't allow it.

PagerDuty API has a field to specify member role when we create a new membership.
https://api-reference.pagerduty.com/#!/Teams/put_teams_id_users_user_id
We'd like to specify the role at the creation time.

UPDATE 2020-03-09:
Now this API assigns manager role by default, but I think this change can still be helpful to manage memberships in detail.

See https://github.com/terraform-providers/terraform-provider-pagerduty/pull/151#issuecomment-596463829 for details.

$ GO111MODULE=on go get -u github.com/heimweh/go-pagerduty/pagerduty $ GO111MODULE=on go mod vendor

dtan4 · 2019-09-18T05:52:23Z

Could anybody take a look...?

pdecat

Hi @dtan4, great addition!

As requested, I reviewed this and left a few comments.
Feel free to do what you like with them, I'm just a contributor here.

pdecat · 2019-09-18T07:26:26Z

pagerduty/resource_pagerduty_team_membership.go

+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,
+				ForceNew: true,


This is not explicitely documented, but it is not required to recreate the team membership resource when only the role changes.
That is, invoking PUT https://api.pagerduty.com/teams/***/users/*** with AddUserWithRole on an already existing team member works:

# curl -s -X GET --header 'Accept: application/vnd.pagerduty+json;version=2' --header 'Authorization: Token token=******' 'https://api.pagerduty.com/teams/P4*****/members' | jq '.members[] | select(.user.id=="P2*****")' { "user": { "id": "P2*****", "type": "user_reference", "summary": "User Name", "self": "https://api.pagerduty.com/users/P2*****", "html_url": "https://claranet.pagerduty.com/users/P2*****" }, "role": "manager" }

# curl -i -X PUT --header 'Content-Type: application/json' --header 'Accept: application/vnd.pagerduty+json;version=2' --header 'Authorization: Token token=******' -d '{ "role": "observer" }' 'https://api.pagerduty.com/teams/P4*****/users/P2*****' |& grep HTTP HTTP/2 204

# curl -s -X GET --header 'Accept: application/vnd.pagerduty+json;version=2' --header 'Authorization: Token token=******' 'https://api.pagerduty.com/teams/P4*****/members' | jq '.members[] | select(.user.id=="P2*****")' { "user": { "id": "P2*****", "type": "user_reference", "summary": "User Name", "self": "https://api.pagerduty.com/users/P2*****", "html_url": "https://claranet.pagerduty.com/users/P2*****" }, "role": "observer" }

Not recreating the team membership avoids the user loosing the role even for a very short period.
That will require introducing a resourcePagerDutyTeamMembershipUpdate function.

Thanks. I added resourcePagerDutyTeamMembershipUpdate with the same logic of resourcePagerDutyTeamMembershipCreate. (hashicorp@3a0aaca) It worked fine.

# pagerduty_team_membership.dtan4 will be updated in-place ~ resource "pagerduty_team_membership" "dtan4" { id = "PPPPPPP:PQQQQQQ" ~ role = "manager" -> "observer" team_id = "PPPPPPP" user_id = "PQQQQQQ" }

pdecat · 2019-09-18T07:37:56Z

pagerduty/resource_pagerduty_team_membership.go

@@ -30,6 +30,12 @@ func resourcePagerDutyTeamMembership() *schema.Resource {
 				Required: true,
 				ForceNew: true,
 			},
+			"role": {
+				Type:     schema.TypeString,
+				Optional: true,


I remember there are issues when Optional and Computed are used together, not sure if it applies here.
This probably needs more attention.

hashicorp/terraform-provider-google#3477 (comment)
hashicorp/terraform#20505 (comment)
hashicorp/terraform-provider-google#4326

Changed to Optional only, as developers can set value by themselves or use the default value.
hashicorp@003ba6c

pdecat · 2019-09-18T07:52:26Z

pagerduty/import_pagerduty_team_membership_test.go

@@ -11,14 +11,15 @@ import (
 func TestAccPagerDutyTeamMembership_import(t *testing.T) {
 	user := fmt.Sprintf("tf-%s", acctest.RandString(5))
 	team := fmt.Sprintf("tf-%s", acctest.RandString(5))
+	role := "manager"


I think the original test should probably be left untouched to ensure no regression is introduced.

Maybe a new TestAccPagerDutyTeamMembership_importWithRole test should be introduced.

Thanks. Created a new testcase for withRole.
hashicorp@6ce1d0c

pdecat · 2019-09-18T07:52:46Z

pagerduty/resource_pagerduty_team_membership_test.go

@@ -13,14 +13,15 @@ import (
 func TestAccPagerDutyTeamMembership_Basic(t *testing.T) {
 	user := fmt.Sprintf("tf-%s", acctest.RandString(5))
 	team := fmt.Sprintf("tf-%s", acctest.RandString(5))
+	role := "manager"


I think the original test should probably be left untouched to ensure no regression is introduced.

Maybe a new TestAccPagerDutyTeamMembership_WithRole test should be introduced.

Thanks. Created a new testcase for withRole.
hashicorp@89b0e5f

pdecat · 2019-09-18T08:03:15Z

pagerduty/resource_pagerduty_team_membership.go

@@ -30,6 +30,12 @@ func resourcePagerDutyTeamMembership() *schema.Resource {
 				Required: true,
 				ForceNew: true,
 			},
+			"role": {


Validation could probably be added:

The role of the user on the team.
Can be observer, responder or manager

https://api-reference.pagerduty.com/#!/Teams/put_teams_id_users_user_id

Added! hashicorp@8521226

pdecat · 2019-09-18T08:05:01Z

pagerduty/resource_pagerduty_team_membership.go


 	retryErr := resource.Retry(2*time.Minute, func() *resource.RetryError {
-		if _, err := client.Teams.AddUser(teamID, userID); err != nil {
+		if _, err := client.Teams.AddUserWithRole(teamID, userID, role); err != nil {


What if the role is empty? Shouldn't client.Teams.AddUser(teamID, userID) be used instead?

I defined a default value for role to be the same as the current API behavior, calling AddUser API without role.
hashicorp@a560e09

With validation implemented at hashicorp@8521226, this resource guarantees a valid value is set in role at every time. So I think it's okay to use AddUserWithRole at every time too.

pdecat · 2019-09-18T08:31:40Z

pagerduty/resource_pagerduty_team_membership.go

+	d.Set("role", "")
+
+	if isTeamMember(user, teamID) {
+		resp, _, err := client.Teams.GetMembers(teamID, &pagerduty.GetMembersOptions{})


Shouldn't client.Teams.GetMembers become the main API to query for resourcePagerDutyTeamMembershipRead instead of calling client.Users.Get?

This could probably simplify things a bit, isn't it?

Instead of using client.Users.Get, I could simplify the logic with client.Teams.GetMembers only.
hashicorp@1f71c0c

How about this?

dtan4 · 2019-09-24T12:56:10Z

@pdecat Thank you for reviewing! I added some more changes you pointed out. Could you take another look?

pdecat

LGTM!

PS: I cannot run the acceptance tests as my test Pagerduty account does not have the Teams feature.

dtan4 · 2019-11-08T07:30:31Z

Is there any chance to merge this...? 👀

cilindrox · 2019-11-14T19:42:08Z

Subbed - this just hit us and we had to fall back to the legacy team_responder roles

cilindrox · 2020-01-02T21:45:51Z

Any updates on this? Is there any way I can help out?

vanvlack · 2020-02-18T20:44:31Z

@stmcallister any chance on getting this reviewed/merged for the next release?

stmcallister · 2020-02-19T21:59:20Z

Sorry for the delay! What are you getting for your test results? I'm getting the following failures.

=== RUN   TestAccPagerDutyTeamMembership_import
--- FAIL: TestAccPagerDutyTeamMembership_import (5.24s)
    testing.go:569: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: pagerduty_team_membership.foo
          id:      "PA03194:PHWZQAX" => "PA03194:PHWZQAX"
          role:    "manager" => "observer"
          team_id: "PHWZQAX" => "PHWZQAX"
          user_id: "PA03194" => "PA03194"
        
        
        
        STATE:
        
        pagerduty_team.foo:
          ID = PHWZQAX
          provider = provider.pagerduty
          description = foo
          html_url = https://pdt-smcallister.pagerduty.com/teams/PHWZQAX
          name = tf-xyvc9
        pagerduty_team_membership.foo:
          ID = PA03194:PHWZQAX
          provider = provider.pagerduty
          role = manager
          team_id = PHWZQAX
          user_id = PA03194
        
          Dependencies:
            pagerduty_team.foo
            pagerduty_user.foo
        pagerduty_user.foo:
          ID = PA03194
          provider = provider.pagerduty
          avatar_url = https://secure.gravatar.com/avatar/fedbea5960c2dcf789279bf3ac8a6a5a.png?d=mm&r=PG
          color = dark-orchid
          description = Managed by Terraform
          email = tf-nccl6@foo.com
          html_url = https://pdt-smcallister.pagerduty.com/users/PA03194
          invitation_sent = true
          job_title = 
          name = tf-nccl6
          role = user
          time_zone = America/Los_Angeles


=== RUN   TestAccPagerDutyTeamMembership_Basic
--- FAIL: TestAccPagerDutyTeamMembership_Basic (5.75s)
    testing.go:569: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: pagerduty_team_membership.foo
          id:      "PV0EBSA:PT2RNH4" => "PV0EBSA:PT2RNH4"
          role:    "manager" => "observer"
          team_id: "PT2RNH4" => "PT2RNH4"
          user_id: "PV0EBSA" => "PV0EBSA"
        
        
        
        STATE:
        
        pagerduty_team.foo:
          ID = PT2RNH4
          provider = provider.pagerduty
          description = foo
          html_url = https://pdt-smcallister.pagerduty.com/teams/PT2RNH4
          name = tf-3lbig
        pagerduty_team_membership.foo:
          ID = PV0EBSA:PT2RNH4
          provider = provider.pagerduty
          role = manager
          team_id = PT2RNH4
          user_id = PV0EBSA
        
          Dependencies:
            pagerduty_team.foo
            pagerduty_user.foo
        pagerduty_user.foo:
          ID = PV0EBSA
          provider = provider.pagerduty
          avatar_url = https://secure.gravatar.com/avatar/848fb5e400214da9ef9317b36ef59346.png?d=mm&r=PG
          color = dark-orchid
          description = Managed by Terraform
          email = tf-4nlgr@foo.com
          html_url = https://pdt-smcallister.pagerduty.com/users/PV0EBSA
          invitation_sent = true
          job_title = 
          name = tf-4nlgr
          role = user
          time_zone = America/Los_Angeles



=== RUN   TestAccPagerDutyUserContactMethodPhone_Basic
--- FAIL: TestAccPagerDutyUserContactMethodPhone_Basic (8.37s)
    testing.go:569: Step 1 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: pagerduty_user.foo
          avatar_url:      "https://secure.gravatar.com/avatar/1d02467c6b92a031c787d3ef5d81a386.png?d=mm&r=PG" => "https://secure.gravatar.com/avatar/1d02467c6b92a031c787d3ef5d81a386.png?d=mm&r=PG"
          color:           "red" => "red"
          description:     "bar" => "bar"
          email:           "tf-vxyt2@foo.com" => "tf-np4dw@foo.com"
          html_url:        "https://pdt-smcallister.pagerduty.com/users/P3WWYZ7" => "https://pdt-smcallister.pagerduty.com/users/P3WWYZ7"
          id:              "P3WWYZ7" => "P3WWYZ7"
          invitation_sent: "true" => "true"
          job_title:       "bar" => "bar"
          name:            "tf-vxyt2" => "tf-np4dw"
          role:            "user" => "user"
          teams.#:         "0" => "0"
          time_zone:       "America/Los_Angeles" => "America/Los_Angeles"
        
        
        
        STATE:
        
        pagerduty_user.foo:
          ID = P3WWYZ7
          provider = provider.pagerduty
          avatar_url = https://secure.gravatar.com/avatar/1d02467c6b92a031c787d3ef5d81a386.png?d=mm&r=PG
          color = red
          description = bar
          email = tf-vxyt2@foo.com
          html_url = https://pdt-smcallister.pagerduty.com/users/P3WWYZ7
          invitation_sent = true
          job_title = bar
          name = tf-vxyt2
          role = user
          time_zone = America/Los_Angeles
        pagerduty_user_contact_method.foo:
          ID = PJYKO2F
          provider = provider.pagerduty
          address = 8669351337
          blacklisted = false
          country_code = 1
          enabled = false
          label = tf-np4dw
          send_short_email = false
          type = phone_contact_method
          user_id = P3WWYZ7
        
          Dependencies:
            pagerduty_user.foo
            
            
            
=== RUN   TestAccPagerDutyUserWithTeams_Basic
--- FAIL: TestAccPagerDutyUserWithTeams_Basic (5.35s)
    testing.go:569: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: pagerduty_team_membership.foo
          id:      "PF76XUB:P57U8D1" => "PF76XUB:P57U8D1"
          role:    "manager" => "observer"
          team_id: "P57U8D1" => "P57U8D1"
          user_id: "PF76XUB" => "PF76XUB"
        
        
        
        STATE:
        
        pagerduty_team.foo:
          ID = P57U8D1
          provider = provider.pagerduty
          description = Managed by Terraform
          html_url = https://pdt-smcallister.pagerduty.com/teams/P57U8D1
          name = tf-yy2er
        pagerduty_team_membership.foo:
          ID = PF76XUB:P57U8D1
          provider = provider.pagerduty
          role = manager
          team_id = P57U8D1
          user_id = PF76XUB
        
          Dependencies:
            pagerduty_team.foo
            pagerduty_user.foo
        pagerduty_user.foo:
          ID = PF76XUB
          provider = provider.pagerduty
          avatar_url = https://secure.gravatar.com/avatar/592504d0dd3931101adb736084c1506b.png?d=mm&r=PG
          color = dark-orchid
          description = Managed by Terraform
          email = tf-fcib2@foo.com
          html_url = https://pdt-smcallister.pagerduty.com/users/PF76XUB
          invitation_sent = true
          job_title = 
          name = tf-fcib2
          role = user
          time_zone = America/Los_Angeles

dtan4 · 2020-03-09T11:08:56Z

@stmcallister Sorry for the late reply,

When I submitted this Pull Request, this Add a user to a team API assigns observer role by default. Due to this, I chose observer as a default membership role in this provider too.

However, the behavior changed (reverted?) recently; this API assigns manager role by default. That's why the acceptance tests failed. This behavior can be reproduced with the current stable PagerDuty provider.

I modified the default membership role to manager at hashicorp@2a72eb4 and confirmed it passes all acceptance tests.

Could you review again? Thanks!

stmcallister · 2020-03-19T01:11:09Z

@dtan4 looks great! thank you!

dtan4 added 3 commits September 4, 2019 20:15

Update go-pagerduty to the latest one

40124d8

$ GO111MODULE=on go get -u github.com/heimweh/go-pagerduty/pagerduty $ GO111MODULE=on go mod vendor

Add role field to pagerduty_team_membership

b3875bd

Add role field to pagerduty_team_membership document

5e12f83

ghost added size/M dependencies documentation labels Sep 4, 2019

pdecat reviewed Sep 18, 2019

View reviewed changes

Remain existing acctest and add new one

89b0e5f

ghost added size/L and removed size/M labels Sep 19, 2019

dtan4 added 7 commits September 19, 2019 19:25

Remain existing import acctest and add new one

6ce1d0c

Delete Computed attribute from role

003ba6c

Add validation for role field

8521226

Make role in pagerduty_team_membership modifiable

3a0aaca

Use "observer" as a default value of team membership role

a560e09

Describe supported and default value of team membership role

70201a4

Simplify the logic in reading team membership by reducing API call

1f71c0c

pdecat approved these changes Sep 25, 2019

View reviewed changes

Merge branch 'master' into add-teammember-with-role

48bb3b1

dtan4 added 2 commits March 9, 2020 19:40

Merge branch 'master' into add-teammember-with-role

a4832b0

Change default team membership role to "manager"

2a72eb4

stmcallister merged commit 8a14d87 into PagerDuty:master Mar 19, 2020

dtan4 deleted the add-teammember-with-role branch March 25, 2020 07:19

tecnobrat mentioned this pull request Apr 1, 2020

DOCS: Team membership role defaults to manager #194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/pagerduty_team_membership: Support team member's role #151

resource/pagerduty_team_membership: Support team member's role #151

dtan4 commented Sep 4, 2019 •

edited

Loading

dtan4 commented Sep 18, 2019

pdecat left a comment

pdecat Sep 18, 2019

dtan4 Sep 24, 2019

pdecat Sep 18, 2019

dtan4 Sep 24, 2019

pdecat Sep 18, 2019

dtan4 Sep 20, 2019

pdecat Sep 18, 2019

dtan4 Sep 20, 2019

pdecat Sep 18, 2019

dtan4 Sep 24, 2019

pdecat Sep 18, 2019

dtan4 Sep 24, 2019

pdecat Sep 18, 2019 •

edited

Loading

dtan4 Sep 24, 2019

dtan4 commented Sep 24, 2019 •

edited

Loading

pdecat left a comment

dtan4 commented Nov 8, 2019

cilindrox commented Nov 14, 2019

cilindrox commented Jan 2, 2020

vanvlack commented Feb 18, 2020

stmcallister commented Feb 19, 2020

dtan4 commented Mar 9, 2020

stmcallister commented Mar 19, 2020

resource/pagerduty_team_membership: Support team member's role #151

resource/pagerduty_team_membership: Support team member's role #151

Conversation

dtan4 commented Sep 4, 2019 • edited Loading

WHAT

WHY

dtan4 commented Sep 18, 2019

pdecat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdecat Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtan4 commented Sep 24, 2019 • edited Loading

pdecat left a comment

Choose a reason for hiding this comment

dtan4 commented Nov 8, 2019

cilindrox commented Nov 14, 2019

cilindrox commented Jan 2, 2020

vanvlack commented Feb 18, 2020

stmcallister commented Feb 19, 2020

dtan4 commented Mar 9, 2020

stmcallister commented Mar 19, 2020

dtan4 commented Sep 4, 2019 •

edited

Loading

pdecat Sep 18, 2019 •

edited

Loading

dtan4 commented Sep 24, 2019 •

edited

Loading