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
Replace brute search on d/tfe_team
and d/tfe_organization_membership
#491
Conversation
} | ||
|
||
return fmt.Errorf("Could not find organization membership for organization %s and email %s", organization, email) | ||
d.SetId(oml.Items[0].ID) |
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.
What if this is used against a TFE version before this filter did anything? I think the brute search needs to be a fallback?
b873583
to
995f3b0
Compare
github.com/hashicorp/go-safetemp v1.0.0 h1:2HR189eFNrjHQyENnQMMpCiBAsRxzbTMIgBhEyExpmo= | ||
github.com/hashicorp/go-safetemp v1.0.0/go.mod h1:oaerMy3BhqiTbVye6QuFhFtIceqFoDHxNAB65b+Rj1I= | ||
github.com/hashicorp/go-slug v0.8.0 h1:h7AGtXVAI/cJ/Wwa/JQQaftQnWQmZbAzkzgZeZVVmLw= | ||
github.com/hashicorp/go-slug v0.8.0/go.mod h1:Ib+IWBYfEfJGI1ZyXMGNbu2BU+aa3Dzu41RKLH301v4= | ||
github.com/hashicorp/go-slug v0.8.1 h1:srN7ivgAjHfZddYY1DjBaihRCFy20+vCcOrlx1O2AfE= |
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 we need to run go mod tidy
?
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.
Done 👍
@@ -40,28 +40,44 @@ func dataSourceTFEOrganizationMembershipRead(d *schema.ResourceData, meta interf | |||
// Create an options struct. | |||
options := &tfe.OrganizationMembershipListOptions{ | |||
Include: []tfe.OrgMembershipIncludeOpt{tfe.OrgMembershipUser}, | |||
Emails: []string{email}, |
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.
email is a required field, right? If email is an empty string for some reason, the API will still use the filter.
Try this example:
curl -g -H "Authorization: Bearer $(tfc_staging_token)" https://app.staging.terraform.io/api/v2/organizations/bcroft/organization-memberships?filter[email]=
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 and it would behave as expected, i.e letting the user know that the org user with an empty email does not exist -- either by returning zero org members or by brute searching against all org members and checking if orgMember.Email == ""
(obviously will never return a match)
case 0: | ||
return fmt.Errorf("Could not find organization membership for organization %s and email %s", organization, email) | ||
case 1: | ||
if oml.Items[0].User.Email != email { |
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.
Just one suggestion... this case (and the team case) has become odd but you and I both know it's totally reasonable. Maybe just comment about how this and the default case are to support versions of TFE that do not support the email filter option?
046c93c
to
2008ebd
Compare
Description
Previously, the List Organization Members and List Teams API did not support any way to retrieve the desired object(s) through the use of query params. Therefore, the current implementation of
d/tfe_team
andd/tfe_organization_membership
relies on brute searching through the paginated API until a match was found.This PR makes use of the new
filter[email]
(for org members) andfilter[names]
(for teams) to reduce the number of API calls to fetch the desired resource from N to 1. This resolves issue #326Testing plan
External links