From 0815f935ca359537c023efadf286671e31dc3374 Mon Sep 17 00:00:00 2001
From: Rick Elrod <rick@elrod.me>
Date: Wed, 1 Feb 2023 15:37:08 -0600
Subject: [PATCH] [collection] remove module defaults where API defaults are
 the same (#13037)

Providing defaults for API parameters where the API already provides
defaults leads to some confusing scenarios, because we end up always
sending those collection-defaulted fields in the request even if the
field isn't provided by the user.

For example, we previously set the `scm_type` default to 'manual' and
someone using the collection to update a project who does not explicitly
include the `scm_type` every time they call the module, would
inadvertently change the `scm_type` of the project back to 'manual'
which is surprising behavior.

This change removes the collection defaults for API parameters, unless
they differed from the API default. We let the API handle the defaults
or otherwise ignore fields not given by the user so that the user does
not end up changing unexpected fields when they use a module.

Signed-off-by: Rick Elrod <rick@elrod.me>
---
 .../plugins/modules/ad_hoc_command.py         |  2 +-
 .../plugins/modules/execution_environment.py  |  1 +
 awx_collection/plugins/modules/host.py        |  3 +-
 .../plugins/modules/instance_group.py         | 17 ++----
 awx_collection/plugins/modules/inventory.py   |  3 +-
 .../modules/inventory_source_update.py        |  2 +-
 awx_collection/plugins/modules/job_launch.py  |  6 +-
 .../plugins/modules/job_template.py           | 11 ++--
 .../plugins/modules/organization.py           |  3 +-
 awx_collection/plugins/modules/project.py     | 58 ++++++++-----------
 .../plugins/modules/project_update.py         |  2 +-
 .../plugins/modules/subscriptions.py          |  2 +-
 awx_collection/plugins/modules/token.py       |  3 +-
 awx_collection/plugins/modules/user.py        |  6 +-
 .../plugins/modules/workflow_launch.py        |  2 +-
 .../plugins/modules/workflow_node_wait.py     |  4 +-
 awx_collection/test/awx/test_project.py       |  2 +-
 17 files changed, 53 insertions(+), 74 deletions(-)

diff --git a/awx_collection/plugins/modules/ad_hoc_command.py b/awx_collection/plugins/modules/ad_hoc_command.py
index d2446e5804..bfe22eb890 100644
--- a/awx_collection/plugins/modules/ad_hoc_command.py
+++ b/awx_collection/plugins/modules/ad_hoc_command.py
@@ -129,7 +129,7 @@ def main():
         diff_mode=dict(type='bool'),
         wait=dict(default=False, type='bool'),
         interval=dict(default=2.0, type='float'),
-        timeout=dict(default=None, type='int'),
+        timeout=dict(type='int'),
         execution_environment=dict(),
     )
 
diff --git a/awx_collection/plugins/modules/execution_environment.py b/awx_collection/plugins/modules/execution_environment.py
index 8401944693..552c805720 100644
--- a/awx_collection/plugins/modules/execution_environment.py
+++ b/awx_collection/plugins/modules/execution_environment.py
@@ -84,6 +84,7 @@ def main():
         organization=dict(),
         credential=dict(),
         state=dict(choices=['present', 'absent'], default='present'),
+        # NOTE: Default for pull differs from API (which is blank by default)
         pull=dict(choices=['always', 'missing', 'never'], default='missing'),
     )
 
diff --git a/awx_collection/plugins/modules/host.py b/awx_collection/plugins/modules/host.py
index 8e1490f423..21d063f39e 100644
--- a/awx_collection/plugins/modules/host.py
+++ b/awx_collection/plugins/modules/host.py
@@ -43,7 +43,6 @@ options:
       description:
         - If the host should be enabled.
       type: bool
-      default: 'yes'
     variables:
       description:
         - Variables to use for the host.
@@ -82,7 +81,7 @@ def main():
         new_name=dict(),
         description=dict(),
         inventory=dict(required=True),
