Repository: ansible/ansible
Pull request: cron: removal of deprecations and minor fix
  name or cron_file is required.
  reboot was removed.
  updated docs to clarify usage.
  now using cron_file=/etc/crontab is an error

  fixes #37355

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
modules/cron

Problem statement:
Cron module is not idempotent until name option is set
##### ISSUE TYPE
 - Feature Idea

##### COMPONENT NAME
cron_module

##### ANSIBLE VERSION
```
ansible 2.3.1.0
  config file =
  configured module search path = Default w/o overrides
  python version = 2.7.10 (default, Feb  7 2017, 00:08:15) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)]
```

##### CONFIGURATION
Everything standard

##### OS / ENVIRONMENT
RedHat Enterprise Linux 7

##### SUMMARY
I am expecting every playbook to be idempotent, however cron module will just add new entry to crontab each time Ansible playbook was executed.  It can be idempotent only when `name` option is set. In my opinion, `name` option should be required or at least there should be warning informing about such, not obvious, behavior. 

##### STEPS TO REPRODUCE
1. Create playbook with content pasted below
1. Run playbook on server
1. Check `crontab -l`
1. Run playbook again
1. Check `crontab -l` again

```yaml
- name: "Testing cron module idempotency"
  cron:
    state: present
    minute: 0
    job: "/bin/bash echo test >> /tmp/test.txt"
```

##### EXPECTED RESULTS
Job entry should not be duplicated.

##### ACTUAL RESULTS
Job entry is duplicated.

Hints from issue discussion:
Probably this is a feature request and not a bug. This behaviour is described in the documentation of the module[1]:

> name: Description of a crontab entry or, if env is set, the name of environment variable. Required if state=absent. Note that if name is not set and state=present, then a new crontab entry will always be created, regardless of existing ones.

[1] http://docs.ansible.com/ansible/latest/cron_module.html 
Changed type to `Feature Idea`. To me this is still a bug, even though it’s documented.
cc @EvanK @dsummersl @lberruti
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
IMNSHO this is not a feature request but a bug. The `name` attribute ought to be mandatory in order to default to idempotency. What we should do is to deprecate the non-mandatory `name` in a future version and then make `name` mandatory. 
idempotence is not a requirement .. otherwise shell/raw would not exist. 
The user should have the option to do this in any manner they wish. 

That said, the documentation should be very clear on the expectations when 'name' is not included.
I find it doubtful that making `name` mandatory will hurt anybody, and it'll prevent people from mistakenly shooting themselves in the foot in future. I see no harm in preventing that.

i'm experiencing that Ansible 2.6 is adding redundant entries to the crontab even when a `name` is provided.

```yaml
- set_fact:
    daily_reboot_hour: "{{ daily_reboot_time.split(':')[0] }}"
    daily_reboot_minute: "{{ daily_reboot_time.split(':')[1] }}"
- name: set daily reboot time
  cron:
    hour: "{{ daily_reboot_hour }}"
    minute: "{{ daily_reboot_minute }}"
    job: reboot
  when: daily_reboot_time != ''
```

repeated executions result in:

```
#Ansible: None
04 04 * * * reboot
#Ansible: None
04 04 * * * reboot
#Ansible: None
04 04 * * * reboot
```


@funkyfuture 
IIRC It is different name.
You provided task name, not crontab entry name. So in your case it would be rather:

```yaml
- set_fact:
    daily_reboot_hour: "{{ daily_reboot_time.split(':')[0] }}"
    daily_reboot_minute: "{{ daily_reboot_time.split(':')[1] }}"
- name: set daily reboot time
  cron:
    name: entry name
    hour: "{{ daily_reboot_hour }}"
    minute: "{{ daily_reboot_minute }}"
    job: reboot
  when: daily_reboot_time != ''
```
cc @dirtyharrycallahan @rhaido
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Yes, and I will gladly repeat myself: the `cron` module's `name` attribute ought to be mandatory.

Commit message: rstified

