-
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
Add support for automatic provisioning using synchronization jobs #830
Conversation
3caaf51
to
eda8da6
Compare
eda8da6
to
1ab9e5c
Compare
1ab9e5c
to
0f554da
Compare
0f554da
to
1f62fc5
Compare
The PR should be ready for review now. I ran into a typing issue after some further testing which I solved in the vendored library manicminer/hamilton#172 |
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.
@JoostvDoorn Thanks for this contribution, and for your patience with the delay on reviewing this. This is looking really great, I have some comments below which are largely related to each other. Your Hamilton fix was merged earlier today and this should be vendored into main fairly soon.
If you could take a look at the suggested changes, and also if we can add an acceptance test for a disabled synchronization job, then this should be good to merge. Thanks again!
internal/services/serviceprincipals/synchronization_job_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_job_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_job_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_job_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/synchronization_job_resource.go
Outdated
Show resolved
Hide resolved
1dbf4c6
to
808134c
Compare
Co-authored-by: Tom Bamford <[email protected]>
808134c
to
f04a55e
Compare
@manicminer I refactored this over the weekend (and squashed the history). This should be good to go, let me know if there is still some issues. |
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.
Thanks for making those changes @JoostvDoorn.
I see that you have changed the owners
property for both azuread_application
and azuread_service_principal
to be optional & computed. Whilst there are some bugs related to these properties, this is not our preferred way to dismiss an unwanted diff and I am hoping to be able to fix some of these bugs separately. Accordingly, I've reverted these additions but this PR otherwise LGTM and I'm happy to merge this. Thanks again!
This functionality has been released in v2.30.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! |
Hi, I have used the above example to implement SCIM configuration for Syncing AAD users with Databricks Workspace but am not able to pass the value of Databricks URL programmatically. The above syntax throws below error:
Could you please help me understand why it might be appearing? |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #744. This adds support for the synchronization job endpoints documented here.
A partially working example for databricks SCIM provisioning (you'll need to add app roles and assign users/group to app roles to make it work 100%):