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

Resource: 'azuread_group_member' #100

Merged
merged 25 commits into from Jul 12, 2019

Conversation

@evenh
Copy link
Contributor

evenh commented Jun 6, 2019

This PR continues where #63 left off. In addition to fixing the comments pointed out in the review, it introduces support for adding members to a group.

Go is not my main language and I'm not that familiar with Terraform so don't be afraid to do a proper review :-)

@evenh evenh force-pushed the digipost:r_group_member branch from f9454b6 to b1dd10d Jun 6, 2019
@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Jun 12, 2019

ping @katbyte :)

Copy link
Contributor

katbyte left a comment

Thanks for opening this PR to get this in @evenh,

I've left some comments inline with my main concerns being:

  • moving some of the shared code into to graph helper package
  • supporting all object types in the group.members property
  • changing the ID to group_id/members/object_id sp it doesn't collide with group owners

In addition to the inline comments could we add a test that has 1 or more of each type of member as well as an update test going from none -> one -> many -> differnt_one -> none with import checks in between each step?

azuread/resource_group.go Outdated Show resolved Hide resolved
azuread/resource_group.go Outdated Show resolved Hide resolved
azuread/resource_group.go Outdated Show resolved Hide resolved
azuread/resource_group_member.go Outdated Show resolved Hide resolved
URL: &memberGraphURL,
}

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

This comment has been minimized.

Copy link
@katbyte

katbyte Jun 12, 2019

Contributor

Will this fail if the member already exists?

This comment has been minimized.

Copy link
@katbyte

katbyte Jun 12, 2019

Contributor

Could we also reuse the graph.GroupAddMember function discussed above?

This comment has been minimized.

Copy link
@evenh

evenh Jun 18, 2019

Author Contributor

I've refactored to use the new helper function. However, if a member already exists it fails. What is the desired outcome here? Given the following HCL:

// User one already exists
resource "azuread_group" "some_group" {
  name = "SomeGroup"
}

resource "azuread_group_member" "one" {
  group_object_id   = azuread_group.some_group.id
  member_object_id  = azuread_user.one.id
}

resource "azuread_group_member" "another" {
  group_object_id   = azuread_group.some_group.id
  member_object_id  = azuread_user.one.id
}

In that case Terraform sees two "new" resource in azuread_group_member.one and azuread_group_member.another. After applying the first one, another fails with

graphrbac.GroupsClient#AddMember: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_BadRequest","date":"2019-06-18T11:09:20","message":{"lang":"en","value":"One or more added object references already exist for the following modified properties: 'members'."},"requestId":"c7ad6982-559e-46eb-bfed-56b6455141c8"}}]
website/docs/r/group.markdown Outdated Show resolved Hide resolved
website/docs/r/group.markdown Outdated Show resolved Hide resolved
azuread/resource_group.go Outdated Show resolved Hide resolved
website/docs/r/group_member.markdown Outdated Show resolved Hide resolved
website/docs/r/group_member.markdown Outdated Show resolved Hide resolved
@katbyte katbyte added this to the v0.5.0 milestone Jun 12, 2019
@katbyte katbyte mentioned this pull request Jun 12, 2019
3 of 3 tasks complete
@katbyte

This comment has been minimized.

Copy link
Contributor

katbyte commented Jun 12, 2019

I have also opened a PR for a data source to get object IDs for multiple users (#109) that would be nice to be used here in the example/tests here somewhere. but its not a blocker as i can add that in afterwards

@evenh evenh force-pushed the digipost:r_group_member branch from f4bb69e to 4d6e3f9 Jun 17, 2019
@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Jun 18, 2019

Some of the comments is now fixed!

I've used a day trying to make acceptance tests work from my local setup but to no avail (makes writing new acceptance tests a little hard). Any pointers on how the service principal used for running acceptance tests is configured?

@hashibot hashibot bot removed the waiting-response label Jun 18, 2019
@katbyte

This comment has been minimized.

Copy link
Contributor

katbyte commented Jun 18, 2019

Hi @evenh,

You should be able to follow the instructions here: https://www.terraform.io/docs/providers/azuread/auth/service_principal_client_secret.html

Make sure you grant it the appropriate access here: https://www.terraform.io/docs/providers/azuread/auth/service_principal_configuration.html

I believe there is also an example on master in the examples folder demonstrating this too if you'd prefer a terraform'y way

evenh added 2 commits Jun 19, 2019
This satisfies resource_group_member_test
@katbyte

This comment has been minimized.

Copy link
Contributor

katbyte commented Jun 20, 2019

@evenh, is this ready for re-review?

@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Jun 24, 2019

Ready for a re-review now @katbyte. Also note the two open inline comments :-)

