Provide a clear error message when using client.Client without a sessiontrain-em3.1.2stable/train

Currently we fail with _construct_http_client() takes at least 1 argument.
This change provides a proper TypeError and updates documentation to make
it clear where a session is required. Also provided are explicit unit
tests on passing a session via various means.

Change-Id: I96073dc80d225a9b88fdc12bb058c0145aca623b
(cherry picked from commit e8914a7ef9eb582441a7067f2663e19209f32822)

5 files changed, 71 insertions, 4 deletions

diff --git a/ironicclient/client.py b/ironicclient/client.py
index dbcd5c5..29cf335 100644
--- a/ironicclient/client.py
+++ b/ironicclient/client.py
@@ -22,7 +22,7 @@ LOG = logging.getLogger(__name__)
 
 
 def get_client(api_version, auth_type=None, os_ironic_api_version=None,
-               max_retries=None, retry_interval=None, **kwargs):
+               max_retries=None, retry_interval=None, session=None, **kwargs):
     """Get an authenticated client, based on the credentials.
 
     :param api_version: the API version to use. Valid value: '1'.
@@ -31,6 +31,8 @@ def get_client(api_version, auth_type=None, os_ironic_api_version=None,
     :param max_retries: Maximum number of retries in case of conflict error
     :param retry_interval: Amount of time (in seconds) between retries in case
         of conflict error.
+    :param session: An existing keystoneauth session. Will be created from
+        kwargs if not provided.
     :param kwargs: all the other params that are passed to keystoneauth.
     """
     # TODO(TheJulia): At some point, we should consider possibly noting
@@ -48,7 +50,6 @@ def get_client(api_version, auth_type=None, os_ironic_api_version=None,
             auth_type = 'token'
         else:
             auth_type = 'password'
-    session = kwargs.get('session')
     if not session:
         loader = kaloading.get_plugin_loader(auth_type)
         loader_options = loader.get_options()
@@ -104,8 +105,22 @@ def get_client(api_version, auth_type=None, os_ironic_api_version=None,
     return Client(api_version, **ironicclient_kwargs)
 
 
-def Client(version, *args, **kwargs):
+def Client(version, endpoint_override=None, session=None, *args, **kwargs):
+    """Create a client of an appropriate version.
+
+    This call requires a session. If you want it to be created, use
+    ``get_client`` instead.
+
+    :param endpoint_override: A bare metal endpoint to use.
+    :param session: A keystoneauth session to use. This argument is actually
+        required and is marked optional only for backward compatibility.
+    :param args: Other arguments to pass to the HTTP client. Not recommended,
+        use kwargs instead.
+    :param kwargs: Other keyword arguments to pass to the HTTP client (e.g.
+        ``insecure``).
+    """
     module = importutils.import_versioned_module('ironicclient',
                                                  version, 'client')
     client_class = getattr(module, 'Client')
-    return client_class(*args, **kwargs)
+    return client_class(endpoint_override=endpoint_override, session=session,
+                        *args, **kwargs)
diff --git a/ironicclient/tests/unit/test_client.py b/ironicclient/tests/unit/test_client.py
index a420310..fc43aa7 100644
--- a/ironicclient/tests/unit/test_client.py
+++ b/ironicclient/tests/unit/test_client.py
@@ -265,3 +265,21 @@ class ClientTest(utils.BaseTestCase):
         }
         self.assertRaises(exc.AmbiguousAuthSystem, iroclient.get_client,
                           '1', **kwargs)
+
+    def test_client_no_session(self):
+        # get_client can create a session, all other calls require it
+        self.assertRaisesRegex(TypeError,
+                               "session is required",
+                               iroclient.Client,
+                               1, "http://example.com")
+
+    def test_client_session_via_posargs(self):
+        session = mock.Mock()
+        session.get_endpoint.return_value = 'http://localhost:35357/v2.0'
+        iroclient.Client('1', "http://example.com", session)
+
+    def test_client_session_via_kwargs(self):
+        session = mock.Mock()
+        session.get_endpoint.return_value = 'http://localhost:35357/v2.0'
+        iroclient.Client('1', session=session,
+                         endpoint_override="http://example.com")
diff --git a/ironicclient/tests/unit/v1/test_client.py b/ironicclient/tests/unit/v1/test_client.py
index bc62863..e765cf7 100644
--- a/ironicclient/tests/unit/v1/test_client.py
+++ b/ironicclient/tests/unit/v1/test_client.py
@@ -115,3 +115,25 @@ class ClientTest(utils.BaseTestCase):
         # is being called in the client and returns a version,
         # although mocking might need to be restrutured to
         # properly achieve that.
+
+    def test_client_no_session(self, http_client_mock):
+        self.assertRaisesRegex(TypeError,
+                               "session is required",
+                               client.Client,
+                               "http://example.com")
+
+    def test_client_session_via_posargs(self, http_client_mock):
+        session = mock.Mock()
+        client.Client("http://example.com", session)
+        http_client_mock.assert_called_once_with(
+            session, api_version_select_state='default',
+            endpoint_override="http://example.com",
+            os_ironic_api_version=client.DEFAULT_VER)
+
+    def test_client_session_via_kwargs(self, http_client_mock):
+        session = mock.Mock()
+        client.Client(session=session, endpoint_override="http://example.com")
+        http_client_mock.assert_called_once_with(
+            session, api_version_select_state='default',
+            endpoint_override="http://example.com",
+            os_ironic_api_version=client.DEFAULT_VER)
diff --git a/ironicclient/v1/client.py b/ironicclient/v1/client.py
index cf729d9..254185e 100644
--- a/ironicclient/v1/client.py
+++ b/ironicclient/v1/client.py
@@ -44,6 +44,11 @@ class Client(object):
 
     def __init__(self, endpoint_override=None, *args, **kwargs):
         """Initialize a new client for the Ironic v1 API."""
+        if not args and not kwargs.get('session'):
+            raise TypeError("A session is required for creating a client, "
+                            "use ironicclient.client.get_client to create "
+                            "it automatically")
+
         allow_downgrade = kwargs.pop('allow_api_version_downgrade', False)
         if kwargs.get('os_ironic_api_version'):
             # TODO(TheJulia): We should sanity check os_ironic_api_version
diff --git a/releasenotes/notes/client-session-09e6ced1fbc6a9b0.yaml b/releasenotes/notes/client-session-09e6ced1fbc6a9b0.yaml
new file mode 100644
index 0000000..376d4fc
--- /dev/null
+++ b/releasenotes/notes/client-session-09e6ced1fbc6a9b0.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fails with a clear TypeError when a session is not provided to
+    ``client.Client`` or ``v1.client.Client``. Before we used to throw::
+
+      _construct_http_client() takes at least 1 argument