Repository: ansible/ansible
Pull request: win_domain reboot required exception incorrectly reported
##### SUMMARY
If the module fails because a reboot is pending it is stated in the exception message, but reboot_required is still false.

Fixes: #60493

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
win_domain

##### ADDITIONAL INFORMATION

```
fatal: [****-****]: FAILED! => {
    "changed": true,
    "msg": "Failed to install ADDSForest with DCPromo: Name change pending. A reboot is required.\r\n",
    "reboot_required": false
}
```


Problem statement:
win_domain reboot required exception incorrectly reported
##### SUMMARY
If the module fails because a reboot is pending it is stated in the exception message, but reboot_required is still false.

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
win_domain

##### ANSIBLE VERSION
```paste below
ansible 2.9.0.dev0
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/user/PycharmProjects/akaTest/lib/ansible
  executable location = /home/user/PycharmProjects/akaTest/bin/ansible
  python version = 3.7.4 (default, Jul 16 2019, 07:12:58) [GCC 9.1.0]
```

##### CONFIGURATION

##### OS / ENVIRONMENT
ArchLinux

##### STEPS TO REPRODUCE
Rename the computer and than run win_domain.

##### EXPECTED RESULTS
The task failing, but reboot_required also being true.

##### ACTUAL RESULTS

```paste below
fatal: [****-****]: FAILED! => {
    "changed": true,
    "msg": "Failed to install ADDSForest with DCPromo: Name change pending. A reboot is required.\r\n",
    "reboot_required": false
}
```

I'll do a pr in a few.

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/windows/win_domain.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_domain.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @ShachafGoldstein @if-meaton @jborean93 @jhawkesworth @nitzmahone
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Set reboot required dependent on exception message.

Patch to review:
diff --git a/lib/ansible/modules/windows/win_domain.ps1 b/lib/ansible/modules/windows/win_domain.ps1
index de31134ba7de77..998eda0d50125e 100644
--- a/lib/ansible/modules/windows/win_domain.ps1
+++ b/lib/ansible/modules/windows/win_domain.ps1
@@ -127,6 +127,9 @@ if (-not $forest) {
         if ($_.Exception.ExitCode -eq 15) {
             $result.reboot_required = $true
         } else {
+            if ($_.Exception.Message.Contains("A reboot is required.")) {
+                $result.reboot_required = $true
+            }
             Fail-Json -obj $result -message "Failed to install ADDSForest with DCPromo: $($_.Exception.Message)"
         }
     }


Code review comments:
diff_hunk: @@ -127,6 +127,9 @@ if (-not $forest) {
         if ($_.Exception.ExitCode -eq 15) {
             $result.reboot_required = $true
         } else {
+            if ($_.Exception.Message.Contains("A reboot is required.")) {
path: lib/ansible/modules/windows/win_domain.ps1
text: @user1:
Using the exception message to detect this is probably the wrong way to go, it would most likely fall flat as soon as another language is being used. Looking at https://docs.microsoft.com/en-us/windows-server/identity/ad-ds/deploy/troubleshooting-domain-controller-deployment it seems like we should include 19 as an ExitCode in the check above, e.g. `$_.Exception.ExitCode -in @(15, 19)`. We should also do the same for `win_domain_controller` as I believe it already has a similar logic to this right now.

It might be a good idea ot include the ExitCode in the Fail-Json message as well to help detect these problems in the future.

@author:
I encountered that issue on a german server, so I just assumed it is always english, but you're right we should not rely on the message and use exit codes instead.

@author:
I don't see any try catch around the `Install-ADDSDomainController` except for the generic one. As I don't know if the error codes are equivalent, I'm not including it for now into this pr.

@user1:
> I don't see any try catch around the Install-ADDSDomainController except for the generic one

Hmm I must be imagining things then sorry.