Copy link
Contributor

katbyte left a comment

Hi @evenh,

Thank you for all the changes, the is looking pretty good. I have left some comments inline with my main concern being the tests. Would we change them to be more consistent with how the others are written?

Look forward to get this this merged once the comments are all addressed 🙂

azuread/resource_group_member.go Outdated Show resolved Hide resolved
azuread/resource_group_member.go Outdated Show resolved Hide resolved
azuread/resource_group.go Outdated Show resolved Hide resolved
azuread/helpers/graph/group.go Outdated Show resolved Hide resolved
azuread/helpers/graph/group.go Outdated Show resolved Hide resolved
azuread/resource_group_test.go Outdated Show resolved Hide resolved
azuread/resource_group_test.go Outdated Show resolved Hide resolved
azuread/resource_group_test.go Outdated Show resolved Hide resolved
azuread/resource_group_test.go Outdated Show resolved Hide resolved
azuread/resource_group_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

katbyte left a comment

Clicked the wrong button 😓

@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Jun 25, 2019

I've fixed most of your comments @katbyte and added some of my own :)

@hashibot hashibot bot removed the waiting-response label Jun 25, 2019
@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Jun 26, 2019

I've converted all of resource_group_test.go to HCL @katbyte :)

Copy link
Contributor

katbyte left a comment

Thanks for the revisions @evenh,

Only a couple more minor comments and this should be good to merge 🙂

azuread/resource_group.go Outdated Show resolved Hide resolved
azuread/resource_group.go Outdated Show resolved Hide resolved
@evenh evenh force-pushed the digipost:r_group_member branch from 45c32e5 to 2bef9ed Jun 28, 2019
@evenh evenh force-pushed the digipost:r_group_member branch from 2bef9ed to dbde7fb Jun 28, 2019
@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Jun 28, 2019

There you go @katbyte :-)

@evenh

This comment has been minimized.

Copy link
Contributor Author

evenh commented Jul 3, 2019

Could you please have another look @katbyte?

@aambert

This comment has been minimized.

Copy link

aambert commented Jul 8, 2019

Up

Copy link
Contributor

katbyte left a comment

Thanks for the revisions @evenh! Apologies for the delay in reviewing it but azurerm required some attention.

This LGTM however there is a test failure due to replication:

------- Stdout: -------
=== RUN   TestAccAzureADGroup_progression
--- FAIL: TestAccAzureADGroup_progression (65.21s)
    testing.go:568: Step 8 error: errors during apply:
        
        Error: Error Deleting group member "994abfa7-3681-40a5-962d-50cca6383c46" from Azure AD Group with ID "cd0932a7-2fda-4572-9350-f86f09dbd0c3": graphrbac.GroupsClient#RemoveMember: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_BadRequest","date":"2019-07-11T22:06:08","message":{"lang":"en","value":"One or more removed object references do not exist for the following modified properties: 'members'."},"requestId":"33425947-652e-45df-b224-37d0aa7ec671"}}]
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test261138976/main.tf line 2:
          (source code not available)
        
        
FAIL

However I feel like that is larger than the scope of this PR (will also apply to owners) so I am going to merge anyways.

@katbyte katbyte merged commit e9db6a8 into terraform-providers:master Jul 12, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
katbyte added a commit that referenced this pull request Jul 12, 2019
@evenh evenh deleted the digipost:r_group_member branch Jul 12, 2019
@hashibot

This comment has been minimized.

Copy link

hashibot bot commented Aug 11, 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!

@hashibot hashibot bot locked and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.