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

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

Merged
merged 14 commits into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ module github.com/terraform-providers/terraform-provider-pagerduty
require (
github.com/hashicorp/go-hclog v0.7.0 // indirect
github.com/hashicorp/terraform v0.12.0
github.com/heimweh/go-pagerduty v0.0.0-20190807171021-2a6540956dc5
github.com/heimweh/go-pagerduty v0.0.0-20190903123207-5a16d0d0290f
)
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ github.com/heimweh/go-pagerduty v0.0.0-20190625093811-4038e4c7ddb6 h1:K1vFV+w33Q
github.com/heimweh/go-pagerduty v0.0.0-20190625093811-4038e4c7ddb6/go.mod h1:6+bccpjQ/PM8uQY9m8avM4MJea+3vo3ta9r8kGQ4XFY=
github.com/heimweh/go-pagerduty v0.0.0-20190807171021-2a6540956dc5 h1:UZQ03lpxS/AUrMTlh1yQ/MJEJ+2he5bCItZ4W9Lgs1c=
github.com/heimweh/go-pagerduty v0.0.0-20190807171021-2a6540956dc5/go.mod h1:6+bccpjQ/PM8uQY9m8avM4MJea+3vo3ta9r8kGQ4XFY=
github.com/heimweh/go-pagerduty v0.0.0-20190903123207-5a16d0d0290f h1:KxRmqNpunRKzTdjqdM5IG5v38NGWCpQjZ/vGULImZjQ=
github.com/heimweh/go-pagerduty v0.0.0-20190903123207-5a16d0d0290f/go.mod h1:6+bccpjQ/PM8uQY9m8avM4MJea+3vo3ta9r8kGQ4XFY=
github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
github.com/jellevandenhooff/dkim v0.0.0-20150330215556-f50fe3d243e1/go.mod h1:E0B/fFc00Y+Rasa88328GlI/XbtyysCtTHZS8h7IrBU=
github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI=
Expand Down
3 changes: 2 additions & 1 deletion pagerduty/import_pagerduty_team_membership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckPagerDutyTeamMembershipDestroy,
Steps: []resource.TestStep{
{
Config: testAccCheckPagerDutyTeamMembershipConfig(user, team),
Config: testAccCheckPagerDutyTeamMembershipConfig(user, team, role),
},

{
Expand Down
27 changes: 24 additions & 3 deletions pagerduty/resource_pagerduty_team_membership.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ func resourcePagerDutyTeamMembership() *schema.Resource {
Required: true,
ForceNew: true,
},
"role": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Computed: true,
ForceNew: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"
    }

},
},
}
}
Expand All @@ -38,11 +44,12 @@ func resourcePagerDutyTeamMembershipCreate(d *schema.ResourceData, meta interfac

userID := d.Get("user_id").(string)
teamID := d.Get("team_id").(string)
role := d.Get("role").(string)

log.Printf("[DEBUG] Adding user: %s to team: %s", userID, teamID)
log.Printf("[DEBUG] Adding user: %s to team: %s with role: %s", userID, teamID, role)

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if isErrCode(err, 500) {
return resource.RetryableError(err)
}
Expand Down Expand Up @@ -73,7 +80,21 @@ func resourcePagerDutyTeamMembershipRead(d *schema.ResourceData, meta interface{
return err
}

if !isTeamMember(user, teamID) {
d.Set("role", "")

if isTeamMember(user, teamID) {
resp, _, err := client.Teams.GetMembers(teamID, &pagerduty.GetMembersOptions{})
Copy link
Contributor

@pdecat pdecat Sep 18, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

How about this?

if err != nil {
return err
}

for _, member := range resp.Members {
if member.User.ID == userID {
d.Set("role", member.Role)
break
}
}
} else {
log.Printf("[WARN] Removing %s since the user: %s is not a member of: %s", d.Id(), userID, teamID)
d.SetId("")
}
Expand Down
22 changes: 19 additions & 3 deletions pagerduty/resource_pagerduty_team_membership_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckPagerDutyTeamMembershipDestroy,
Steps: []resource.TestStep{
{
Config: testAccCheckPagerDutyTeamMembershipConfig(user, team),
Config: testAccCheckPagerDutyTeamMembershipConfig(user, team, role),
Check: resource.ComposeTestCheckFunc(
testAccCheckPagerDutyTeamMembershipExists("pagerduty_team_membership.foo"),
),
Expand Down Expand Up @@ -62,6 +63,7 @@ func testAccCheckPagerDutyTeamMembershipExists(n string) resource.TestCheckFunc

userID := rs.Primary.Attributes["user_id"]
teamID := rs.Primary.Attributes["team_id"]
role := rs.Primary.Attributes["role"]

user, _, err := client.Users.Get(userID, &pagerduty.GetUserOptions{})
if err != nil {
Expand All @@ -72,11 +74,24 @@ func testAccCheckPagerDutyTeamMembershipExists(n string) resource.TestCheckFunc
return fmt.Errorf("%s is not a member of: %s", userID, teamID)
}

resp, _, err := client.Teams.GetMembers(teamID, &pagerduty.GetMembersOptions{})
if err != nil {
return err
}

for _, member := range resp.Members {
if member.User.ID == userID {
if member.Role != role {
return fmt.Errorf("%s does not have the role: %s in: %s", userID, role, teamID)
}
}
}

return nil
}
}

func testAccCheckPagerDutyTeamMembershipConfig(user, team string) string {
func testAccCheckPagerDutyTeamMembershipConfig(user, team, role string) string {
return fmt.Sprintf(`
resource "pagerduty_user" "foo" {
name = "%[1]v"
Expand All @@ -91,6 +106,7 @@ resource "pagerduty_team" "foo" {
resource "pagerduty_team_membership" "foo" {
user_id = "${pagerduty_user.foo.id}"
team_id = "${pagerduty_team.foo.id}"
role = "%[3]v"
}
`, user, team)
`, user, team, role)
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions vendor/github.com/heimweh/go-pagerduty/pagerduty/team.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 17 additions & 17 deletions vendor/modules.txt
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,13 @@ github.com/hashicorp/hcl/json/token
github.com/hashicorp/hcl2/hcl
github.com/hashicorp/hcl2/hcl/hclsyntax
github.com/hashicorp/hcl2/hcldec
github.com/hashicorp/hcl2/gohcl
github.com/hashicorp/hcl2/hcled
github.com/hashicorp/hcl2/hclparse
github.com/hashicorp/hcl2/gohcl
github.com/hashicorp/hcl2/ext/typeexpr
github.com/hashicorp/hcl2/ext/dynblock
github.com/hashicorp/hcl2/hclwrite
github.com/hashicorp/hcl2/hcl/json
github.com/hashicorp/hcl2/hcled
github.com/hashicorp/hcl2/hclwrite
# github.com/hashicorp/hil v0.0.0-20190212112733-ab17b08d6590
github.com/hashicorp/hil
github.com/hashicorp/hil/ast
Expand All @@ -126,6 +126,7 @@ github.com/hashicorp/logutils
# github.com/hashicorp/terraform v0.12.0
github.com/hashicorp/terraform/plugin
github.com/hashicorp/terraform/helper/logging
github.com/hashicorp/terraform/helper/resource
github.com/hashicorp/terraform/helper/schema
github.com/hashicorp/terraform/helper/structure
github.com/hashicorp/terraform/helper/validation
Expand All @@ -138,45 +139,44 @@ github.com/hashicorp/terraform/plugin/discovery
github.com/hashicorp/terraform/providers
github.com/hashicorp/terraform/provisioners
github.com/hashicorp/terraform/version
github.com/hashicorp/terraform/addrs
github.com/hashicorp/terraform/command/format
github.com/hashicorp/terraform/config
github.com/hashicorp/terraform/config/hcl2shim
github.com/hashicorp/terraform/helper/hashcode
github.com/hashicorp/terraform/configs
github.com/hashicorp/terraform/configs/configload
github.com/hashicorp/terraform/helper/config
github.com/hashicorp/terraform/internal/initwd
github.com/hashicorp/terraform/plans
github.com/hashicorp/terraform/states
github.com/hashicorp/terraform/tfdiags
github.com/hashicorp/terraform/addrs
github.com/hashicorp/terraform/helper/hashcode
github.com/hashicorp/terraform/config/module
github.com/hashicorp/terraform/configs
github.com/hashicorp/terraform/dag
github.com/hashicorp/terraform/flatmap
github.com/hashicorp/terraform/helper/didyoumean
github.com/hashicorp/terraform/httpclient
github.com/hashicorp/terraform/lang
github.com/hashicorp/terraform/moduledeps
github.com/hashicorp/terraform/plans
github.com/hashicorp/terraform/plans/objchange
github.com/hashicorp/terraform/states
github.com/hashicorp/terraform/states/statefile
github.com/hashicorp/terraform/helper/acctest
github.com/hashicorp/terraform/helper/resource
github.com/hashicorp/terraform/registry
github.com/hashicorp/terraform/registry/regsrc
github.com/hashicorp/terraform/registry/response
github.com/hashicorp/terraform/svchost/disco
github.com/hashicorp/terraform/helper/hilmapstructure
github.com/hashicorp/terraform/internal/modsdir
github.com/hashicorp/terraform/internal/earlyconfig
github.com/hashicorp/terraform/lang/blocktoattr
github.com/hashicorp/terraform/lang/funcs
github.com/hashicorp/terraform/command/format
github.com/hashicorp/terraform/configs/configload
github.com/hashicorp/terraform/helper/config
github.com/hashicorp/terraform/internal/initwd
github.com/hashicorp/terraform/svchost
github.com/hashicorp/terraform/svchost/auth
github.com/hashicorp/terraform/internal/modsdir
github.com/hashicorp/terraform/internal/earlyconfig
# github.com/hashicorp/terraform-config-inspect v0.0.0-20190327195015-8022a2663a70
github.com/hashicorp/terraform-config-inspect/tfconfig
# github.com/hashicorp/yamux v0.0.0-20180604194846-3520598351bb
github.com/hashicorp/yamux
# github.com/heimweh/go-pagerduty v0.0.0-20190625093811-4038e4c7ddb6
# github.com/heimweh/go-pagerduty v0.0.0-20190903123207-5a16d0d0290f
github.com/heimweh/go-pagerduty/pagerduty
# github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af
github.com/jmespath/go-jmespath
Expand Down Expand Up @@ -314,6 +314,7 @@ google.golang.org/genproto/googleapis/rpc/code
google.golang.org/genproto/googleapis/api/annotations
# google.golang.org/grpc v1.18.0
google.golang.org/grpc
google.golang.org/grpc/test/bufconn
google.golang.org/grpc/credentials
google.golang.org/grpc/health
google.golang.org/grpc/health/grpc_health_v1
Expand Down Expand Up @@ -342,7 +343,6 @@ google.golang.org/grpc/resolver/passthrough
google.golang.org/grpc/stats
google.golang.org/grpc/status
google.golang.org/grpc/tap
google.golang.org/grpc/test/bufconn
google.golang.org/grpc/credentials/internal
google.golang.org/grpc/balancer/base
google.golang.org/grpc/binarylog/grpc_binarylog_v1
Expand Down
3 changes: 3 additions & 0 deletions website/docs/r/team_membership.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ resource "pagerduty_team" "foo" {
resource "pagerduty_team_membership" "foo" {
user_id = "${pagerduty_user.foo.id}"
team_id = "${pagerduty_team.foo.id}"
role = "manager"
}
```

Expand All @@ -35,13 +36,15 @@ The following arguments are supported:

* `user_id` - (Required) The ID of the user to add to the team.
* `team_id` - (Required) The ID of the team in which the user will belong.
* `role` - (Optional) The role of the user in the team

## Attributes Reference

The following attributes are exported:

* `user_id` - The ID of the user belonging to the team.
* `team_id` - The team ID the user belongs to.
* `role` - The role of the user in the team.


## Import
Expand Down