-        enabled=dict(type='bool', default=True),
+        enabled=dict(type='bool'),
         variables=dict(type='dict'),
         state=dict(choices=['present', 'absent'], default='present'),
     )
diff --git a/awx_collection/plugins/modules/instance_group.py b/awx_collection/plugins/modules/instance_group.py
index a3debed8ea..dc993f8b5b 100644
--- a/awx_collection/plugins/modules/instance_group.py
+++ b/awx_collection/plugins/modules/instance_group.py
@@ -41,31 +41,26 @@ options:
         - Signifies that this InstanceGroup should act as a ContainerGroup. If no credential is specified, the underlying Pod's ServiceAccount will be used.
       required: False
       type: bool
-      default: False
     policy_instance_percentage:
       description:
         - Minimum percentage of all instances that will be automatically assigned to this group when new instances come online.
       required: False
       type: int
-      default: '0'
     policy_instance_minimum:
       description:
         - Static minimum number of Instances that will be automatically assign to this group when new instances come online.
       required: False
       type: int
-      default: '0'
     max_concurrent_jobs:
       description:
         - Maximum number of concurrent jobs to run on this group. Zero means no limit.
       required: False
       type: int
-      default: '0'
     max_forks:
       description:
         - Max forks to execute on this group. Zero means no limit.
       required: False
       type: int
-      default: '0'
     policy_instance_list:
       description:
         - List of exact-match Instances that will be assigned to this group
@@ -104,14 +99,14 @@ def main():
         name=dict(required=True),
         new_name=dict(),
         credential=dict(),
-        is_container_group=dict(type='bool', default=False),
-        policy_instance_percentage=dict(type='int', default='0'),
-        policy_instance_minimum=dict(type='int', default='0'),
-        max_concurrent_jobs=dict(type='int', default='0'),
-        max_forks=dict(type='int', default='0'),
+        is_container_group=dict(type='bool'),
+        policy_instance_percentage=dict(type='int'),
+        policy_instance_minimum=dict(type='int'),
+        max_concurrent_jobs=dict(type='int'),
+        max_forks=dict(type='int'),
         policy_instance_list=dict(type='list', elements='str'),
         pod_spec_override=dict(),
-        instances=dict(required=False, type="list", elements='str', default=None),
+        instances=dict(required=False, type="list", elements='str'),
         state=dict(choices=['present', 'absent'], default='present'),
     )
 
diff --git a/awx_collection/plugins/modules/inventory.py b/awx_collection/plugins/modules/inventory.py
index b4784f5260..8e739b2211 100644
--- a/awx_collection/plugins/modules/inventory.py
+++ b/awx_collection/plugins/modules/inventory.py
@@ -54,7 +54,6 @@ options:
     kind:
       description:
         - The kind field. Cannot be modified after created.
-      default: ""
       choices: ["", "smart"]
       type: str
     host_filter:
@@ -112,7 +111,7 @@ def main():
         description=dict(),
         organization=dict(required=True),
         variables=dict(type='dict'),
-        kind=dict(choices=['', 'smart'], default=''),
+        kind=dict(choices=['', 'smart']),
         host_filter=dict(),
         instance_groups=dict(type="list", elements='str'),
         prevent_instance_group_fallback=dict(type='bool'),
diff --git a/awx_collection/plugins/modules/inventory_source_update.py b/awx_collection/plugins/modules/inventory_source_update.py
index 4767d6ea3b..5bd6cdfefc 100644
--- a/awx_collection/plugins/modules/inventory_source_update.py
+++ b/awx_collection/plugins/modules/inventory_source_update.py
@@ -94,7 +94,7 @@ def main():
         organization=dict(),
         wait=dict(default=False, type='bool'),
         interval=dict(default=2.0, type='float'),
-        timeout=dict(default=None, type='int'),
+        timeout=dict(type='int'),
     )
 
     # Create a module for ourselves
