From 1d975bdc9349e4cef02ed522adfbc610d9804eb6 Mon Sep 17 00:00:00 2001
From: saichint <saichint@cisco.com>
Date: Fri, 13 Apr 2018 03:17:32 -0700
Subject: [PATCH] fix nxos_ntp_options (#38695)

---
 .../modules/network/nxos/nxos_ntp_options.py  | 65 ++++++++-----------
 .../nxos_ntp_options/tests/common/sanity.yaml | 60 ++++++++++++-----
 test/sanity/validate-modules/ignore.txt       |  1 -
 3 files changed, 72 insertions(+), 54 deletions(-)

diff --git a/lib/ansible/modules/network/nxos/nxos_ntp_options.py b/lib/ansible/modules/network/nxos/nxos_ntp_options.py
index c153c988f7..30882041a2 100644
--- a/lib/ansible/modules/network/nxos/nxos_ntp_options.py
+++ b/lib/ansible/modules/network/nxos/nxos_ntp_options.py
@@ -34,12 +34,8 @@ author:
     - Jason Edelman (@jedelman8)
 notes:
     - Tested against NXOSv 7.3.(0)D1(1) on VIRL
-    - At least one of C(master) or C(logging) params must be supplied.
-    - When C(state=absent), boolean parameters are flipped,
-      e.g. C(master=true) will disable the authoritative server.
-    - When C(state=absent) and C(master=true), the stratum will be removed as well.
-    - When C(state=absent) and C(master=false), the stratum will be configured
-      to its default value, 8.
+    - When C(state=absent), master and logging will be set to False and
+      stratum will be removed as well
 options:
     master:
         description:
@@ -75,7 +71,7 @@ updates:
     description: command sent to the device
     returned: always
     type: list
-    sample: ["no ntp logging", "ntp master 11"]
+    sample: ["no ntp logging", "ntp master 12"]
 '''
 import re
 
@@ -85,20 +81,20 @@ from ansible.module_utils.basic import AnsibleModule
 
 
 def get_current(module):
-    cmd = ('show running-config', 'show ntp logging')
+    cmd = ('show running-config | inc ntp')
 
-    output = run_commands(module, ({'command': cmd[0], 'output': 'text'},
-                                   {'command': cmd[1], 'output': 'text'}))
+    master = False
+    logging = False
+    stratum = None
 
-    match = re.search(r"^ntp master(?: (\d+))", output[0], re.M)
-    if match:
-        master = True
-        stratum = match.group(1)
-    else:
-        master = False
-        stratum = None
+    output = run_commands(module, ({'command': cmd, 'output': 'text'}))[0]
 
-    logging = 'enabled' in output[1].lower()
+    if output:
+        match = re.search(r"^ntp master(?: (\d+))", output, re.M)
+        if match:
+            master = True
+            stratum = match.group(1)
+        logging = 'ntp logging' in output.lower()
 
     return {'master': master, 'stratum': stratum, 'logging': logging}
 
@@ -106,7 +102,7 @@ def get_current(module):
 def main():
     argument_spec = dict(
         master=dict(required=False, type='bool'),
-        stratum=dict(required=False, type='str', default='8'),
+        stratum=dict(required=False, type='str'),
         logging=dict(required=False, type='bool'),
         state=dict(choices=['absent', 'present'], default='present'),
     )
@@ -124,15 +120,10 @@ def main():
     logging = module.params['logging']
     state = module.params['state']
 
-    if stratum:
-        try:
-            stratum_int = int(stratum)
-            if stratum_int < 1 or stratum_int > 15:
-                raise ValueError
-        except ValueError:
-            module.fail_json(msg='stratum must be an integer between 1 and 15')
+    if stratum and master is False:
+        if stratum != 8:
+            module.fail_json(msg='master MUST be True when stratum is changed')
 
-    desired = {'master': master, 'stratum': stratum, 'logging': logging}
     current = get_current(module)
 
     result = {'changed': False}
@@ -146,19 +137,17 @@ def main():
             commands.append('no ntp logging')
 
     elif state == 'present':
-        if desired['master'] and desired['master'] != current['master']:
-            if desired['stratum']:
-                commands.append('ntp master %s' % stratum)
-            else:
-                commands.append('ntp master')
-        elif desired['stratum'] and desired['stratum'] != current['stratum']:
+        if master and not current['master']:
+            commands.append('ntp master')
+        elif master is False and current['master']:
+            commands.append('no ntp master')
+        if stratum and stratum != current['stratum']:
             commands.append('ntp master %s' % stratum)
 
-        if desired['logging'] and desired['logging'] != current['logging']:
-            if desired['logging']:
-                commands.append('ntp logging')
-            else:
-                commands.append('no ntp logging')
+        if logging and not current['logging']:
+            commands.append('ntp logging')
+        elif logging is False and current['logging']:
+            commands.append('no ntp logging')
 
     result['commands'] = commands
     result['updates'] = commands
diff --git a/test/integration/targets/nxos_ntp_options/tests/common/sanity.yaml b/test/integration/targets/nxos_ntp_options/tests/common/sanity.yaml
index 43833de90a..fd64136117 100644
--- a/test/integration/targets/nxos_ntp_options/tests/common/sanity.yaml
+++ b/test/integration/targets/nxos_ntp_options/tests/common/sanity.yaml
@@ -31,20 +31,9 @@
       that:
         - "result.changed == false"
 
-  - name: Remove ntp with master and default stratum
-    nxos_ntp_options: &remove_master_default_stratum
-      logging: true
-      master: true
-      state: absent
-      provider: "{{ connection }}"      
-    register: result
-
-  - assert: *true
-
   - name: Configure ntp with master and non-default stratum
     nxos_ntp_options: &configure_master_non_default_stratum
       master: true
-      logging: true
       stratum: 10
       state: present
       provider: "{{ connection }}"      
@@ -58,16 +47,57 @@
 
   - assert: *false
 
-  - name: Remove ntp with master and non-default stratum
-    nxos_ntp_options: &remove_master_non_default_stratum
-      logging: true
+  - name: Configure ntp with master and no logging
+    nxos_ntp_options: &configure_no_log
       master: true
-      state: absent
+      stratum: 10
+      logging: false
+      state: present
       provider: "{{ connection }}"
     register: result
 
   - assert: *true
 
+  - name: "Check Idempotence - Configure ntp with master and no logging"
+    nxos_ntp_options: *configure_no_log
+    register: result
+
+  - assert: *false
+
+  - name: Configure ntp with logging and no master
+    nxos_ntp_options: &configure_no_master
+      master: false
+      logging: true
+      state: present
+      provider: "{{ connection }}"
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Configure ntp with logging and no master"
+    nxos_ntp_options: *configure_no_master
+    register: result
+
+  - assert: *false
+
+  - name: "Configure ntp with master and non-default stratum again"
+    nxos_ntp_options: *configure_master_non_default_stratum
+    register: result
+
+  - assert: *true
+
+  - name: Remove ntp options
+    nxos_ntp_options: *default
+    register: result
+
+  - assert: *true
+
+  - name: "Check Idempotence - Remove"
+    nxos_ntp_options: *default
+    register: result
+
+  - assert: *false
+
   always:
   - name: Cleanup ntp config
     nxos_ntp_options: *default
diff --git a/test/sanity/validate-modules/ignore.txt b/test/sanity/validate-modules/ignore.txt
index 3573cd7de4..b6e0efbd52 100644
--- a/test/sanity/validate-modules/ignore.txt
+++ b/test/sanity/validate-modules/ignore.txt
@@ -1347,7 +1347,6 @@ lib/ansible/modules/network/nxos/nxos_igmp_interface.py E326
 lib/ansible/modules/network/nxos/nxos_interface.py E324
 lib/ansible/modules/network/nxos/nxos_lldp.py E326
 lib/ansible/modules/network/nxos/nxos_logging.py E325
-lib/ansible/modules/network/nxos/nxos_ntp_options.py E324
 lib/ansible/modules/network/nxos/nxos_nxapi.py E324
 lib/ansible/modules/network/nxos/nxos_nxapi.py E325
 lib/ansible/modules/network/nxos/nxos_nxapi.py E326