Repository: ansible/ansible
Pull request: Added support for user-specified log file in win_package
##### SUMMARY
Users often want the ability to refer back to installer logs long after a deployment is done. This feature provides an optional parameter to win_package to facilitate specifying a log path. Default/existing behaviour for log path is unchanged.

Fixes #38353

##### ISSUE TYPE
- Feature Pull Request

##### COMPONENT NAME
win_package

##### ADDITIONAL INFORMATION
Standard behaviour for msiexec seems to overwrite the nominated log file should it already exist.
Additionally, standard msiexec behaviour for a path that does not exist is to fail with 1622.

fatal: [sdcmkt-tstets21]: FAILED! => {
    "changed": false,
    "msg": "unexpected rc from install  C:\\Windows\\system32\\msiexec.exe /i D:\\good.msi /L*V D:\\no\\path\\good-present.log /qn /norestart: see rc, stdout and stderr for more details",
    "rc": 1622,
    "reboot_required": false,
    "stderr": "",
    "stderr_lines": [],
    "stdout": "Error opening installation log file.  Verify that the specified log file location exists and is writable.\n\r",
    "stdout_lines": [
        "Error opening installation log file.  Verify that the specified log file location exists and is writable.",
        ""
    ]
}

Problem statement:
Feature: log_path variable for win_package and keep log output on success
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and devel branch are affected too.
Always add information AFTER of these html comments.
-->

##### ISSUE TYPE
 - Feature Idea

##### COMPONENT NAME
<!---
Insert below this comment the name of the module, plugin, task or feature.
Do not include extra details here, e.g. "vyos_command" not "the network module vyos_command" or the full path
-->
win_package

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
N/A

##### CONFIGURATION
<!---
If using Ansible 2.4 or above, paste the results of "ansible-config dump --only-changed"
Otherwise, mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->
N/A

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say "N/A" for anything that is not platform-specific.
Also mention the specific version of what you are trying to control,
e.g. if this is a network bug the version of firmware on the network device.
-->
N/A

##### SUMMARY
<!--- Explain the problem briefly -->
This is a actually two feature ideas in one:
1) Allow user to specify a `log_path` variable for `win_package` when installing an MSI to save the installation log in a specific location (could also be used to save any EXE output)
2) Keep and/or return the content of the log on success installation of an MSI.

##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->
1) Currently `win_package` always send log output to a temporary file that is later removed.
2) `win_package` also does not return the content of the log unless `msiexec` terminated with an exit code not in the list of expected results, making it impossible to use a following task to save or check that log when things worked as expected.
3) User cannot specify his own msiexec `/log` or `/l*v` options as `win_package` already do and `msiexec` will fail.

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
Allowing the user to do something like this:
```yaml
win_package:
  path: C:\path\to\myinstaller.msi
  log_path: C:\Windows\Temp\InstallerLogs\myinstaller.log
  ...
register: myinstaller_out
```
Would save the MSI log to `C:\Windows\Temp\InstallerLogs\myinstaller.log`.

May `log_path` be present or not, `win_package` would still return the content of the log file in `myinstaller_out.log` even on success.

##### ACTUAL RESULTS
N/A

Hints from issue discussion:
Just for context: Some admins at work like to keep installation logs around as artifacts just-in-case. I understand this can be done by other means (e.g. `win_shell`, `win_command`, etc...) but it seems to be this could be added to `win_package` and benefit others. It's sad to have to replicate `win_package` functionality using multiple tasks (or building your own role) when this could be done easily directly in the module.

My 2 cents of course. There might be reasons why this was decided otherwise.
cc @dagwieers @jborean93 @jhawkesworth @nitzmahone @trondhindenes
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
though documentation says for arguments parameter "do not supply ... /log .." it does function as expected 
`win_package:

  path: C:\path\to\myinstaller.msi

  arguments: "/log C:\Windows\Temp\InstallerLogs\myinstaller.log"`