diff --git a/awx_collection/plugins/modules/job_launch.py b/awx_collection/plugins/modules/job_launch.py
index fc60aea995..9a76f3a8b7 100644
--- a/awx_collection/plugins/modules/job_launch.py
+++ b/awx_collection/plugins/modules/job_launch.py
@@ -180,10 +180,10 @@ def main():
     argument_spec = dict(
         name=dict(required=True, aliases=['job_template']),
         job_type=dict(choices=['run', 'check']),
-        inventory=dict(default=None),
+        inventory=dict(),
         organization=dict(),
         # Credentials will be a str instead of a list for backwards compatability
-        credentials=dict(type='list', default=None, aliases=['credential'], elements='str'),
+        credentials=dict(type='list', aliases=['credential'], elements='str'),
         limit=dict(),
         tags=dict(type='list', elements='str'),
         extra_vars=dict(type='dict'),
@@ -200,7 +200,7 @@ def main():
         job_timeout=dict(type='int'),
         wait=dict(default=False, type='bool'),
         interval=dict(default=2.0, type='float'),
-        timeout=dict(default=None, type='int'),
+        timeout=dict(type='int'),
     )
 
     # Create a module for ourselves
diff --git a/awx_collection/plugins/modules/job_template.py b/awx_collection/plugins/modules/job_template.py
index 98c35f52f2..4508bc18d5 100644
--- a/awx_collection/plugins/modules/job_template.py
+++ b/awx_collection/plugins/modules/job_template.py
@@ -109,7 +109,6 @@ options:
       description:
         - Control the output level Ansible produces as the playbook runs. 0 - Normal, 1 - Verbose, 2 - More Verbose, 3 - Debug, 4 - Connection Debug.
       choices: [0, 1, 2, 3, 4]
-      default: 0
       type: int
     extra_vars:
       description:
@@ -123,7 +122,6 @@ options:
       description:
         - Enable forcing playbook handlers to run even if a task fails.
       type: bool
-      default: 'no'
       aliases:
         - force_handlers_enabled
     skip_tags:
@@ -271,7 +269,6 @@ options:
       description:
         - The number of jobs to slice into at runtime. Will cause the Job Template to launch a workflow if value is greater than 1.
       type: int
-      default: '1'
     webhook_service:
       description:
         - Service that webhook requests will be accepted from
@@ -407,13 +404,13 @@ def main():
         instance_groups=dict(type="list", elements='str'),
         forks=dict(type='int'),
         limit=dict(),
-        verbosity=dict(type='int', choices=[0, 1, 2, 3, 4], default=0),
+        verbosity=dict(type='int', choices=[0, 1, 2, 3, 4]),
         extra_vars=dict(type='dict'),
         job_tags=dict(),
-        force_handlers=dict(type='bool', default=False, aliases=['force_handlers_enabled']),
+        force_handlers=dict(type='bool', aliases=['force_handlers_enabled']),
         skip_tags=dict(),
         start_at_task=dict(),
-        timeout=dict(type='int', default=0),
+        timeout=dict(type='int'),
         use_fact_cache=dict(type='bool', aliases=['fact_caching_enabled']),
         host_config_key=dict(no_log=False),
         ask_diff_mode_on_launch=dict(type='bool', aliases=['ask_diff_mode']),
@@ -438,7 +435,7 @@ def main():
         allow_simultaneous=dict(type='bool', aliases=['concurrent_jobs_enabled']),
         scm_branch=dict(),
         ask_scm_branch_on_launch=dict(type='bool'),
-        job_slice_count=dict(type='int', default='1'),
+        job_slice_count=dict(type='int'),
         webhook_service=dict(choices=['github', 'gitlab', '']),
         webhook_credential=dict(),
         labels=dict(type="list", elements='str'),
diff --git a/awx_collection/plugins/modules/organization.py b/awx_collection/plugins/modules/organization.py
index 99bff3d472..de78eb228b 100644
--- a/awx_collection/plugins/modules/organization.py
+++ b/awx_collection/plugins/modules/organization.py
@@ -47,7 +47,6 @@ options:
     max_hosts:
       description:
         - The max hosts allowed in this organizations
