DXCDT-364: Add organization data source #475
Conversation
sergiught
force-pushed
the
feature/DXCDT-364-org-data-source
branch
3 times, most recently
from
e07db05 to
b27042e
Compare
sergiught
force-pushed
the
feature/DXCDT-364-org-data-source
branch
from
b27042e to
926ca21
Compare
| outerLoop: | ||
| for { | ||
| organizations, err := api.Organization.List(management.Page(page)) | ||
| if err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| for _, organization := range organizations.Organizations { | ||
| if organization.GetName() == name { | ||
| foundOrganization = organization | ||
| break outerLoop | ||
| } | ||
| } | ||
|
|
||
| if !organizations.HasNext() { | ||
| break | ||
| } | ||
|
|
||
| page++ | ||
| } | ||
|
|
||
| if foundOrganization == nil { | ||
| return diag.Errorf("No organization found with \"name\" = %q", name) | ||
| } |
There was a problem hiding this comment.
I think that it is ok to provide the ability to lookup by name. But given that some tenants have thousands of organizations, might we suggest to the end user to lookup by ID for most performant results? I can just see this loop taking a very long time and users not understanding why.
| foundConnections, err := fetchAllOrganizationConnections(api, foundOrganization.GetID()) | ||
| if err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| result = multierror.Append( | ||
| result, | ||
| data.Set("connections", flattenOrganizationConnections(foundConnections)), | ||
| ) | ||
|
|
||
| foundMembers, err := fetchAllOrganizationMembers(api, foundOrganization.GetID()) | ||
| if err != nil { | ||
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| result = multierror.Append( | ||
| result, | ||
| data.Set("members", flattenOrganizationMembers(foundMembers)), | ||
| ) |
There was a problem hiding this comment.
Any performance concerns about retrieving all connections and members? Connections should be a reasonable quality but it is possible for an organization to possess a large number of members.
There was a problem hiding this comment.
I believe there's a limit on the API, they only return 1000 items at most.
Add docs
sergiught
force-pushed
the
feature/DXCDT-364-org-data-source
branch
from
926ca21 to
7fc205c
Compare
| return diag.FromErr(err) | ||
| } | ||
| } else { | ||
| name := data.Get("name").(string) |
There was a problem hiding this comment.
Is it necessary to offer looking up the org by name, besides just by ID? Just curious.
There was a problem hiding this comment.
Not necessarily, we could consider removing it and allowing only lookup based on ID. But then it would be best to remove the functionality from all other data sources as well as there was a precedent set through the client data source.
There was a problem hiding this comment.
Note that this won't apply to all resources (only those that have unique names). Whereas in others it makes sense. If possible, consider having this for those resources only.
There was a problem hiding this comment.
Yep indeed, for the resource server data source I'm using the identifier instead of the name, as that is not unique for those.
There was a problem hiding this comment.
Note that if it turns out that customers actually need this functionality in other resources, it can always be added later.
There was a problem hiding this comment.
You raise a good point, I'll have to investigate the topic a bit deeper. For now I'll keep this as is in the PR however before we cut a release with all the data sources added we'll take a decision whether to only allow id's for the lookup or id's and names.
| return diag.FromErr(result.ErrorOrNil()) | ||
| } | ||
|
|
||
| func fetchAllOrganizationConnections(api *management.Management, organizationID string) ([]*management.OrganizationConnection, error) { |
There was a problem hiding this comment.
In the future we can consider using a generic helper for the paging logic, which seems to be repeated in a number of places.
There was a problem hiding this comment.
IMO a better place for this would be the Go SDK itself. It would be nice if it would provide functions like "ListAll()" to wrap that logic. Wdyt?
🔧 Changes
Adding support for organization data sources in this PR. They'll be available by quering based on
organization_idorname. The data source will also additionally include connections compared to the organization resource.📚 References
🔬 Testing
📝 Checklist