Repository: ansible/ansible
Pull request: Deprecate alias 'thirsty' from all usages
Fixes: #61236

##### SUMMARY
Deprecate alias 'thirsty' from all usages
Add handling for a `deprecated_aliases` option in `_handle_aliases` to account for mod_utils arg spec.

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/modules/files/copy.py
lib/ansible/modules/files/iso_extract.py
lib/ansible/modules/net_tools/basics/uri.py
lib/ansible/modules/net_tools/basics/get_url.py
lib/ansible/module_utils/urls.py
lib/ansible/module_utils/basic.py
lib/ansible/plugins/doc_fragments/url.py

Problem statement:
Consider removing `thirsty` alias from the copy module (only the copy module)
<!--- Verify first that your feature was not already discussed on GitHub -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
When I read the `copy` documentation, I couldn't work out what it meant.  Unfortunately, this meant it sounded like it might be a weird sex joke.

It doesn't sound like a big problem, but in an ideal world, I think we should prefer to avoid weird sex jokes in an alias for a parameter called `force`.

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
copy

##### ADDITIONAL INFORMATION
Issue search tells me "thirsty" was added as an option to the `get_url` module.  In that context, it is appropriate.  But in the context of the `copy` module, it does not make sense, because `copy` only works in the upload direction.

The first three pages of Google Search results (Ansible thirsty) show people using "thirsty" for downloads only, not uploads.  Similarly for GitHub code search (copy thirsty language:YAML).

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/files/copy.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/files/copy.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 @ptux
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Handle deprecated aliases in mod_utils

Patch to review:
diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py
index 1e11bea4e58207..fd94843a908800 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -1427,6 +1427,12 @@ def _handle_aliases(self, spec=None, param=None, option_prefix=''):
         alias_results, self._legal_inputs = handle_aliases(spec, param, alias_warnings=alias_warnings)
         for option, alias in alias_warnings:
             self._warnings.append('Both option %s and its alias %s are set.' % (option_prefix + option, option_prefix + alias))
+        if 'deprecated_aliases' in spec.keys():
+            if spec['deprecated_aliases']['alias'] in param.keys():
+                self._deprecations.append(
+                    {'msg': "Alias '%s' is deprecated. See the module docs for more information" % spec['deprecated_aliases']['alias'],
+                     'version': spec['deprecated_aliases']['version']})
+
         return alias_results
 
     def _handle_no_log_values(self, spec=None, param=None):
diff --git a/lib/ansible/module_utils/urls.py b/lib/ansible/module_utils/urls.py
index 8574792aa6cabd..447d850fc4990a 100644
--- a/lib/ansible/module_utils/urls.py
+++ b/lib/ansible/module_utils/urls.py
@@ -1418,6 +1418,7 @@ def url_argument_spec():
         force_basic_auth=dict(type='bool', default=False),
         client_cert=dict(type='path'),
         client_key=dict(type='path'),
+        deprecated_aliases=dict(alias='thirsty', version='2.13'),
     )
 
 
diff --git a/lib/ansible/modules/files/copy.py b/lib/ansible/modules/files/copy.py
index 788fec9f99c130..6912439af6f61c 100644
--- a/lib/ansible/modules/files/copy.py
+++ b/lib/ansible/modules/files/copy.py
@@ -60,6 +60,7 @@
     - Influence whether the remote file must always be replaced.
     - If C(yes), the remote file will be replaced when contents are different than the source.
     - If C(no), the file will only be transferred if the destination does not exist.
+    - Alias C(thirsty) has been deprecated and will be removed in 2.13.
     type: bool
     default: yes
     aliases: [ thirsty ]
@@ -510,6 +511,9 @@ def main():
         supports_check_mode=True,
     )
 
+    if module.params.get('thirsty'):
+        module.deprecate('The alias "thirsty" has been deprecated and will be removed, use "force" instead', version='2.13')
+
     src = module.params['src']
     b_src = to_bytes(src, errors='surrogate_or_strict')
     dest = module.params['dest']
diff --git a/lib/ansible/modules/files/iso_extract.py b/lib/ansible/modules/files/iso_extract.py
index ad65a66e9d34ef..f6d24b309d74f5 100644
--- a/lib/ansible/modules/files/iso_extract.py
+++ b/lib/ansible/modules/files/iso_extract.py
@@ -56,6 +56,7 @@
     description:
     - If C(yes), which will replace the remote file when contents are different than the source.
     - If C(no), the file will only be extracted and copied if the destination does not already exist.
