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

New resource 'azuread_group_member' #63

Closed
wants to merge 3 commits into from

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented Mar 13, 2019

This adds a new resource to manage Azure AD Group Membership with Terraform.

  • New resource 'azuread_group_member'
  • Add tests
  • Add documentation

Example Usage

data "azuread_user" "my_user" {
  user_principal_name = "johndoe@hashicorp.com"
}

resource "azuread_group" "my_group" {
  name = "my_group"
}

resource "azuread_group_member" "test" {
  group_object_id   = "${azuread_group.my_group.id}"
  member_object_id  = "${data.azuread_user.my_user.id}"
}

Argument Reference

The following arguments are supported:

  • group_object_id - (Required) The Object ID of the Azure AD Group you want to add the Member to.
  • member_object_id - (Required) The Object ID of the Azure AD Object you want to add as a Member to the Group. Supported Object types are Users, Groups or Service Principals.

@tiwood
Copy link
Contributor Author

tiwood commented Mar 13, 2019

Related: #36

@tiwood
Copy link
Contributor Author

tiwood commented Apr 4, 2019

@tombuildsstuff @katbyte
any idea when this can be reviewed?
Thanks!

@katbyte
Copy link
Collaborator

katbyte commented Apr 12, 2019

@tiwood, we are currently busy with .12 and some work on the azurerm provider. Unfortunately I can't make any promises but I hope to earmark some time towards azuread in a couple weeks near the end of the month.

@katbyte
Copy link
Collaborator

katbyte commented Apr 12, 2019

Additionally i'd like to support setting group members via the group resource itself at the same time this is released.

@tiwood
Copy link
Contributor Author

tiwood commented May 6, 2019

I will look into this in the next days.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @tiwood,

Thanks for the resource. Overall this looks pretty good to me and i've only left a couple comment inline. I would like to see support for setting group members from the group resource at the same time we merge this. If thats to big of ask let me know and i am happy to implement it myself.

}

_, err := client.AddMember(ctx, groupID, properties)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor these two lines can be merged:

	if _, err := client.AddMember(ctx, groupID, properties); err != nil {

}

err = members.NextWithContext(ctx)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two lines could be merged

}

if memberObjectID == "" {
log.Printf("[DEBUG] Azure AD Group Member was not found (groupObjectId:%q / memberObjectId:%q ) - removing from state!", groupID, memberID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this debug log redundant as we immediately return an error?


The following arguments are supported:

* `group_object_id` - (Required) The Object ID of the Azure AD Group you want to add the Member to.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we note that this forces a new resource?

Suggested change
* `group_object_id` - (Required) The Object ID of the Azure AD Group you want to add the Member to.
* `group_object_id` - (Required) The Object ID of the Azure AD Group you want to add the Member to. Changing this forces a new resource to be created.

The following arguments are supported:

* `group_object_id` - (Required) The Object ID of the Azure AD Group you want to add the Member to.
* `member_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Member to the Group. Supported Object types are Users, Groups or Service Principals.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `member_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Member to the Group. Supported Object types are Users, Groups or Service Principals.
* `member_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Member to the Group. Supported Object types are Users, Groups or Service Principals. Changing this forces a new resource to be created.

memberID := id[1]

resp, err := client.RemoveMember(ctx, groupID, memberID)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

@landro
Copy link
Contributor

landro commented May 22, 2019

Any idea about ETA for this PR, @tiwood ? Can I help in any way?

@ghost ghost removed the waiting-response label May 22, 2019
@katbyte
Copy link
Collaborator

katbyte commented May 23, 2019

@landro, looks like PR comments still need to be addressed as well as adding the members property to the group resource

@evenh
Copy link
Contributor

evenh commented Jun 5, 2019

@katbyte Would a new PR (based on this PR) be acceptable (with fixes)?

@ghost ghost removed the waiting-response label Jun 5, 2019
@katbyte
Copy link
Collaborator

katbyte commented Jun 6, 2019

@evenh, yep given this has sat for a month i think its reasonable to open a new one.

@landro
Copy link
Contributor

landro commented Jun 6, 2019 via email

@katbyte
Copy link
Collaborator

katbyte commented Jun 6, 2019

Looks like i had some fixes on a local branch, i'll push that now. nope was for group owners

One thing also is i was going to use {group_id}/member/{object_id} to prevent conflicts with owners

@tiwood
Copy link
Contributor Author

tiwood commented Jun 7, 2019

@katbyte @evenh @landro Thank you for helping, I'm currently pretty busy with customer projects and was unable to work on this in a timely matter.

As this is a much requested feature, your help is much appreciated.

@katbyte
Copy link
Collaborator

katbyte commented Jun 7, 2019

no bother @tiwood, thank you for starting them 🙂

@katbyte
Copy link
Collaborator

katbyte commented Jun 12, 2019

closing in favour of #100

@katbyte katbyte closed this Jun 12, 2019
katbyte pushed a commit that referenced this pull request Jul 12, 2019
@ghost
Copy link

ghost commented Jul 13, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Jul 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants