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.

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

Update win_domain_group_membership.ps1 #56953

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@AravindBalajiS
Copy link
Contributor

commented May 25, 2019

Bug fixes while Adding and Deleting a group from another group

SUMMARY

I was trying to add DEF group to ABC group after executing the ansible script I checked the members that are present in ABC groups, the DEF group was not added to it. As DEF group was not present in ABC group after executing the ansible script then I tried to add DEF group to ABC group using the ps1 script, after execution of the ps1 script I found that DEF is a universal group and ABC is a global group, as a global group cannot have a universal group as a member. So the ps1 script failed while executing. But while executing the ansible script it got passed, it didn't show any error while execting it.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_domain_group_membership

ADDITIONAL INFORMATION

STEPS TO REPRODUCE
This can be reproduced by running the following playbook

---
- hosts: nodes
  vars:
    domain_user_name: '************************'                                                           # Service Account Username
    domain_user_password: '********************'                                                           # Service Account Password                                                                         
    domain_name: '**********************'                                                                      # Zone of the DNS

  tasks:
  - name: Add a domain group to a domain group
    win_domain_group_membership:
      name: "ABC"
      members:
        - "DEF"
      domain_username: "{{ domain_user_name }}"
      domain_password: "{{ domain_user_password }}"
      domain_server: "{{ domain_name }}"
      state: present

EXPECTED RESULTS

TASK [Add a domain group to a domain group] *************************************************************************************************************************************************************************************************
task path: /home/test.yml:12
Using module file /usr/lib/python2.7/site-packages/ansible/modules/windows/win_domain_group_membership.ps1
<xx.xx.xx.xx> ESTABLISH WINRM CONNECTION FOR USER: test on PORT 5986 TO xx.xx.xx.xx
checking if winrm_host xx.xx.xx.xx is an IPv6 address
EXEC (via pipeline wrapper)
fatal: [xx.xx.xx.xx]: FAILED! => {
    "added": [],
    "changed": false,
    "msg": "Failed to add a group CN=DEF: A global group cannot have a universal group as a member",
    "removed": []
}

ACTUAL RESULTS

TASK [Add a domain group to a domain group] *************************************************************************************************************************************************************************************************
task path: /home/test.yml:12
Using module file /usr/lib/python2.7/site-packages/ansible/modules/windows/win_domain_group_membership.ps1
<xx.xx.xx.xx> ESTABLISH WINRM CONNECTION FOR USER: test on PORT 5986 TO xx.xx.xx.xx
checking if winrm_host xx.xx.xx.xx is an IPv6 address
EXEC (via pipeline wrapper)
changed: [xx.xx.xx.xx] => {
    "added": [
        "DEF"
    ],
    "changed": true,
    "members": [
        "XYZ"
    ],
    "removed": []
}

AravindBalajiS added some commits May 25, 2019

Update win_domain_group_membership.ps1
Bug fixes while Adding and Deleting a group from another group
Update win_domain_group_membership.ps1
Bug fixes while Adding and Deleting a group from another group
@ansibot

This comment has been minimized.

AravindBalajiS added some commits May 25, 2019

Update win_domain_group_membership.ps1
Bug fixes while Adding and Deleting a group from another group
Update win_domain_group_membership.ps1
Bug fixes while Adding and Deleting a group from another group

@bcoca bcoca removed the needs_triage label May 28, 2019

@bcoca bcoca requested a review from jborean93 May 28, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Please add a changelog fragment to document the bug fix.

@AravindBalajiS

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I have added the changelog fragment 57104 @ShachafGoldstein

@AravindBalajiS

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

Hi @ShachafGoldstein
Any update?

try {
Add-ADGroupMember -Identity $name -Members $group_member -WhatIf:$check_mode @extra_args
} catch {
Fail-Json $result "Failed to add a group $($group_member): $($_.Exception.Message)"

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 22, 2019

Contributor

A silly comment but probably should be Failed to add group $($group_member): $($_.Exception.Message) or Failed to add a group: $($group_member) - $($_.Exception.Message)

try {
Remove-ADGroupMember -Identity $name -Members $group_member -WhatIf:$check_mode @extra_args -Confirm:$False
} catch {
Fail-Json $result "Failed to remove a group $($group_member): $($_.Exception.Message)"

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 22, 2019

Contributor

Same as the add comment

try {
Remove-ADGroupMember -Identity $name -Members $current_member -WhatIf:$check_mode @extra_args -Confirm:$False
} catch {
Fail-Json $result "Failed to remove a group $($group_member): $($_.Exception.Message)"

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 22, 2019

Contributor

Here as well, regarding the grammar/text

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

Hi @ShachafGoldstein
Any update?

I can't really merge anything but if it is urgent for you, you can add it to the weekly meeting agenda and join the IRC to move it faster.

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

Personally I don’t see the point to this PR. A try/catch is only fired in the event of a terminating error which means the script would have stopped. Since 2.8 we should be giving better error message that will result in a lot better error output which removes the necessity of the Fail-Json for individual commands. We should only be using Fail-Json if we are manually throwing a failure or the error message doesn’t make sense in the context where it happens.

@AravindBalajiS

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

@ShachafGoldstein @jborean93
Thanks for comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.