-
Notifications
You must be signed in to change notification settings - Fork 71
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
Issue58 - Add extra attributes to resource user #63
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.
Looks great, well done!
Hi @jpatigny I added another commit to this PR after generating the docs. This provider uses https://github.com/hashicorp/terraform-plugin-docs/ for doc generation. This tool will generate the documentation based on the Please take a look and if you're happy let me know and I'll go ahead and merge this PR. |
ad/resource_ad_user.go
Outdated
"github.com/hashicorp/terraform-provider-ad/ad/internal/winrmhelper" | ||
) | ||
|
||
const countryRegexp = `^(A(D|E|F|G|I|L|M|N|O|R|S|T|Q|U|W|X|Z)|B(A|B|D|E|F|G|H|I|J|L|M|N|O|R|S|T|V|W|Y|Z)|C(A|C|D|F|G|H|I|K|L|M|N|O|R|U|V|X|Y|Z)|D(E|J|K|M|O|Z)|E(C|E|G|H|R|S|T)|F(I|J|K|M|O|R)|G(A|B|D|E|F|G|H|I|L|M|N|P|Q|R|S|T|U|W|Y)|H(K|M|N|R|T|U)|I(D|E|Q|L|M|N|O|R|S|T)|J(E|M|O|P)|K(E|G|H|I|M|N|P|R|W|Y|Z)|L(A|B|C|I|K|R|S|T|U|V|Y)|M(A|C|D|E|F|G|H|K|L|M|N|O|Q|P|R|S|T|U|V|W|X|Y|Z)|N(A|C|E|F|G|I|L|O|P|R|U|Z)|OM|P(A|E|F|G|H|K|L|M|N|R|S|T|W|Y)|QA|R(E|O|S|U|W)|S(A|B|C|D|E|G|H|I|J|K|L|M|N|O|R|T|V|Y|Z)|T(C|D|F|G|H|J|K|L|M|N|O|R|T|V|W|Z)|U(A|G|M|S|Y|Z)|V(A|C|E|G|I|N|U)|W(F|S)|Y(E|T)|Z(A|M|W))$` |
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.
Is the country code enforced on the AD side ? If not we could "relax" the requirement a bit, for example "any three letter string".
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.
Country code is enforced on AD level :
PS C:\terraform> Set-ADUser -Identity tfuser -Country belgium
Set-ADUser : A value for the attribute was not in the acceptable range of values
At line:1 char:1
+ Set-ADUser -Identity tfuser -Country belgium
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (tfuser:ADUser) [Set-ADUser], ADException
+ FullyQualifiedErrorId : ActiveDirectoryServer:8322,Microsoft.ActiveDirectory.Management.Commands.SetADUser
PS C:\terraform> Set-ADUser -Identity tfuser -Country be
PS C:\terraform> Get-ADUser -Identity tfuser -Properties * | select country
country
-------
be
I adapted the code to allow string lenght of 2 or 3 char max.
I've tested this locally, I will eventually update the tests to not having hard coded values and add some documentation on how to use them. |
add-input-validation-for-country add-input-validation-for-initials
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! |
Description
Adding almost all built-in attributes for the resource user (custom attributes are not added) :
NB: I'm not comfortable (yet) to write tests (acceptance etc). It would be nice if I could get some help or if someone could take care of it
References
issue-58
Community Note