+    - Alias C(thirsty) has been deprecated and will be removed in 2.13.
     type: bool
     default: yes
     aliases: [ thirsty ]
@@ -117,6 +118,9 @@ def main():
     force = module.params['force']
     executable = module.params['executable']
 
+    if module.params.get('thirsty'):
+        module.deprecate('The alias "thirsty" has been deprecated and will be removed, use "force" instead', version='2.13')
+
     result = dict(
         changed=False,
         dest=dest,
diff --git a/lib/ansible/modules/net_tools/basics/get_url.py b/lib/ansible/modules/net_tools/basics/get_url.py
index cd64818996efe5..f7696cd73bf462 100644
--- a/lib/ansible/modules/net_tools/basics/get_url.py
+++ b/lib/ansible/modules/net_tools/basics/get_url.py
@@ -61,6 +61,7 @@
         will only be downloaded if the destination does not exist. Generally
         should be C(yes) only for small local files.
       - Prior to 0.6, this module behaved as if C(yes) was the default.
+      - Alias C(thirsty) has been deprecated and will be removed in 2.13.
     type: bool
     default: no
     aliases: [ thirsty ]
@@ -446,6 +447,9 @@ def main():
         mutually_exclusive=[['checksum', 'sha256sum']],
     )
 
+    if module.params.get('thirsty'):
+        module.deprecate('The alias "thirsty" has been deprecated and will be removed, use "force" instead', version='2.13')
+
     url = module.params['url']
     dest = module.params['dest']
     backup = module.params['backup']
diff --git a/lib/ansible/modules/net_tools/basics/uri.py b/lib/ansible/modules/net_tools/basics/uri.py
index eea77d8a858c27..ac5234ddf6bcd6 100644
--- a/lib/ansible/modules/net_tools/basics/uri.py
+++ b/lib/ansible/modules/net_tools/basics/uri.py
@@ -154,6 +154,7 @@
   force:
     description:
       - If C(yes) do not get a cached copy.
+      - Alias C(thirsty) has been deprecated and will be removed in 2.13.
     type: bool
     default: no
     aliases: [ thirsty ]
@@ -574,6 +575,9 @@ def main():
         mutually_exclusive=[['body', 'src']],
     )
 
+    if module.params.get('thirsty'):
+        module.deprecate('The alias "thirsty" has been deprecated and will be removed, use "force" instead', version='2.13')
+
     url = module.params['url']
     body = module.params['body']
     body_format = module.params['body_format'].lower()
diff --git a/lib/ansible/plugins/doc_fragments/url.py b/lib/ansible/plugins/doc_fragments/url.py
index efbf39bd160a24..d6fdcc384d005c 100644
--- a/lib/ansible/plugins/doc_fragments/url.py
+++ b/lib/ansible/plugins/doc_fragments/url.py
@@ -16,6 +16,7 @@ class ModuleDocFragment(object):
   force:
     description:
       - If C(yes) do not get a cached copy.
+      - Alias C(thirsty) has been deprecated and will be removed in 2.13.
     type: bool
     default: no
     aliases: [ thirsty ]


Code review comments:
diff_hunk: @@ -1418,6 +1418,7 @@ def url_argument_spec():
         force_basic_auth=dict(type='bool', default=False),
         client_cert=dict(type='path'),
         client_key=dict(type='path'),
+        deprecated_aliases=dict(alias='thirsty', version='2.13'),
path: lib/ansible/module_utils/urls.py
text: @user1:
I wonder if this makes more sense as a list or something since more than one alias may need deprecation and might need a different deprecation version. If it was just this arg spec it probably wouldn't matter, but since basic.py is handling this other people are going to want to utilize it and it seems like it should be flexible.

@author:
I'm reconsidering if this is the right approach anyway. It needs to go into the docs for the sanity check, which could probably be hand-waved for one case but feels pretty hacky as a general solution.  Need to see if I can come up with a way to make an internal arg or something like that.

@user2:
i suggest a list of dicts, each one containing 'name, why, version, alternative' as we do for options

@user2:
alternatively a dict of dicts, with key being teh deprecated alias and the other 3 keys as subdicts

@author:
@user2 So you're thinking more like the latest rev but with 2 extra keys to be used in the msg text?

@user2:
i was even thinking of making it top level in argspec and not in the type itself, but either way works, as long as each alias can have their own version/msg

@author:
Where would you consider to be the top level in argspec?  If I missed a better location to do this in I'd be interested to look into it.