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

Add azuread_administrative_unit_role_member #983

Merged
merged 7 commits into from
Apr 13, 2023

Conversation

alexwilcox9
Copy link
Contributor

Having a go at adding azuread_administrative_unit_role_member. This resource will assign a directory object a role at the scope of the administrative unit.

I have hit some issues I am hoping you can help with.
Firstly I have had to comment out the ParseUUID check as for some reason these IDs are not of that format

GET https://graph.microsoft.com/v1.0/directory/administrativeUnits/23e87ebc-7ecb-4de7-9f11-468fcd3486da/scopedRoleMembers
{
    "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#scopedRoleMemberships",
    "value": [
        {
            "id": "zX37MRLyF0uvE-xf2WH4B7x-6CPLfudNnxFGj800htpBXqkxW7bITqGb6Rj4kuTuS",
            "roleId": "31fb7dcd-f212-4b17-af13-ec5fd961f807",
            "administrativeUnitId": "23e87ebc-7ecb-4de7-9f11-468fcd3486da",
            "roleMemberInfo": {
                "id": "31a95e41-b65b-4ec8-a19b-e918f892e4ee",
                "displayName": "acctestServicePrincipal-230119232618040660"
            }
        }
    ]
}

Secondly I am having issues with the exists func. I'm mostly confident I'm giving the GetScopedRoleMember function the right inputs but I end up with the error below.

testcase.go:60: Step 1/2 error: Check failed: Check 1/3 error: running exists func for "azuread_administrative_unit_role_member.test": failed to retrieve administrative unit role member "zX37MRLyF0uvE-xf2WH4B7x-6CPLfudNnxFGj800htpBXqkxW7bITqGb6Rj4kuTuS" (AU ID: "23e87ebc-7ecb-4de7-9f11-468fcd3486da"): AdministrativeUnitsClient.BaseClient.Get(): Get "https://graph.microsoft.com/beta/<TENANT_ID>/administrativeUnits/23e87ebc-7ecb-4de7-9f11-468fcd3486da/scopedRoleMembers/zX37MRLyF0uvE-xf2WH4B7x-6CPLfudNnxFGj800htpBXqkxW7bITqGb6Rj4kuTuS": http: RoundTripper implementation (*retryablehttp.RoundTripper) returned a nil *Response with a nil error

Any insight would be really appreciated!

@manicminer
Copy link
Member

manicminer commented Feb 13, 2023

Hi @alexwilcox9, thanks for working on this. For some reason which I haven't yet gotten to the bottom of, we encountered an issue specifically with terraform-plugin-sdk v2.24.1 that causes the context to be cancelled during acceptance test checks, which in turn yields a "nil response with nil error" message for Hamilton clients. I've pushed a commit to your branch rolling this back to v2.24.0 - you are welcome to squash/rebase this away.

For the ParseUUID issue, if you could split this logic out into two functions, one for each ID type, that would be ideal.

@alexwilcox9
Copy link
Contributor Author

HI @manicminer, thanks for the help
Your commit did get things to start working however when I rebased from main things broke again. I instead rebased off of tag v2.33.0 and that seems to be working. I suspect the change in setting the API version explicitly is causing me problems, I did try adding administrativeUnitsClient.BaseClient.ApiVersion = msgraph.VersionBeta to the admin units client.go but this didn't seem to make a difference

I've added a new Parse function as suggested, I don't recognise the IDs MS are using as a standard format and the docs aren't insightful so just checking it's not empty at the moment. Here are a couple examples of valid IDs just in-case you do recognise the format

ePROZI_iKE653D_d6aoLH4IFHUtHrxtBvJjGx9C96Vln6KeGcIToSI0VIiz_1qpS-2
zX37MRLyF0uvE-xf2WH4B7x-6CPLfudNnxFGj800htpBXqkxW7bITqGb6Rj4kuTuS

I've also included a commit to update references to parts[2] to id.Type to make it more readable

@alexwilcox9 alexwilcox9 force-pushed the scoped-role-member branch 2 times, most recently from 0a5c68a to 71e72a3 Compare February 14, 2023 23:54
@alexwilcox9 alexwilcox9 marked this pull request as ready for review February 14, 2023 23:56
@manicminer
Copy link
Member

@alexwilcox9 How is it working if you rebase off main now? main was broken for much of last week; I merged some fixes on Friday.

@alexwilcox9
Copy link
Contributor Author

@manicminer just rebased against main and the tests are passing fine now, looks like those fixes worked

@manicminer manicminer added this to the v2.37.0 milestone Apr 13, 2023
Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@alexwilcox9 Thanks for working on this! Sorry for the delay in reviewing, I've merged main and pushed a couple of fixes and this LGTM 🛥️

@manicminer
Copy link
Member

Test results

Screenshot 2023-04-13 at 22 48 51

@manicminer manicminer merged commit a539dbe into hashicorp:main Apr 13, 2023
7 checks passed
manicminer added a commit that referenced this pull request Apr 13, 2023
@github-actions
Copy link

This functionality has been released in v2.37.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2024
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

2 participants