-
Notifications
You must be signed in to change notification settings - Fork 301
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
fixed #1031 adds provision on demand #1032
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iwarapter Thanks for working on this. If you aren't able to run acceptance tests that's no problem, we can run them in our testing tenant. I'm happy to make small fixes to the tests as needed and I can report test results back to you for potentially larger fixes.
On the whole this is looking good. We will need to rewrite the doc as the format/style produced by tfplugindocs unfortunately is different to the provider docs - I recommend copying an existing doc and changing it as needed. Additionally if you can look at the comments below, and finish up the Read, Update and Delete funcs, then I'll be happy to take another look. Thanks!
internal/services/serviceprincipals/synchronization_provision_on_demand_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_provision_on_demand_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_provision_on_demand_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_provision_on_demand_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_provision_on_demand_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_provision_on_demand_resource.go
Outdated
Show resolved
Hide resolved
b0c1284
to
a99707c
Compare
a99707c
to
e639ddb
Compare
hey @manicminer, thanks for the feedback. I have addressed all the parts you raised. For the acceptance test i'd need to know what the |
e639ddb
to
95c9255
Compare
@manicminer is there anything further I can do to help with this? |
@iwarapter Thanks, and apologies for the delayed reply. At this time I don't have the bandwidth to create tests for this, so if you could have a go at writing some acceptance tests that will help enormously. I don't mind kicking the tires and tweaking existing tests to get them working reliably, but this will be waiting a little while if I have to write the tests. Thanks! |
hey @manicminer i had a quick stab - iwarapter@9da78c0 there are a few //TODO which i need extra info for |
@manicminer have pushed the tests - from what i can see the databricks integration used for acctests isnt fully integrated so this can't be tested e2e with it (im just testing config and then the expected failure). is there another app the acctests can use to test full e2e provisioning? |
service_principal_id = azuread_service_principal.test.id | ||
synchronization_job_id = trimprefix(azuread_synchronization_job.test.id, "${azuread_service_principal.test.id}/job/") | ||
parameter { | ||
rule_id = "03f7d90d-bf71-41b1-bda6-aaf0ddbee5d8" //no api to check this so assuming the rule id is the same globally :finger_crossed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this ID is the same in my tenant
2757fa4
to
9be6c66
Compare
6d02e70
to
b8765ff
Compare
b8765ff
to
559acda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iwarapter Apologies for the delay in re-reviewing this. I was originally anticipating the resource would have Read and Delete functions, but on closer inspection of the API and provisioning process, I see that there is no way to track a single "provision".
This makes it impossible to track state in Terraform for this operation, as this is more of a fire and forget task than a resource having some sort of lifecycle. With that in mind, I'm unsure whether this would be a good fit for Terraform unless we can find some other way to track state here? Alternatively, if the primary use case is to provision users, this could possibly make sense as an optional block in the azuread_user
resource instead? That way, we'd have some tangible state to track after the sync job has been triggered. WDYT?
Hi @manicminer, thanks for getting back to this. Thats correct, but there is prior art for this style of resource - mainly the aws lambda invocation resource. The only way you could check if the resource has been provisioned currently is by calling the provision api and checking the response code - but this is definitely not what we want in the READ part 😄. I certainly wouldn't bundle this into any other resources as the closest one is the I disagree that it's not a good fit for terraform as it enables a key part of fully managing resources across azuread and a given service provider. Here's a quick example use case: Add a group to a azure application (all covered with this provider) This resource solves the gap between the two where the group won't exist in AWS as you at the mercy of the SCIM scheduler. I deploy many hundreds of groups/role bindings and this is quite painful. |
@iwarapter Thanks for the context, I see now that the sync can happen in either direction and the resulting object(s) are not likely to be directly managed anyway. In which case this seems like a reasonable way to implement this. Do you think it's worth adding a |
its not a bad shout, i cant think of any (personal) use cases for it. But simple enough to just add a map trigger |
I'll try add this tomorrow morning |
@manicminer lifted straight from the aws provider 😂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iwarapter Thanks, I pushed some minor style fixes and also a crash fix in expandSynchronizationJobApplicationParameters()
. LGTM! 👍
cool glad to see this getting merged :) |
<Actions> <action id="6d17e7acdb2f3311576150379e22805f2f9b4aa72ff00ec136aceee45cae4b98"> <h3>Bump Terraform `azuread` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>changes detected:
	"hashicorp/azuread" updated from "2.48.0" to "2.49.0" in file ".terraform.lock.hcl"</p> <details> <summary>2.49.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azuread/releases/tag/v2.49.0
FEATURES:

* **New Data Source:** `azuread_group_role_management_policy` ([#1327](hashicorp/terraform-provider-azuread#1327 **New Resource:** `azuread_group_role_management_policy` ([#1327](hashicorp/terraform-provider-azuread#1327 **New Resource:** `azuread_privileged_access_group_assignment_schedule` ([#1327](hashicorp/terraform-provider-azuread#1327 **New Resource:** `azuread_privileged_access_group_eligibility_schedule` ([#1327](hashicorp/terraform-provider-azuread#1327 **New Resource:** `azuread_synchronization_job_provision_on_demand` ([#1032](https://github.com/hashicorp/terraform-provider-azuread/issues/1032))

ENHANCEMENTS:

* `data.azuread_group` - support for the `include_transitive_members` property ([#1300](hashicorp/terraform-provider-azuread#1300 `azuread_application` - relax validation for the `identifier_uris` property to allow more values ([#1351](hashicorp/terraform-provider-azuread#1351 `azuread_application_identifier_uri` - relax validation for the `identifier_uri` property to allow more values ([#1351](hashicorp/terraform-provider-azuread#1351 `azuread_group` - support the `SkipExchangeInstantOn` value for the `behaviors` property ([#1370](hashicorp/terraform-provider-azuread#1370 `azuread_user` - relax validation for the `employee_type` property to allow more values ([#1328](https://github.com/hashicorp/terraform-provider-azuread/issues/1328))

BUG FIXES:

* `azuread_application_pre_authorized` - fix a destroy-time bug that could prevent deletion of the resource ([#1299](https://github.com/hashicorp/terraform-provider-azuread/issues/1299))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/158/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
Hey @manicminer,
I have tested this with an AWS SSO configured app and it worked well, however I don't have access to an Azure tenant I can test the providers functional tests on. Not sure how best to proceed atm.
Contains existing PR changes for reference only, will rebase once they are merged.