New data source: azuread_directory_object #847
Conversation
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @Threpio, thanks for working on this PR. This is looking pretty good! I have made some suggestions inline below, and we will also need to add some acceptance tests for this data source.
If you can take a look at the comments below, and look to add one or two acceptance tests (take a look at some other data sources for inspiration :), then I'll be happy to take another look. Thanks!
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/directoryobjects/principal_type_data_source.go
Outdated
Show resolved
Hide resolved
Additional Interpolation removed Co-authored-by: Tom Bamford <[email protected]>
Imports tidied up Co-authored-by: Tom Bamford <[email protected]>
Wording as per suggestion Co-authored-by: Tom Bamford <[email protected]>
|
Most of the comments have been applied and committed -Sorry for the horrible commit mess. I am not sure that I have handled the I will work on the tests asap. |
|
@Threpio Thanks, I'll take another look once the tests are pushed. Don't worry about the commit history, we can squash it :) |
|
Tests have been added -I cannot apply the last suggestion as it is outdated and yet not updated. For the tests I have done 3 success cases - I do not see examples of failing to terraform configuration being used for different unit tests and so have not written these. The docs are linted but the content might need a 5-minute review. |
There was a problem hiding this comment.
Hi @Threpio, many thanks for working on this and for adding the tests - which should be enough to cover usage of this data source.
I've run through this again and made some changes, mainly renaming the data source from azuread_principal_type to azuread_directory_object - because I expect we'll support additional object types in future and this will help us avoid deprecating a data source in order to rename it.
In addition to that, I've made some fixes to the tests, added some nil checking that I missed on my first review, and made some documentation updates. I added the latter as inline suggestions below to give you an idea of how we like to structure and word the docs.
This is a great addition, many thanks for your effort on this! As I have made some additional changes, I'm going to open this for another maintainer to review but pending any other changes I'm happy for this to be merged and added to the provider.
manicminer
changed the title
…ord (hashicorp#844) * Simple fix for end_date_relative bug * Testing locally + logic * There is no reason these two functions should be this different
|
This functionality has been released in v2.28.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! |
|
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. |
#824