Add data source & resource group_members #148
Conversation
| Required: true, | ||
| }, | ||
|
|
||
| "members": { |
There was a problem hiding this comment.
I apologize for not saying anything yesterday. I'm wondering if we would be better off making this a TypeList and writing a diffSuppressFunc to suppress any diffs on order? TypeSet can sometimes be a nuisance to maintain, and in this case, the diffSuppress might be easier. Do you have thoughts for/against this idea?
There was a problem hiding this comment.
Will this work with TypeList? hashicorp/terraform-plugin-sdk#477
The function is giving me strings and not entries from a list. How will this work? Do you have an example?
I used TypeSet to fix the issue with the unordered list
There was a problem hiding this comment.
@megan07 thank you for the review
I also added the ForceNew: true on the field group_id.
| "role": member.Role, | ||
| "type": member.Type, | ||
| "status": member.Status, | ||
| "delivery_settings": member.DeliverySettings, |
There was a problem hiding this comment.
I saw this yesterday, but thought I was able to get it working - apparenlty I didn't? deliverySettings isn't returned by the API when we do a List, so we need to find a way to assign it the value we intended, I think.
Just throwing something together, I got this to work - but there very well might be a friendlier way to do it:
for i, member := range membersObj.Members {
deliverySettings := resourceGroupMembers().Schema["members"].Elem.(*schema.Resource).Schema["delivery_settings"].Default.(string)
if ds := d.Get(fmt.Sprintf("members.%d.delivery_settings", i)); ds != nil {
deliverySettings = ds.(string)
}
members[i] = map[string]interface{}{
...
"delivery_settings": deliverySettings,
...
}
}
| Required: true, | ||
| }, | ||
|
|
||
| "members": { |
There was a problem hiding this comment.
I apologize for not saying anything yesterday. I'm wondering if we would be better off making this a TypeList and writing a diffSuppressFunc to suppress any diffs on order? TypeSet can sometimes be a nuisance to maintain, and in this case, the diffSuppress might be easier. Do you have thoughts for/against this idea?
| @@ -213,12 +212,18 @@ func resourceGroupMembersRead(ctx context.Context, d *schema.ResourceData, meta | |||
|
|
|||
| members := make([]interface{}, len(membersObj.Members)) | |||
| for i, member := range membersObj.Members { | |||
| // Use value if presnet or default as "delivery_settings" is not provided by API | |||
| deliverySettings := resourceGroupMembers().Schema["members"].Elem.(*schema.Resource).Schema["delivery_settings"].Default.(string) | |||
| if ds := d.Get(fmt.Sprintf("members.%d.delivery_settings", i)); ds != nil { | |||
There was a problem hiding this comment.
I realized that I copied this over from when I was testing it as a TypeList, so the index i here needs to actually be the hash of the object that we saved in the Set function. Then, as I was looking into that, I conferred with some other team members and we realized that example I sent you was actually broken there, broken in the sense that it actually ends up as the default Set function. Since I don't work with TypeSet much (or ever) I've spent the day going over this and am still trying to decide if we want a Set or a List. I'm sorry I don't have more for you today, but I'm running through tests and will hopefully have more details for you tomorrow! Thanks for your patience!
There was a problem hiding this comment.
Ok! I think we will have to keep it TypeSet. There's a bug in the diffSuppressFunc that will prevent us from moving forward with a list. That being said...a coworker and I discussed it and rather than trying to find out the hash here, we can do a configMembers := d.Get("members").(*schema.Set), then loop through there to find the correct configured member and just grab it's deliverySettings. Does that make sense?
configMembers := d.Get("members").(*schema.Set)
members := make([]interface{}, len(membersObj.Members))
for i, member := range membersObj.Members {
// Use value if present or default as "delivery_settings" is not provided by API
deliverySettings := deliverySettingsDefault
for _, cm := range configMembers.List() {
cMem := cm.(map[string]interface{})
if cMem["email"].(string) == member.Email {
deliverySettings = cMem["delivery_settings"].(string)
break
}
}
You can also get rid of that Set function that I suggested you create earlier too. So sorry about all the confusion. And also, if you wouldn't mind making the "ALL_MAIL" a constant at the top (I have deliverySettingsDefault here) so we can have it defined in one place, and not have that long line of code we had earlier.
Again, sorry for all the confusion on this! If at any point you don't have the time to finish it off, let me know and I can pick it up! There may need to be some further tweaking in the tests as they seem a bit flakey, but might just be some eventual consistency problems. Thanks so much for your help on this!
There was a problem hiding this comment.
Hi Thanks for all the hints. I applied your suggestions, but if you have time to work on it, it would be great because I think we get the feature then quite faster ;)
This is a proposed resource & datasource which allows to manage all group members of a group in an authoritative way.
Primary used instead of group_member when many groups & members are managed to increase the performance and reduce the number of api requests during the state refresh.
#147