Skip to content
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.

Sign up for GitHub

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

Jump to bottom

New Module: win_domain_ou take 2 #278

Merged
merged 51 commits into from
Sep 23, 2021
Merged

New Module: win_domain_ou take 2 #278

merged 51 commits into from
Sep 23, 2021

Conversation

gamethis
Copy link
Contributor

The win_domain_ou module manages Active Directory Organizational Units (OUs).
ISSUE TYPE

New Module Pull Request

COMPONENT NAME

win_domain_ou
ADDITIONAL INFORMATION

Includes Integration Tests - See documentation file for full details on how the module works.

gamethis added 29 commits August 20, 2020 16:10
@gamethis
Add Proxy option
7a8bb09
@gamethis
add proxy to doc
9a9edc5
@gamethis
version_added: 1.1.0
6dec2b7
@gamethis
Merge pull request #1 from ansible-collections/main
78946ad 
pulling current state
@gamethis
Adding win_feature_info module
ca74f44
@gamethis
fixing win_feature_info doc file
fc2638a
@gamethis
Update win_feature_info.py
d20bfe3
@gamethis
Update win_feature_info.ps1
c465d77
@gamethis
added version_added: '1.4.0'
a0ad39c
@gamethis
removed ordered, and unneeded lines at top
d6b6849
@gamethis
Ading tests
40e1f3c
@gamethis
added gnu back
49863aa
@gamethis
put proper basic import in
219ef6d
@gamethis
trying different alias
4f62b9e
@gamethis
updated with test_feature
10d1bcf
@gamethis
updated names for clarity of test
399a657
@gamethis
Merge pull request #2 from ansible-collections/main
7def516 
syncing
@gamethis
added win_domain_ou module
47adcb9
@gamethis
adding aliases
c170fda
@gamethis
adding defaults
bb69cbc
@gamethis
Create main.yml
acb35aa
@gamethis
Create default.yml
282ef48
@gamethis
Create main.yml
3fe224e
@gamethis
Create quick.yml
c681e2f
@gamethis
fixing doc
fce2ac2
@gamethis
linting
1ed27d3
@gamethis
linted
c95bbe9
@gamethis
more linting
e7d30e0
@gamethis
fix assertion test
30e7a04
jborean93
jborean93 requested changes Aug 23, 2021
View reviewed changes
Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the information here looks good. I do have some concerns with trying to convert the LDAP properties to and from snake_cake. I personally think they should just be as is and have people set the actual LDAP property name in that dict and avoid all the conversions in the module. For example I think postal_code should stay as postalCode to better reflect the raw nature of properties.

What are your thoughts on this?

plugins/modules/win_domain_ou.py Outdated Show resolved Hide resolved
plugins/modules/win_domain_ou.py Outdated Show resolved Hide resolved
@gamethis
Copy link
Contributor Author

gamethis commented Sep 16, 2021 •
edited
Loading

fixed issues. closed and reopened to get the test to run again. everything is good now.

@gamethis gamethis requested a review from jborean93 September 16, 2021 17:14
@gamethis gamethis closed this Sep 16, 2021
@gamethis gamethis reopened this Sep 16, 2021
@gamethis
Copy link
Contributor Author

Most of the information here looks good. I do have some concerns with trying to convert the LDAP properties to and from snake_cake. I personally think they should just be as is and have people set the actual LDAP property name in that dict and avoid all the conversions in the module. For example I think postal_code should stay as postalCode to better reflect the raw nature of properties.

What are your thoughts on this?
Made the changes

jborean93
jborean93 reviewed Sep 16, 2021
View reviewed changes
Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor nits, looks like the doc examples also need to be updated to use the actual LDAP property names rather then the snake_case ones that were originally in place.

plugins/modules/win_domain_ou.ps1 Outdated Show resolved Hide resolved
plugins/modules/win_domain_ou.ps1 Outdated Show resolved Hide resolved
plugins/modules/win_domain_ou.py Outdated Show resolved Hide resolved
@gamethis gamethis requested a review from jborean93 September 16, 2021 23:10
jborean93
jborean93 reviewed Sep 16, 2021
View reviewed changes
plugins/modules/win_domain_ou.ps1 Outdated Show resolved Hide resolved
plugins/modules/win_domain_ou.py Outdated Show resolved Hide resolved
plugins/modules/win_domain_ou.py Outdated Show resolved Hide resolved
plugins/modules/win_domain_ou.py Outdated Show resolved Hide resolved
plugins/modules/win_domain_ou.py Outdated Show resolved Hide resolved
@gamethis gamethis requested a review from jborean93 September 17, 2021 01:42
@jborean93 jborean93 merged commit f3a2c5d into ansible-collections:main Sep 23, 2021
jborean93 added a commit to jborean93/community.windows that referenced this pull request Sep 23, 2021
@jborean93
Revert "New Module: win_domain_ou take 2 (ansible-collections#278)"
8535035 
This reverts commit f3a2c5d.
@jborean93 jborean93 mentioned this pull request Sep 23, 2021
Merged
jborean93 added a commit that referenced this pull request Sep 23, 2021
@jborean93
Revert "New Module: win_domain_ou take 2 (#278)" (#297)
cc4bd71 
This reverts commit f3a2c5d.
@jborean93
Copy link
Collaborator

I've unfortunately had to revert this commit as further testing found some larger problems #297. It seems like some of the tests have been written around actual results rather than the proper expectations, testing something had not changed when in reality it should for that scenario.

@gamethis
Copy link
Contributor Author

I don't fully comprehend the difference in the final test and why it shows different from the other... bu t I think I have found the error in the code that caused it... I will go back and iprove the test and reoopen a pr in next day or so

@gamethis gamethis mentioned this pull request Sep 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jborean93 jborean93 jborean93 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gamethis @jborean93