-
Notifications
You must be signed in to change notification settings - Fork 154
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
add datasource tfe_organization_members #635
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all, phenomenal work 🚀 ! I've requested some minor changes, but we can discuss them if you have other thoughts! Excited for this change 😁
Smoke tested ✅
tfe/organization_members_helpers.go
Outdated
if orgMembership.Status == tfe.OrganizationMembershipActive { | ||
member := map[string]string{"user_id": orgMembership.User.ID, "membership_id": orgMembership.ID} | ||
members = append(members, member) | ||
} else if orgMembership.Status == tfe.OrganizationMembershipInvited { | ||
member := map[string]string{"user_id": orgMembership.User.ID, "membership_id": orgMembership.ID} | ||
membersWaiting = append(membersWaiting, member) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love that you sort this distinction in membership status 👍
tfe/organization_members_helpers.go
Outdated
tfe "github.com/hashicorp/go-tfe" | ||
) | ||
|
||
func fetchOrganizationMembers(client *tfe.Client, orgName string, options *tfe.OrganizationMembershipListOptions) ([]map[string]string, []map[string]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think options
should be defined locally here rather than as a parameter. Suppose the caller of fetchOrganizationMembers()
mutates options
... could lead to some wonky behavior here. Alternatively, if you do want the caller to be able to define options
, it should be passed by value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, to make it flexible for callers of this function to pass in respective options, I will update the option to pass by value.
tfe/testing.go
Outdated
return orgMembership, func() { | ||
if err := client.OrganizationMemberships.Delete(ctx, orgMembership.ID); err != nil { | ||
t.Errorf("Error destroying organization membership! WARNING: Dangling resources\n"+ | ||
"may exist! The full error is show below:\n\n"+ | ||
"Organization memberships:%s\nError: %s", orgMembership.ID, err) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if we can get away with registering the cleanup function inside the helper rather than returning it:
t.Cleanup(func() {
// delete orgMembership
})
return orgMembership
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this should work as well
data "tfe_organization_members" "foo" { | ||
organization = "organization-name" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 Typically in our examples we also include defining the resources our documented resource would depend on:
resource "tfe_organization" "deathstar" {
name = "deathstar"
email = "sheev.palpatine@hashicorp.com"
}
data "tfe_organization_members" "stormtroopers" {
organization = tfe_organization.deathstar.name
}
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
I would like this to be released in conjunction with #617 💜 |
The `member` block contains: | ||
|
||
* `user_id` - The ID of the user. | ||
* `membership_id` - The ID of the membership. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere in the provider, we call this the organization_membership_id
(tfe_team_organization_member
resource) and colloquially "organization membership id" in other places (tfe_organization_membership
resource and data source)
I think we should reuse this long name so we don't risk id confusion. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sounds reasonable to me, I will update the name to match.
53f1439
to
9753735
Compare
9753735
to
50198d8
Compare
tfeClient := meta.(*tfe.Client) | ||
|
||
organizationName := d.Get("organization").(string) | ||
options := tfe.OrganizationMembershipListOptions{Include: []tfe.OrgMembershipIncludeOpt{tfe.OrgMembershipUser}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we require only the user ID, there is no need to include users because the normal result only returns the ID. This should make fetching them faster since no extra user data has to be transmitted unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I didn't realize user ID comes by default. 🎉
tfe/organization_members_helpers.go
Outdated
tfe "github.com/hashicorp/go-tfe" | ||
) | ||
|
||
func fetchOrganizationMembers(client *tfe.Client, orgName string, options tfe.OrganizationMembershipListOptions) ([]map[string]string, []map[string]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm feeling like this function shouldn't accept an arbitrary ListOptions parameter. Reason being, you always want to start at page 1 and return all pages, plus the shape of the return value determines which include params you need in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion about this one so I'll update it
24470b1
to
ce419d4
Compare
Description
Add datasource
tfe_organization_members
which returns a list of active members and members with pending invite in an organization.Testing plan
External links
Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.
Output from acceptance tests
Please run applicable acceptance tests locally and include the output here. See TESTS.md to learn how to run acceptance tests.
If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.