Fix bugs with CLI and improve the CLI stdout. (#55)

* Fix bugs with CLI and improve the CLI stdout.

* Fix bugs with the CLI for validate command and the  /health endpoint.

* Add TODOs.

* Remove a buggy debugging line.

* Fix a broken test.

* Fix minor styles with the quickstart notebook.

cromwell_tools/cli.py

@@ -1,4 +1,5 @@
 import argparse
+import requests
 from cromwell_tools.cromwell_api import CromwellAPI
 from cromwell_tools.cromwell_auth import CromwellAuth
 
@@ -29,7 +30,7 @@ def parser(arguments=None):
         'validate', help='validate help', description='Validate a cromwell workflow using womtool.')
 
     # cromwell url and authentication arguments apply to all sub-commands
-    cromwell_sub_commands = [submit, wait, status, health]
+    cromwell_sub_commands = (submit, wait, status, abort, release_hold, query, health)
     auth_args = {
         'url': 'The URL to the Cromwell server. e.g. "https://cromwell.server.org/"',
         'username':  'Cromwell username for HTTPBasicAuth.',
@@ -42,11 +43,12 @@ def add_auth_args(subcommand_parser):
         for arg_dest, help_text in auth_args.items():
             subcommand_parser.add_argument('--{arg}'.format(arg=arg_dest.replace('_', '-')),
                                            dest=arg_dest, default=None, type=str, help=help_text)
-    # todo this should be a group which is called authentication
+    # TODO: this should be a group which is called authentication
     for p in cromwell_sub_commands:
         add_auth_args(p)
 
     # submit arguments
+    # TODO: add short flags for arguments
     submit.add_argument('--wdl-file', dest='wdl_file', type=argparse.FileType('r'), required=True,
                         help='The workflow source file to submit for execution.')
     submit.add_argument('--inputs-file', dest='inputs_file', type=argparse.FileType('r'), required=True,
@@ -68,10 +70,10 @@ def add_auth_args(subcommand_parser):
                         help='Whether to submit the workflow in "On Hold" status.')
 
     # wait arguments
-    wait.add_argument('workflow-ids', nargs='+')
-    wait.add_argument('--timeout-minutes', type=int, default=120,
+    wait.add_argument('workflow_ids', nargs='+')
+    wait.add_argument('--timeout-minutes', dest='timeout_minutes', type=int, default=120,
                       help='number of minutes to wait before timeout')
-    wait.add_argument('--poll-interval-seconds', type=int, default=30,
+    wait.add_argument('--poll-interval-seconds', dest='poll_interval_seconds', type=int, default=30,
                       help='seconds between polling cromwell for workflow status')
 
     # status arguments
@@ -87,16 +89,19 @@ def add_auth_args(subcommand_parser):
     # TODO: implement CLI entry for query API.
 
     # validate arguments
-    validate.add_argument('--wdl-file', type=str, required=True)
-    validate.add_argument('--womtool-path', type=str, required=True, help='path to cromwell womtool jar')
-    validate.add_argument('--dependencies-json', type=str, default=None)
+    validate.add_argument('--wdl-file', dest='wdl', type=str, required=True)
+    validate.add_argument('--womtool-path', dest='womtool_path', type=str, required=True,
+                          help='path to cromwell womtool jar')
+    validate.add_argument('--dependencies-json', dest='dependencies_json', type=str, default=None)
 
     args = vars(main_parser.parse_args(arguments))
     # TODO: see if this can be moved or if the commands can be populated from above
     if args['command'] in ('submit', 'wait', 'status', 'abort', 'release_hold', 'health', 'validate'):
-        auth_arg_dict = {k: args.get(k) for k in auth_args.keys()}
-        auth = CromwellAuth.harmonize_credentials(**auth_arg_dict)
-        args['auth'] = auth
+        if args['command'] == 'validate': args['command'] = 'validate_workflow'
+        else:
+            auth_arg_dict = {k: args.get(k) for k in auth_args.keys()}
+            auth = CromwellAuth.harmonize_credentials(**auth_arg_dict)
+            args['auth'] = auth
         for k in auth_args:
             if k in args:
                 del args[k]
@@ -106,6 +111,11 @@ def add_auth_args(subcommand_parser):
 
 
 # this should just getattr from CromwellAPI and call the func with args.
+# TODO: refactor this module into class-based parsers
 def main(arguments=None):
     command, args = parser(arguments)
-    print(command(**args))
+    API_result = command(**args)
+    if isinstance(API_result, requests.Response):
+        print(API_result.text)
+    else:
+        print(API_result)

cromwell_tools/cromwell_api.py

@@ -12,7 +12,6 @@
 from datetime import datetime, timedelta
 from subprocess import PIPE, Popen
 from tenacity import retry, stop_after_delay, wait_exponential
-import warnings
 
 from cromwell_tools import utilities
 from cromwell_tools.utilities import _localize_file, validate_cromwell_label
@@ -47,7 +46,7 @@ class CromwellAPI(object):
     _status_endpoint = '/api/workflows/v1/{uuid}/status'
     _submit_endpoint = '/api/workflows/v1'
     _metadata_endpoint = '/api/workflows/v1/{uuid}/metadata'
-    _health_endpoint = '/api/engine/v1/status'
+    _health_endpoint = '/engine/v1/status'
     _release_hold_endpoint = '/api/workflows/v1/{uuid}/releaseHold'
     _query_endpoint = '/api/workflows/v1/query'
 
@@ -285,9 +284,9 @@ def query(cls, query_dict, auth, raise_for_status=False):
             response (requests.Response): HTTP response from Cromwell.
         """
         if 'additionalQueryResultFields' in query_dict.keys() or 'includeSubworkflows' in query_dict.keys():
-            warnings.warn('Note: additionalQueryResultFields, includeSubworkflows may not scale due to the following '
-                          'issues with Cromwell: https://github.com/broadinstitute/cromwell/issues/3115 and '
-                          'https://github.com/broadinstitute/cromwell/issues/3873', UserWarning)
+            logging.warning('Note: additionalQueryResultFields, includeSubworkflows may not scale due to the '
+                            'following issues with Cromwell: https://github.com/broadinstitute/cromwell/issues/3115 '
+                            'and https://github.com/broadinstitute/cromwell/issues/3873')
 
         query_params = cls._compose_query_params(query_dict)
 

cromwell_tools/cromwell_auth.py

@@ -4,17 +4,13 @@
 import six
 from google.oauth2 import service_account
 import google.auth.transport.requests
-import warnings
+import logging
 
 
 class AuthenticationError(Exception):
     pass
 
 
-class NoAuthenticationWarning(UserWarning):
-    pass
-
-
 class CromwellAuth:
 
     def __init__(self, url, header, auth):
@@ -53,9 +49,8 @@ def _validate_auth(header, auth):
             ValueError: when the header is not a valid header(with Bearer token).
         """
         if not header and not auth:
-            warnings.warn('You are not using any authentication with Cromwell. For security purposes, '
-                          'please consider adding authentication in front of your Cromwell instance!',
-                          NoAuthenticationWarning)
+            logging.warning('You are not using any authentication with Cromwell. For security purposes, '
+                            'please consider adding authentication in front of your Cromwell instance!')
 
         if header:
             if not isinstance(header, dict):
@@ -106,7 +101,6 @@ def from_service_account_key_file(cls, service_account_key, url):
         credentials = service_account.Credentials.from_service_account_file(service_account_key, scopes=scopes)
         if not credentials.valid:
             credentials.refresh(google.auth.transport.requests.Request())
-            credentials.get_access_token()
         header = {}
         credentials.apply(header)
         return cls(url=url, header=header, auth=None)

cromwell_tools/tests/test_cromwell_api.py

@@ -271,7 +271,7 @@ def _request_callback(request, context):
             return expected
 
         for cromwell_auth in self.auth_options:
-            mock_request.get('{0}/api/engine/v1/status'.format(cromwell_auth.url),
+            mock_request.get('{0}/engine/v1/status'.format(cromwell_auth.url),
                               json=_request_callback)
             result = CromwellAPI.health(cromwell_auth)
             self.assertEqual(result.status_code, 200)

cromwell_tools/tests/test_cromwell_auth.py

@@ -4,7 +4,7 @@
 import mock
 import pytest
 import requests
-from cromwell_tools.cromwell_auth import CromwellAuth, NoAuthenticationWarning
+from cromwell_tools.cromwell_auth import CromwellAuth
 
 
 def setup_auth_types():
@@ -73,7 +73,6 @@ def test_harmonize_credentials_from_service_account_key(mock_header):
 def test_harmonize_credentials_from_no_authentication():
     url = "https://fake_url"
     expected_auth = CromwellAuth(url=url, header=None, auth=None)
-    with pytest.warns(NoAuthenticationWarning):
-        auth = CromwellAuth.harmonize_credentials(url=url)
-        assert auth.auth == expected_auth.auth
-        assert auth.header == expected_auth.header
+    auth = CromwellAuth.harmonize_credentials(url=url)
+    assert auth.auth == expected_auth.auth
+    assert auth.header == expected_auth.header

notebooks/Quickstart/quickstart.ipynb

@@ -35,7 +35,7 @@
     "# Authenticate with Cromwell using HTTPBasicAuth (username and password)\n",
     "auth = CromwellAuth.harmonize_credentials(username='username', \n",
     "                                          password='password', \n",
-    "                                          url='https://cromwell.mint-xxx.broadinstitute.org')\n",
+    "                                          url='https://cromwell.xxx.broadinstitute.org')\n",
     "\n",
     "# Authenticate with Cromwell using HTTPBasicAuth (secret JSON file)\n",
     "auth_2 = CromwellAuth.harmonize_credentials(secrets_file='path/to/secrets_file.json')\n",
@@ -233,7 +233,7 @@
    "source": [
     "There are a lot of query keys can be used to filter workflows\n",
     "A complicated query dict could be:\n",
-    "```JSON\n",
+    "```python\n",
     "custom_query_dict = {\n",
     "    'label': {\n",
     "        'cromwell-workflow-id': 'cromwell-xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx',\n",