-      default: "0"
       type: int
     state:
       description:
@@ -124,7 +123,7 @@ def main():
         description=dict(),
         default_environment=dict(),
         custom_virtualenv=dict(),
-        max_hosts=dict(type='int', default="0"),
+        max_hosts=dict(type='int'),
         instance_groups=dict(type="list", elements='str'),
         notification_templates_started=dict(type="list", elements='str'),
         notification_templates_success=dict(type="list", elements='str'),
diff --git a/awx_collection/plugins/modules/project.py b/awx_collection/plugins/modules/project.py
index c224f309f0..f67d774aef 100644
--- a/awx_collection/plugins/modules/project.py
+++ b/awx_collection/plugins/modules/project.py
@@ -46,7 +46,6 @@ options:
       description:
         - Type of SCM resource.
       choices: ["manual", "git", "svn", "insights", "archive"]
-      default: "manual"
       type: str
     scm_url:
       description:
@@ -74,28 +73,23 @@ options:
       description:
         - Remove local modifications before updating.
       type: bool
-      default: 'no'
     scm_delete_on_update:
       description:
         - Remove the repository completely before updating.
       type: bool
-      default: 'no'
     scm_track_submodules:
       description:
         - Track submodules latest commit on specified branch.
       type: bool
-      default: 'no'
     scm_update_on_launch:
       description:
         - Before an update to the local repository before launching a job with this project.
       type: bool
-      default: 'no'
     scm_update_cache_timeout:
       description:
         - Cache Timeout to cache prior project syncs for a certain number of seconds.
             Only valid if scm_update_on_launch is to True, otherwise ignored.
       type: int
-      default: 0
     allow_override:
       description:
         - Allow changing the SCM branch or revision in a job template that uses this project.
@@ -107,7 +101,6 @@ options:
         - The amount of time (in seconds) to run before the SCM Update is canceled. A value of 0 means no timeout.
         - If waiting for the project to update this will abort after this
           amount of seconds
-      default: 0
       type: int
       aliases:
         - job_timeout
@@ -260,19 +253,19 @@ def main():
         new_name=dict(),
         copy_from=dict(),
         description=dict(),
-        scm_type=dict(choices=['manual', 'git', 'svn', 'insights', 'archive'], default='manual'),
+        scm_type=dict(choices=['manual', 'git', 'svn', 'insights', 'archive']),
         scm_url=dict(),
         local_path=dict(),
         scm_branch=dict(),
         scm_refspec=dict(),
         credential=dict(aliases=['scm_credential']),
-        scm_clean=dict(type='bool', default=False),
-        scm_delete_on_update=dict(type='bool', default=False),
-        scm_track_submodules=dict(type='bool', default=False),
-        scm_update_on_launch=dict(type='bool', default=False),
-        scm_update_cache_timeout=dict(type='int', default=0),
+        scm_clean=dict(type='bool'),
+        scm_delete_on_update=dict(type='bool'),
+        scm_track_submodules=dict(type='bool'),
+        scm_update_on_launch=dict(type='bool'),
+        scm_update_cache_timeout=dict(type='int'),
         allow_override=dict(type='bool', aliases=['scm_allow_override']),
-        timeout=dict(type='int', default=0, aliases=['job_timeout']),
+        timeout=dict(type='int', aliases=['job_timeout']),
         default_environment=dict(),
         custom_virtualenv=dict(),
         organization=dict(),
@@ -336,12 +329,6 @@ def main():
         # If the state was absent we can let the module delete it if needed, the module will handle exiting from this
         module.delete_if_needed(project)
 
-    if credential is not None:
-        credential = module.resolve_name_to_id('credentials', credential)
-
-    if signature_validation_credential is not None:
-        signature_validation_credential = module.resolve_name_to_id('credentials', signature_validation_credential)
-
     # Attempt to look up associated field items the user specified.
     association_fields = {}
 
