win_environment: Make this the Windows reference module

As discussed before we selected win_environment to the documentation,
and point to win_uri for a more advanced module.

If we want to make this the reference module, we have to get this one
absolutely right in every possible way.

This PR cleans up both win_environment and win_uri, and makes the
required changes to the windows module development section.

docs/docsite/rst/dev_guide/developing_modules_general_windows.rst

@@ -38,52 +38,17 @@ When creating a new module there are a few things to keep in mind:
 - Use the full cmdlet name instead of aliases, e.g. ``Remove-Item`` over ``rm``
 - Use named parameters with cmdlets, e.g. ``Remove-Item -Path C:\temp`` over ``Remove-Item C:\temp``
 
-A very basic powershell module template can be found found below:
+A very basic powershell module `win_environment <https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_environment.ps1>`_ is included below. It demonstrates how to implement check-mode and diff-support, and also shows a warning to the user when a specific condition is met.
 
-.. code-block:: powershell
+.. .. include:: ../../../../lib/ansible/modules/windows/win_environment.ps1
+..    :code: powershell
 
-    #!powershell
+.. literalinclude:: ../../../../lib/ansible/modules/windows/win_environment.ps1
-    # This file is part of Ansible
+   :language: powershell
 
-    # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+A slightly more advanced module is `win_uri <https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_uri.ps1>`_ which additionally shows how to use different parameter types (bool, str, int, list, dict, path) and a selection of choices for parameters, how to fail a module and how to handle exceptions.
 
-    #Requires -Module Ansible.ModuleUtils.Legacy.psm1
+When in doubt, look at some of the other core modules and see how things have been
-
-    $ErrorActionPreference = 'Stop'
-
-    $params = Parse-Args -arguments $args -supports_check_mode $true
-    $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false
-    $diff_mode = Get-AnsibleParam -obj $params -name "_ansible_diff" -type "bool" -default $false
-
-    # these are your module parameters, there are various types which can be
-    # used to format your parameters. You can also set mandatory parameters
-    # with -failifempty, set defaults with -default and set choices with
-    # -validateset.
-    $string = Get-AnsibleParam -obj $params -name "string" -type "str" -failifempty $true
-    $bool = Get-AnsibleParam -obj $params -name "bool" -type "bool" -default $false
-    $int = Get-AnsibleParam -obj $params -name "int" -type "int"
-    $path = Get-AnsibleParam -obj $params -name "path" -type "path"
-    $list = Get-AnsibleParam -obj $params -name "list" -type "list"
-    $choices = Get-AnsibleParam -obj $params -name "choices" -type "str" -default "present" -validateset "absent","present"
-
-    $result = @{
-        changed = $false
-    }
-
-    if ($diff_mode) {
-        $result.diff = @{}
-    }
-
-    # code goes here
-
-    # you can add/set new result objects with
-    $result.changed = $true
-    $result.new_result = "Hi"
-
-    Exit-Json -obj $result
-
-
-When in doubt, look at some of the core modules and see how things have been
 implemented there.
 
 Sometimes there are multiple ways that Windows offers to complete a task; this

lib/ansible/modules/windows/win_environment.ps1

@@ -1,86 +1,67 @@
 #!powershell
-# This file is part of Ansible
-#
-# Copyright 2015, Jon Hawkesworth (@jhawkesworth) <figs@unity.demon.co.uk>
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
-# WANT_JSON
+# Copyright: (c) 2015, Jon Hawkesworth (@jhawkesworth) <figs@unity.demon.co.uk>
-# POWERSHELL_COMMON
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
-$params = Parse-Args $args -supports_check_mode $true
+#Requires -Module Ansible.ModuleUtils.Legacy.psm1
+
+$ErrorActionPreference = "Stop"
+
+$params = Parse-Args -arguments $args -supports_check_mode $true
 $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false
-$diff_support = Get-AnsibleParam -obj $params -name "_ansible_diff" -type "bool" -default $false
+$diff_mode = Get-AnsibleParam -obj $params -name "_ansible_diff" -type "bool" -default $false
 
 $name = Get-AnsibleParam -obj $params -name "name" -type "str" -failifempty $true
-$state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "present" -validateset "present","absent"
+$state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "present" -validateset "absent","present"
 $value = Get-AnsibleParam -obj $params -name "value" -type "str"
 $level = Get-AnsibleParam -obj $params -name "level" -type "str" -validateSet "machine","user","process" -failifempty $true
 
 $before_value = [Environment]::GetEnvironmentVariable($name, $level)
 
-# When removing environment, set value to $null if set
-if ($state -eq "absent" -and $value) {
-    $value = $null
-}
-
 $result = @{
     before_value = $before_value
     changed = $false
-    level = $level
-    name = $name
     value = $value
 }
 