Commit message: Added support for user-specified log file in win_package - feature (#38353)

Patch to review:
diff --git a/lib/ansible/modules/windows/win_package.ps1 b/lib/ansible/modules/windows/win_package.ps1
index bc8f54c512a4b4..2256cabc7852a1 100644
--- a/lib/ansible/modules/windows/win_package.ps1
+++ b/lib/ansible/modules/windows/win_package.ps1
@@ -25,6 +25,7 @@ $validate_certs = Get-AnsibleParam -obj $params -name "validate_certs" -type "bo
 $creates_path = Get-AnsibleParam -obj $params -name "creates_path" -type "path"
 $creates_version = Get-AnsibleParam -obj $params -name "creates_version" -type "str"
 $creates_service = Get-AnsibleParam -obj $params -name "creates_service" -type "str"
+$log_path = Get-AnsibleParam -obj $params -name "log_path" -type "path"
 
 $result = @{
     changed = $false
@@ -326,10 +327,12 @@ if ($state -eq "absent") {
 
             if ($program_metadata.msi -eq $true) {
                 # we are uninstalling an msi
-                $temp_path = [System.IO.Path]::GetTempPath()
-                $log_file = [System.IO.Path]::GetRandomFileName()
-                $log_path = Join-Path -Path $temp_path -ChildPath $log_file
-                $cleanup_artifacts += $log_path
+                if ( -Not $log_path ) { 
+                  $temp_path = [System.IO.Path]::GetTempPath()
+                  $log_file = [System.IO.Path]::GetRandomFileName()
+                  $log_path = Join-Path -Path $temp_path -ChildPath $log_file
+                  $cleanup_artifacts += $log_path
+                }
 
                 if ($program_metadata.product_id -ne $null) {
                     $id = $program_metadata.product_id
@@ -418,11 +421,13 @@ if ($state -eq "absent") {
 
             if ($program_metadata.msi -eq $true) {
                 # we are installing an msi
-                $temp_path = [System.IO.Path]::GetTempPath()
-                $log_file = [System.IO.Path]::GetRandomFileName()
-                $log_path = Join-Path -Path $temp_path -ChildPath $log_file
+                if ( -Not $log_path ) { 
+                  $temp_path = [System.IO.Path]::GetTempPath()
+                  $log_file = [System.IO.Path]::GetRandomFileName()
+                  $log_path = Join-Path -Path $temp_path -ChildPath $log_file
+                  $cleanup_artifacts += $log_path
+                }
 
-                $cleanup_artifacts += $log_path
                 $install_arguments = @("$env:windir\system32\msiexec.exe", "/i", $local_path, "/L*V", $log_path, "/qn", "/norestart")
             } else {
                 $log_path = $null
diff --git a/lib/ansible/modules/windows/win_package.py b/lib/ansible/modules/windows/win_package.py
index 19db88958605b3..a53151a5f14691 100644
--- a/lib/ansible/modules/windows/win_package.py
+++ b/lib/ansible/modules/windows/win_package.py
@@ -130,6 +130,12 @@
     type: bool
     default: yes
     version_added: '2.4'
+  log_path:
+    description:
+    - Full path of log file.
+    - This file will be kept regardless of overall success or failure
+    type: path
+    version_added: '2.8'
 notes:
 - When C(state=absent) and the product is an exe, the path may be different
   from what was used to install the package originally. If path is not set then
@@ -163,7 +169,7 @@
     product_id: '{CF2BEA3C-26EA-32F8-AA9B-331F7E34BA97}'
     arguments: /install /passive /norestart
 
-- name: Install Visual C thingy with list of arguments instead of a string
+- name: Install Visual C thingy with list of arguments instead of a string, and permanent log
   win_package:
     path: http://download.microsoft.com/download/1/6/B/16B06F60-3B20-4FF2-B699-5E9B7962F9AE/VSU_4/vcredist_x64.exe
     product_id: '{CF2BEA3C-26EA-32F8-AA9B-331F7E34BA97}'
@@ -171,6 +177,7 @@
     - /install
     - /passive
     - /norestart
+    log_path: "D:\\logs\\vcredist_x64-exe-{{lookup('pipe', 'date +%Y%m%dT%H%M%S')}}.log"
 
 - name: Install Remote Desktop Connection Manager from msi
   win_package:


Code review comments:
diff_hunk: @@ -130,6 +130,12 @@
     type: bool
     default: yes
     version_added: '2.4'
+  log_path:
+    description:
+    - Full path of log file.
+    - This file will be kept regardless of overall success or failure
path: lib/ansible/modules/windows/win_package.py
text: @user1:
I will add an extra point here saying this is only valid for MSI files, use `arguments` for other packages or something like that.

@author:
ok

diff_hunk: @@ -163,14 +169,15 @@
     product_id: '{CF2BEA3C-26EA-32F8-AA9B-331F7E34BA97}'
     arguments: /install /passive /norestart
 
-- name: Install Visual C thingy with list of arguments instead of a string
+- name: Install Visual C thingy with list of arguments instead of a string, and permanent log
   win_package:
     path: http://download.microsoft.com/download/1/6/B/16B06F60-3B20-4FF2-B699-5E9B7962F9AE/VSU_4/vcredist_x64.exe
     product_id: '{CF2BEA3C-26EA-32F8-AA9B-331F7E34BA97}'
     arguments:
     - /install
     - /passive
     - /norestart
+    log_path: "D:\\logs\\vcredist_x64-exe-{{lookup('pipe', 'date +%Y%m%dT%H%M%S')}}.log"
path: lib/ansible/modules/windows/win_package.py
text: @user1:
It's best to show examples where we don't have to quote, or use single quotes. This could just be

`log_path: D:\logs\vcredist_x64-exe-{{ lookup('pip', 'date +%Y%m%dT%H%M%S') }}.log`

@author:
right