@@ -366,20 +353,18 @@ def main():
     # Create the data that gets sent for create and update
     project_fields = {
         'name': new_name if new_name else (module.get_item_name(project) if project else name),
-        'scm_type': scm_type,
-        'organization': org_id,
-        'scm_update_on_launch': scm_update_on_launch,
-        'scm_update_cache_timeout': scm_update_cache_timeout,
-        'signature_validation_credential': signature_validation_credential,
     }
 
     for field_name in (
+        'scm_type',
         'scm_url',
         'scm_branch',
         'scm_refspec',
         'scm_clean',
         'scm_delete_on_update',
-        "scm_track_submodules",
+        'scm_track_submodules',
+        'scm_update_on_launch',
+        'scm_update_cache_timeout',
         'timeout',
         'scm_update_cache_timeout',
         'custom_virtualenv',
@@ -390,15 +375,22 @@ def main():
         if field_val is not None:
             project_fields[field_name] = field_val
 
-    if credential is not None:
-        project_fields['credential'] = credential
-    if default_ee is not None:
-        project_fields['default_environment'] = module.resolve_name_to_id('execution_environments', default_ee)
-    if scm_type == '':
-        if local_path is not None:
+    for variable, field, endpoint in (
+        (default_ee, 'default_environment', 'execution_environments'),
+        (credential, 'credential', 'credentials'),
+        (signature_validation_credential, 'signature_validation_credential', 'credentials'),
+    ):
+        if variable is not None:
+            project_fields[field] = module.resolve_name_to_id(endpoint, variable)
+
+    if org_id is not None:
+        # this is resolved earlier, so save an API call and don't do it again in the loop above
+        project_fields['organization'] = org_id
+
+    if scm_type == '' and local_path is not None:
             project_fields['local_path'] = local_path
 
-    if scm_update_cache_timeout != 0 and scm_update_on_launch is not True:
+    if scm_update_cache_timeout not in (0, None) and scm_update_on_launch is not True:
         module.warn('scm_update_cache_timeout will be ignored since scm_update_on_launch was not set to true')
 
     # If we are doing a not manual project, register our on_change method
diff --git a/awx_collection/plugins/modules/project_update.py b/awx_collection/plugins/modules/project_update.py
index 38b55c8e3c..6cbcd39b6d 100644
--- a/awx_collection/plugins/modules/project_update.py
+++ b/awx_collection/plugins/modules/project_update.py
@@ -87,7 +87,7 @@ def main():
         organization=dict(),
         wait=dict(default=True, type='bool'),
         interval=dict(default=2.0, type='float'),
-        timeout=dict(default=None, type='int'),
+        timeout=dict(type='int'),
     )
 
     # Create a module for ourselves
diff --git a/awx_collection/plugins/modules/subscriptions.py b/awx_collection/plugins/modules/subscriptions.py
index 146ef7c451..0f89e71ded 100644
--- a/awx_collection/plugins/modules/subscriptions.py
+++ b/awx_collection/plugins/modules/subscriptions.py
@@ -17,7 +17,7 @@ module: subscriptions
 author: "John Westcott IV (@john-westcott-iv)"
 short_description: Get subscription list
 description:
