azuread_service_principal - support for the AppRoleAssignmentRequired property #127
Conversation
… service principal
There was a problem hiding this comment.
Hi @dmeenhuis,
Thank you for the PR! overall this is looking pretty good,in addition to the comments i've left inline could could we get a new test (_update) that takes the basic config and updates it to complete to ensure the update function is now working? thnks!
| @@ -131,7 +133,8 @@ resource "azuread_application" "test" { | |||
| } | |||
|
|
|||
| resource "azuread_service_principal" "test" { | |||
| application_id = "${azuread_application.test.application_id}" | |||
| application_id = "${azuread_application.test.application_id}" | |||
| app_role_assignment_required = true | |||
There was a problem hiding this comment.
COuld we align these assingments here?
There was a problem hiding this comment.
oauth2_permissions would be great, tags however we always leave at the end. Thanks!
| @@ -27,6 +27,7 @@ resource "azuread_application" "example" { | |||
|
|
|||
| resource "azuread_service_principal" "example" { | |||
| application_id = "${azuread_application.example.application_id}" | |||
| app_role_assignment_required = false | |||
There was a problem hiding this comment.
Could we aling these assingments here?
| @@ -44,6 +45,11 @@ func resourceServicePrincipal() *schema.Resource { | |||
| Computed: true, | |||
| }, | |||
|
|
|||
| "app_role_assignment_required": { | |||
There was a problem hiding this comment.
Could we move this above the computed properties?
There was a problem hiding this comment.
Done. What about the oauth2_permissions and tags, would you like me to move those above the computed properties as well, for consistency's sake?
|
I added a test to check the update function. Wasn't entirely sure how to go about it, so I took the update test from application and followed a similar approach for service principal and the test seems to pass. Could you let me know whether I implemented it correctly? |
There was a problem hiding this comment.
Thanks for making the requested changes @dmeenhuis,
Aside from one minor comment this is looking great!
| @@ -36,43 +36,47 @@ resource "azuread_service_principal" "example" { | |||
|
|
|||
| The following arguments are supported: | |||
|
|
|||
| * `application_id` - (Required) The ID of the Azure AD Application for which to create a Service Principal. | |||
| - `application_id` - (Required) The ID of the Azure AD Application for which to create a Service Principal. | |||
There was a problem hiding this comment.
Could we not change these from * to - for consistency with the rest of the provider docs?
|
This has been released in version 0.6.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azuread" {
version = "~> 0.6.0"
}
# ... other configuration ... |
|
Thanks for merging these changes! I wasn't able to respond earlier since I was on holidays for the past 3 weeks. |
|
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! |
ghost
locked and limited conversation to collaborators
This adds support for setting the
AppRoleAssignmentRequiredproperty on a service principal.I'm not sure if I implemented the update function correctly, but since it was not already there I think I had to create it.