-if ($diff_support) {
+# When removing environment, set value to $null if set
-    $result.diff = @{}
+if ($state -eq "absent" -and $value) {
+    Add-Warning -obj $result -message "When removing environment variable '$name' it should not have a value '$value' set"
+    $value = $null
 }
 
 if ($state -eq "present" -and $before_value -ne $value) {
+
     if (-not $check_mode) {
         [Environment]::SetEnvironmentVariable($name, $value, $level)
     }
     $result.changed = $true
 
-    if ($diff_support) {
+    if ($diff_mode) {
         if ($before_value -eq $null) {
-            $result.diff.prepared = @"
+            $result.diff = @{
-[$level]
+                prepared = " [$level]`n+$name = $value`n"
-+$NAME = $value
+            }
-"@
         } else {
-            $result.diff.prepared = @"
+            $result.diff = @{
-[$level]
+                prepared = " [$level]`n-$name = $before_value`n+$name = $value`n"
--$NAME = $before_value
+            }
-+$NAME = $value
-"@
         }
     }
 
 } elseif ($state -eq "absent" -and $before_value -ne $null) {
+
     if (-not $check_mode) {
         [Environment]::SetEnvironmentVariable($name, $null, $level)
     }
     $result.changed = $true
 
-    if ($diff_support) {
+    if ($diff_mode) {
-        $result.diff.prepared = @"
+        $result.diff = @{
-[$level]
+            prepared = " [$level]`n-$name = $before_value`n"
--$NAME = $before_value
+        }
-"@
     }
 
 }
 
-Exit-Json $result
+Exit-Json -obj $result

lib/ansible/modules/windows/win_environment.py

@@ -1,81 +1,56 @@
 #!/usr/bin/python
 # -*- coding: utf-8 -*-
 
-# (c) 2015, Jon Hawkesworth (@jhawkesworth) <figs@unity.demon.co.uk>
+# Copyright: (c) 2015, Jon Hawkesworth (@jhawkesworth) <figs@unity.demon.co.uk>
-#
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
-# This file is part of Ansible
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
-
-# this is a windows documentation stub.  actual code lives in the .ps1
-# file of the same name
 
 ANSIBLE_METADATA = {'metadata_version': '1.1',
                     'status': ['preview'],
                     'supported_by': 'community'}
 
-
 DOCUMENTATION = r'''
 ---
 module: win_environment
-version_added: "2.0"
+version_added: '2.0'
-short_description: Modifies environment variables on windows hosts.
+short_description: Modify environment variables on windows hosts
 description:
-    - Uses .net Environment to set or remove environment variables and can set at User, Machine or Process level.
+- Uses .net Environment to set or remove environment variables and can set at User, Machine or Process level.
-    - User level environment variables will be set, but not available until the user has logged off and on again.
+- User level environment variables will be set, but not available until the user has logged off and on again.
 options:
   state:
     description:
-      - present to ensure environment variable is set, or absent to ensure it is removed
+    - Set to C(present) to ensure environment variable is set.
-    required: false
+    - Set to C(absent) to ensure it is removed.
+    choices: [ absent, present ]
     default: present
-    choices:
-      - present
-      - absent
   name:
     description:
-      - The name of the environment variable
+    - The name of the environment variable.
     required: true
-    default: no default
   value:
     description:
-      - The value to store in the environment variable. Can be omitted for state=absent
+    - The value to store in the environment variable.
-    required: false
+    - Can be omitted for C(state=absent).
-    default: no default
   level:
     description:
-      - The level at which to set the environment variable.
+    - The level at which to set the environment variable.
-      - Use 'machine' to set for all users.
+    - Use C(machine) to set for all users.
-      - Use 'user' to set for the current user that ansible is connected as.
+    - Use C(user) to set for the current user that ansible is connected as.
-      - Use 'process' to set for the current process.  Probably not that useful.
+    - Use C(process) to set for the current process.  Probably not that useful.
+    choices: [ machine, user, process ]
     required: true
-    default: no default
+author:
-    choices:
+- Jon Hawkesworth (@jhawkesworth)
-      - machine
-      - process
-      - user
-author: "Jon Hawkesworth (@jhawkesworth)"
 notes:
-   - This module is best-suited for setting the entire value of an
+- This module is best-suited for setting the entire value of an
-     environment variable. For safe element-based management of
+  environment variable. For safe element-based management of
-     path-like environment vars, use the M(win_path) module.
+  path-like environment vars, use the M(win_path) module.
-   - This module does not broadcast change events.
+- This module does not broadcast change events.
-     This means that the minority of windows applications which can have
+  This means that the minority of windows applications which can have
-     their environment changed without restarting will not be notified and
+  their environment changed without restarting will not be notified and
-     therefore will need restarting to pick up new environment settings.
+  therefore will need restarting to pick up new environment settings.
-     User level environment variables will require the user to log out
+  User level environment variables will require the user to log out
-     and in again before they become available.
+  and in again before they become available.
 '''
 
 EXAMPLES = r'''
@@ -95,24 +70,12 @@ EXAMPLES = r'''
 
 RETURN = r'''
 before_value:
-  description:
+  description: the value of the environment key before a change, this is null if it didn't exist
-  - the value of the environment key before a change, this is null if it didn't
-    exist
   returned: always
   type: string
   sample: C:\Windows\System32
-level:
-  description: the level set when calling the module
-  returned: always
-  type: string
-  sample: machine
-name:
-  description: the name of the environment key the module checked
-  returned: always
-  type: string
-  sample: JAVA_HOME
 value:
-  description: the value the environment key has been set to
+  description: the value the environment key has been set to, this is null if removed
   returned: always
   type: string
   sample: C:\Program Files\jdk1.8

lib/ansible/modules/windows/win_uri.ps1

@@ -1,26 +1,13 @@
 #!powershell
-# This file is part of Ansible
-#
-# Copyright 2015, Corwin Brown <corwin@corwinbrown.com>
-# Copyright 2017, Dag Wieers <dag@wieers.com>
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
 
-# WANT_JSON
+# Copyright: (c) 2015, Corwin Brown <corwin@corwinbrown.com>
-# POWERSHELL_COMMON
+# Copyright: (c) 2017, Dag Wieers (@dagwieers) <dag@wieers.com>
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+#Requires -Module Ansible.ModuleUtils.Legacy.psm1
 
 $ErrorActionPreference = "Stop"
+
 $safe_methods = @("GET", "HEAD")
 $content_keys = @("Content", "Images", "InputFields", "Links", "RawContent")
 
@@ -29,7 +16,7 @@ Function ConvertTo-SnakeCase($input_string) {
     return $snake_case.ToLower()
 }
 
-$params = Parse-Args $args -supports_check_mode $true
+$params = Parse-Args -arguments $args -supports_check_mode $true
 $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false
 
 $url = Get-AnsibleParam -obj $params -name "url" -type "str" -failifempty $true
@@ -56,12 +43,12 @@ $client_cert = Get-AnsibleParam -obj $params -name "client_cert" -type "path"
 
 if ($creates -and (Test-Path -Path $creates)) {
     $result.skipped = $true
-    Exit-Json $result "The 'creates' file or directory ($creates) already exists."
+    Exit-Json -obj $result -message "The 'creates' file or directory ($creates) already exists."
 }
 
 if ($removes -and -not (Test-Path -Path $removes)) {
     $result.skipped = $true
-    Exit-Json $result "The 'removes' file or directory ($removes) does not exist."
+    Exit-Json -obj $result -message "The 'removes' file or directory ($removes) does not exist."
 }
 
 $result = @{
@@ -110,7 +97,7 @@ if ($client_cert) {
     Try {
         $webrequest_opts.Certificate = Get-PfxCertificate -FilePath $client_cert
     } Catch {
-        Fail-Json $result "Failed to read client certificate '$client_cert'"
+        Fail-Json -obj $result -message "Failed to read client certificate '$client_cert'"
     }
 }
 
@@ -127,6 +114,8 @@ if ($dest -and -not $check_mode) {
 
 if ($user -and $password) {
     $webrequest_opts.Credential = New-Object System.Management.Automation.PSCredential($user, $($password | ConvertTo-SecureString -AsPlainText -Force))
+} elif ($user -or $password) {
+    Add-Warning -obj $result -message "Both 'user' and 'password' parameters are required together, skipping authentication"
 }
 
 try {
@@ -137,7 +126,6 @@ try {
 
 # TODO: When writing to a file, this is not idempotent !
 # FIXME: Assume a change when we are writing to a file
-# FIXME: Implement diff-mode
 if ($dest) {
     $result.changed = $true
 }
@@ -151,7 +139,7 @@ ForEach ($prop in $response.psobject.properties) {
 }
 
 if ($status_code -notcontains $response.StatusCode) {
-    Fail-Json $result "Status code of request '$($response.StatusCode)' is not in list of valid status codes $status_code."
+    Fail-Json -obj $result -message "Status code of request '$($response.StatusCode)' is not in list of valid status codes $status_code."
 }
 
-Exit-Json $result
+Exit-Json -obj $result

lib/ansible/modules/windows/win_uri.py

@@ -1,26 +1,9 @@
 #!/usr/bin/python
 # -*- coding: utf-8 -*-
 
-# (c) 2015, Corwin Brown <corwin@corwinbrown.com>
+# Copyright: (c) 2015, Corwin Brown <corwin@corwinbrown.com>
-# (c) 2017, Dag Wieers <dag@wieers.com>
+# Copyright: (c) 2017, Dag Wieers (@dagwieers) <dag@wieers.com>
-#
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
-# This file is part of Ansible
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
-
-# this is a windows documentation stub.  actual code lives in the .ps1
-# file of the same name
 
 ANSIBLE_METADATA = {'metadata_version': '1.1',
                     'status': ['preview'],

test/integration/targets/win_environment/tasks/main.yml

@@ -45,9 +45,6 @@
     that:
     - create_machine|changed
     - create_machine.before_value == None
-    - create_machine.level == 'machine'
-    - create_machine.name == test_environment_name
-    - create_machine.value == test_machine_environment_value
     - create_machine_actual.stdout == "{{test_machine_environment_value}}\r\n"
 
 - name: create test environment value for machine again
@@ -67,9 +64,6 @@
     that:
     - not create_machine_again|changed
     - create_machine_again.before_value == test_machine_environment_value
-    - create_machine_again.level == 'machine'
-    - create_machine_again.name == test_environment_name
-    - create_machine_again.value == test_machine_environment_value
     - create_machine_actual_again.stdout == "{{test_machine_environment_value}}\r\n"
 
 - name: change test environment value for machine check
@@ -90,9 +84,6 @@
     that:
     - change_machine_check|changed
     - change_machine_check.before_value == test_machine_environment_value
-    - change_machine_check.level == 'machine'
-    - change_machine_check.name == test_environment_name
-    - change_machine_check.value == test_new_machine_environment_value
     - change_machine_actual_check.stdout == "{{test_machine_environment_value}}\r\n"
 
 - name: change test environment value for machine
@@ -112,9 +103,6 @@
     that:
     - change_machine|changed
     - change_machine.before_value == test_machine_environment_value
-    - change_machine.level == 'machine'
-    - change_machine.name == test_environment_name
-    - change_machine.value == test_new_machine_environment_value
     - change_machine_actual.stdout == "{{test_new_machine_environment_value}}\r\n"
 
 - name: change test environment value for machine again
@@ -134,9 +122,6 @@
     that:
     - not change_machine_again|changed
     - change_machine_again.before_value == test_new_machine_environment_value
-    - change_machine_again.level == 'machine'
-    - change_machine_again.name == test_environment_name
-    - change_machine_again.value == test_new_machine_environment_value
     - change_machine_actual_again.stdout == "{{test_new_machine_environment_value}}\r\n"
 
 - name: create test environment value for user check
@@ -175,9 +160,6 @@
     that:
     - create_user|changed
     - create_user.before_value == None
-    - create_user.level == 'user'
-    - create_user.name == test_environment_name
-    - create_user.value == test_user_environment_value
     - create_user_actual.stdout == "{{test_user_environment_value}}\r\n"
 
 - name: create test environment value for user again
@@ -197,9 +179,6 @@
     that:
     - not create_user_again|changed
     - create_user_again.before_value == test_user_environment_value
-    - create_user_again.level == 'user'
-    - create_user_again.name == test_environment_name
-    - create_user_again.value == test_user_environment_value
     - create_user_actual_again.stdout == "{{test_user_environment_value}}\r\n"
 
 - name: change test environment value for user check
@@ -220,9 +199,6 @@
     that:
     - change_user_check|changed
     - change_user_check.before_value == test_user_environment_value
-    - change_user_check.level == 'user'
-    - change_user_check.name == test_environment_name
-    - change_user_check.value == test_new_user_environment_value
     - change_user_actual_check.stdout == "{{test_user_environment_value}}\r\n"
 
 - name: change test environment value for user
@@ -242,9 +218,6 @@
     that:
     - change_user|changed
     - change_user.before_value == test_user_environment_value
-    - change_user.level == 'user'
-    - change_user.name == test_environment_name
-    - change_user.value == test_new_user_environment_value
     - change_user_actual.stdout == "{{test_new_user_environment_value}}\r\n"
 
 - name: change test environment value for user again
@@ -264,9 +237,6 @@
     that:
     - not change_user_again|changed
     - change_user_again.before_value == test_new_user_environment_value
-    - change_user_again.level == 'user'
-    - change_user_again.name == test_environment_name
-    - change_user_again.value == test_new_user_environment_value
     - change_user_actual_again.stdout == "{{test_new_user_environment_value}}\r\n"
 
 - name: cleanup test changes