-    - Get subscriptions available to  Automation Platform Controller. See
+    - Get subscriptions available to Automation Platform Controller. See
       U(https://www.ansible.com/tower) for an overview.
 options:
     username:
diff --git a/awx_collection/plugins/modules/token.py b/awx_collection/plugins/modules/token.py
index c0089240f4..c9ed84f67e 100644
--- a/awx_collection/plugins/modules/token.py
+++ b/awx_collection/plugins/modules/token.py
@@ -45,7 +45,6 @@ options:
         - Allowed scopes, further restricts user's permissions. Must be a simple space-separated string with allowed scopes ['read', 'write'].
       required: False
       type: str
-      default: 'write'
       choices: ["read", "write"]
     existing_token:
       description: The data structure produced from token in create mode to be used with state absent.
@@ -135,7 +134,7 @@ def main():
     argument_spec = dict(
         description=dict(),
         application=dict(),
-        scope=dict(choices=['read', 'write'], default='write'),
+        scope=dict(choices=['read', 'write']),
         existing_token=dict(type='dict', no_log=False),
         existing_token_id=dict(),
         state=dict(choices=['present', 'absent'], default='present'),
diff --git a/awx_collection/plugins/modules/user.py b/awx_collection/plugins/modules/user.py
index 3d5f76bb02..49a6f216a6 100644
--- a/awx_collection/plugins/modules/user.py
+++ b/awx_collection/plugins/modules/user.py
@@ -50,13 +50,11 @@ options:
       description:
         - Designates that this user has all permissions without explicitly assigning them.
       type: bool
-      default: False
       aliases: ['superuser']
     is_system_auditor:
       description:
         - User is a system wide auditor.
       type: bool
-      default: False
       aliases: ['auditor']
     password:
       description:
@@ -134,8 +132,8 @@ def main():
         first_name=dict(),
         last_name=dict(),
         email=dict(),
-        is_superuser=dict(type='bool', default=False, aliases=['superuser']),
-        is_system_auditor=dict(type='bool', default=False, aliases=['auditor']),
+        is_superuser=dict(type='bool', aliases=['superuser']),
+        is_system_auditor=dict(type='bool', aliases=['auditor']),
         password=dict(no_log=True),
         update_secrets=dict(type='bool', default=True, no_log=False),
         organization=dict(),
diff --git a/awx_collection/plugins/modules/workflow_launch.py b/awx_collection/plugins/modules/workflow_launch.py
index b3e5cb9bf6..1613e4fa8b 100644
--- a/awx_collection/plugins/modules/workflow_launch.py
+++ b/awx_collection/plugins/modules/workflow_launch.py
@@ -105,7 +105,7 @@ def main():
         extra_vars=dict(type='dict'),
         wait=dict(required=False, default=True, type='bool'),
         interval=dict(required=False, default=2.0, type='float'),
-        timeout=dict(required=False, default=None, type='int'),
+        timeout=dict(required=False, type='int'),
     )
 
     # Create a module for ourselves
diff --git a/awx_collection/plugins/modules/workflow_node_wait.py b/awx_collection/plugins/modules/workflow_node_wait.py
index 2a744c7526..e82602904e 100644
--- a/awx_collection/plugins/modules/workflow_node_wait.py
+++ b/awx_collection/plugins/modules/workflow_node_wait.py
@@ -20,7 +20,7 @@ DOCUMENTATION = """
 ---
 module: workflow_node_wait
 author: "Sean Sullivan (@sean-m-sullivan)"
-short_description: Approve an approval node in a workflow job.
+short_description: Wait for a workflow node to finish.
 description:
     - Approve an approval node in a workflow job. See
       U(https://www.ansible.com/tower) for an overview.
@@ -43,7 +43,7 @@ options:
       type: float
     timeout:
       description:
-        - Maximum time in seconds to wait for a workflow job to to reach approval node.
+        - Maximum time in seconds to wait for a workflow job to reach approval node.
       default: 10
       type: int
 extends_documentation_fragment: awx.awx.auth
diff --git a/awx_collection/test/awx/test_project.py b/awx_collection/test/awx/test_project.py
index 8ec3e8816e..2f8724566a 100644
--- a/awx_collection/test/awx/test_project.py
+++ b/awx_collection/test/awx/test_project.py
@@ -28,7 +28,7 @@ def test_create_project(run_module, admin_user, organization, silence_warning):
 
 @pytest.mark.django_db
 def test_create_project_copy_from(run_module, admin_user, organization, silence_warning):
-    ''' Test the copy_from functionality'''
+    '''Test the copy_from functionality'''
     result = run_module(
         'project',
         dict(name='foo', organization=organization.name, scm_type='git', scm_url='https://foo.invalid', wait=False, scm_update_cache_timeout=5),