Patch to review:
diff --git a/changelogs/fragments/cron_removals.yml b/changelogs/fragments/cron_removals.yml
new file mode 100644
index 00000000000000..1e081ccada3093
--- /dev/null
+++ b/changelogs/fragments/cron_removals.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - M(cron), removed previously deprecated C(reboot) and now requires either C(name) or C(cron_file) as unique identifier.
diff --git a/lib/ansible/modules/cron.py b/lib/ansible/modules/cron.py
index a84a4bada7f2a4..3f029c158f71f2 100644
--- a/lib/ansible/modules/cron.py
+++ b/lib/ansible/modules/cron.py
@@ -57,12 +57,14 @@
   cron_file:
     description:
       - If specified, uses this file instead of an individual user's crontab.
+        The assumption is that this file is exclusively managed by the module,
+        do not use if the file contains multiple entries, NEVER use for /etc/crontab.
       - If this is a relative path, it is interpreted with respect to I(/etc/cron.d).
-      - If it is absolute, it will typically be C(/etc/crontab).
       - Many linux distros expect (and some require) the filename portion to consist solely
         of upper- and lower-case letters, digits, underscores, and hyphens.
-      - To use the I(cron_file) parameter you must specify the I(user) as well.
-    type: str
+      - Using this parameter requires you to specify the I(user) as well, unless I(state) is not I(present).
+      - Either this parameter or I(name) is required
+    type: path
   backup:
     description:
       - If set, create a backup of the crontab before it is modified.
@@ -232,6 +234,7 @@ def __init__(self, module, user=None, cron_file=None):
         self.cron_cmd = self.module.get_bin_path('crontab', required=True)
 
         if cron_file:
+
             if os.path.isabs(cron_file):
                 self.cron_file = cron_file
                 self.b_cron_file = to_bytes(cron_file, errors='surrogate_or_strict')
@@ -557,7 +560,7 @@ def main():
             name=dict(type='str', required=True),
             user=dict(type='str'),
             job=dict(type='str', aliases=['value']),
-            cron_file=dict(type='str'),
+            cron_file=dict(type='path'),
             state=dict(type='str', default='present', choices=['present', 'absent']),
             backup=dict(type='bool', default=False),
             minute=dict(type='str', default='*'),
@@ -575,6 +578,9 @@ def main():
         mutually_exclusive=[
             ['insertafter', 'insertbefore'],
         ],
+        required_one_of=[
+            ['name', 'cron_file']
+        ],
     )
 
     name = module.params['name']
@@ -600,6 +606,10 @@ def main():
     warnings = list()
 
     if cron_file:
+
+        if cron_file == '/etc/crontab':
+            module.fail_json(msg="Will not manage /etc/crontab via cron_file, see documentation.")
+
         cron_file_basename = os.path.basename(cron_file)
         if not re.search(r'^[A-Z0-9_-]+$', cron_file_basename, re.I):
             warnings.append('Filename portion of cron_file ("%s") should consist' % cron_file_basename +
@@ -632,15 +642,15 @@ def main():
     if special_time and platform.system() == 'SunOS':
         module.fail_json(msg="Solaris does not support special_time=... or @reboot")
 
-    if cron_file and do_install:
-        if not user:
+    if do_install:
+        if cron_file and not user:
             module.fail_json(msg="To use cron_file=... parameter you must specify user=... as well")
 
-    if job is None and do_install:
-        module.fail_json(msg="You must specify 'job' to install a new cron job or variable")
+        if job is None:
+            module.fail_json(msg="You must specify 'job' to install a new cron job or variable")
 
-    if (insertafter or insertbefore) and not env and do_install:
-        module.fail_json(msg="Insertafter and insertbefore parameters are valid only with env=yes")
+        if (insertafter or insertbefore) and not env:
+            module.fail_json(msg="Insertafter and insertbefore parameters are valid only with env=yes")
 
     # if requested make a backup before making a change
     if backup and not module.check_mode:


Code review comments:
diff_hunk: @@ -575,6 +578,9 @@ def main():
         mutually_exclusive=[
             ['insertafter', 'insertbefore'],
         ],
+        required_one_of=[
+            ['name', 'cron_file']
path: lib/ansible/modules/cron.py
text: @user1:
The `name` parameter above has `required=True` and upon a closer look, being optional went through a deprecation period already and was removed a couple weeks ago in https://github.com/ansible/ansible/commit/d7df8a4484e50b27e921e9fe887dc84c86617e7f.