Consul: make service_port optional in service definition, like specified in Consul docs (#21737)

* Consul: make service_port optional in service definition, Fixes #21727

* Remove consul module from legacy-files.txt

* consul: Pep8 fixes

lib/ansible/modules/clustering/consul.py

@@ -95,9 +95,10 @@ options:
         default: None
     service_port:
         description:
-          - the port on which the service is listening required for
+          - the port on which the service is listening. Can optionally be supplied for
             registration of a service, i.e. if service_name or service_id is set
         required: false
+        default: None
     service_address:
         description:
           - the address to advertise that the service will be listening on.
@@ -211,6 +212,13 @@ EXAMPLES = '''
     service_name: nginx
     state: absent
 
+- name: register celery worker service
+  consul:
+    service_name: celery-worker
+    tags:
+      - prod
+      - worker
+
 - name: create a node level check to test disk usage
   consul:
     check_name: Disk usage
@@ -234,8 +242,8 @@ try:
 except ImportError:
     python_consul_installed = False
 
-def register_with_consul(module):
 
+def register_with_consul(module):
     state = module.params.get('state')
 
     if state == 'present':
@@ -359,7 +367,6 @@ def get_service_by_id_or_name(consul_api, service_id_or_name):
 
 
 def parse_check(module):
-
     if len(filter(None, [module.params.get('script'), module.params.get('ttl'), module.params.get('http')])) > 1:
         module.fail_json(
             msg='check are either script, http or ttl driven, supplying more than one does not make sense')
@@ -382,8 +389,7 @@ def parse_check(module):
 
 
 def parse_service(module):
-
+    if module.params.get('service_name'):
-    if module.params.get('service_name') and module.params.get('service_port'):
         return ConsulService(
             module.params.get('service_id'),
             module.params.get('service_name'),
@@ -391,10 +397,8 @@ def parse_service(module):
             module.params.get('service_port'),
             module.params.get('tags'),
         )
-    elif module.params.get('service_name') and not module.params.get('service_port'):
+    elif module.params.get('service_port') and not module.params.get('service_name'):
-
+        module.fail_json(msg="service_port supplied but no service_name, a name is required to configure a service.")
-        module.fail_json(msg="service_name supplied but no service_port, a port is required to configure a service. Did you configure "
-                             "the 'port' argument meaning 'service_port'?")
 
 
 class ConsulService():
@@ -415,23 +419,20 @@ class ConsulService():
             self.tags = loaded['Tags']
 
     def register(self, consul_api):
+        optional = {}
+
+        if self.port:
+            optional['port'] = self.port
+
         if len(self.checks) > 0:
-            check = self.checks[0]
+            optional['check'] = checks[0].check
-
+
-            consul_api.agent.service.register(
+        consul_api.agent.service.register(
-                self.name,
+            self.name,
-                service_id=self.id,
+            service_id=self.id,
-                address=self.address,
+            address=self.address,
-                port=self.port,
+            tags=self.tags,
-                tags=self.tags,
+            **optional)
-                check=check.check)
-        else:
-            consul_api.agent.service.register(
-                self.name,
-                service_id=self.id,
-                address=self.address,
-                port=self.port,
-                tags=self.tags)
 
     def add_check(self, check):
         self.checks.append(check)
@@ -443,11 +444,11 @@ class ConsulService():
         return len(self.checks) > 0
 
     def __eq__(self, other):
-        return (isinstance(other, self.__class__)
+        return (isinstance(other, self.__class__) and
-                and self.id == other.id
+                self.id == other.id and
-                and self.name == other.name
+                self.name == other.name and
-                and self.port == other.port
+                self.port == other.port and
-                and self.tags == other.tags)
+                self.tags == other.tags)
 
     def __ne__(self, other):
         return not self.__eq__(other)
@@ -466,7 +467,7 @@ class ConsulService():
 class ConsulCheck():
 
     def __init__(self, check_id, name, node=None, host='localhost',
-                    script=None, interval=None, ttl=None, notes=None, http=None, timeout=None, service_id=None):
+                 script=None, interval=None, ttl=None, notes=None, http=None, timeout=None, service_id=None):
         self.check_id = self.name = name
         if check_id:
             self.check_id = check_id
@@ -495,7 +496,6 @@ class ConsulCheck():
 
             self.check = consul.Check.http(http, self.interval, self.timeout)
 
-
     def validate_duration(self, name, duration):
         if duration:
             duration_units = ['ns', 'us', 'ms', 's', 'm', 'h']
@@ -509,12 +509,12 @@ class ConsulCheck():
                                         check=self.check)
 
     def __eq__(self, other):
-        return (isinstance(other, self.__class__)
+        return (isinstance(other, self.__class__) and
-                and self.check_id == other.check_id
+                self.check_id == other.check_id and
-                and self.service_id == other.service_id
+                self.service_id == other.service_id and
-                and self.name == other.name
+                self.name == other.name and
-                and self.script == script
+                self.script == script and
-                and self.interval == interval)
+                self.interval == interval)
 
     def __ne__(self, other):
         return not self.__eq__(other)
@@ -542,10 +542,12 @@ class ConsulCheck():
         except:
             pass
 
+
 def test_dependencies(module):
     if not python_consul_installed:
         module.fail_json(msg="python-consul required for this module. see http://python-consul.readthedocs.org/en/latest/#installation")
 
+
 def main():
     module = AnsibleModule(
         argument_spec=dict(
@@ -562,7 +564,7 @@ def main():
             service_id=dict(required=False),
             service_name=dict(required=False),
             service_address=dict(required=False, type='str', default=None),
-            service_port=dict(required=False, type='int'),
+            service_port=dict(required=False, type='int', default=None),
             state=dict(default='present', choices=['present', 'absent']),
             interval=dict(required=False, type='str'),
             ttl=dict(required=False, type='str'),

test/integration/roles/test_consul_service/tasks/main.yml

@@ -5,6 +5,7 @@
   with_items:
       - service1
       - service2
+      - service3
       - http_check
       - with_check
       - with_tags
@@ -46,6 +47,20 @@
         - basic2_result.service_id == 'service2'
         - basic2_result.service_name == 'Basic Service'
 
+- name: register very basic service without service_port
+  consul:
+    service_name: Basic Service Without Port
+    service_id: service3
+  register: basic3_result
+
+- name: verify service3 registration
+  assert:
+    that:
+        - basic3_result.changed
+        - basic3_result.service_port == None
+        - basic3_result.service_id == 'service3'
+        - basic3_result.service_name == 'Basic Service Without Port'
+
 - name: register a service with an http check
   consul:
     service_name: http_check

test/sanity/pep8/legacy-files.txt

@@ -212,7 +212,6 @@ lib/ansible/modules/cloud/webfaction/webfaction_db.py
 lib/ansible/modules/cloud/webfaction/webfaction_domain.py
 lib/ansible/modules/cloud/webfaction/webfaction_mailbox.py
 lib/ansible/modules/cloud/webfaction/webfaction_site.py
-lib/ansible/modules/clustering/consul.py
 lib/ansible/modules/clustering/consul_acl.py
 lib/ansible/modules/clustering/consul_kv.py
 lib/ansible/modules/clustering/consul_session.py