RuntimeConfig: address review comments.

runtimeconfig/google/cloud/runtimeconfig/_helpers.py

@@ -31,8 +31,13 @@ def config_name_from_full_name(full_name):
 
     :rtype: string
     :returns: the config's short name, given its full resource name
+    :raises: :class:`ValueError` if ``full_name`` is not the expected format
     """
-    _, _, _, result = full_name.split('/')
+    projects, _, configs, result = full_name.split('/')
+    if projects != 'projects' or configs != 'configs':
+        raise ValueError(
+            'Unexpected format of resource', full_name,
+            'Expected "projects/{proj}/configs/{cfg}"')
     return result
 
 
@@ -56,12 +61,12 @@ def variable_name_from_full_name(full_name):
 
     :rtype: string
     :returns: the variable's short name, given its full resource name
+    :raises: :class:`ValueError` if ``full_name`` is not the expected format
     """
-    parts = full_name.split('/')
-    assert parts[0] == 'projects'
-    assert parts[2] == 'configs'
-    assert parts[4] == 'variables'
-
-    # Variables are hierarchical. Each node in the hierarchy is separated by a
-    # / character.
-    return '/'.join(parts[5:])
+    projects, _, configs, _, variables, result = full_name.split('/', 5)
+    if (projects != 'projects' or configs != 'configs' or
+            variables != 'variables'):
+        raise ValueError(
+            'Unexpected format of resource', full_name,
+            'Expected "projects/{proj}/configs/{cfg}/variables/..."')
+    return result

runtimeconfig/google/cloud/runtimeconfig/config.py

@@ -189,7 +189,7 @@ def reload(self, client=None, fields=None):
                                              query_params=query_params)
         self._set_properties(api_response=resp)
 
-    def get_variable(self, variable_name, client=None):
+    def get_variable(self, variable_name, fields=None, client=None):
         """API call:  get a variable via a ``GET`` request.
 
         This will return None if the variable doesn't exist::
@@ -205,6 +205,12 @@ def get_variable(self, variable_name, client=None):
         :type variable_name: string
         :param variable_name: The name of the variable to retrieve.
 
+        :type fields: string or ``NoneType``
+        :param fields: Selector specifying which fields to include in a
+                       partial response. Must be a list of fields. For example
+                       to get a partial response with just the name and value:
+                       'name,value'
+
         :type client: :class:`~google.cloud.runtimeconfig.client.Client` or
                       ``NoneType``
         :param client: Optional. The client to use.  If not passed, falls back
@@ -216,10 +222,7 @@ def get_variable(self, variable_name, client=None):
         client = self._require_client(client)
         variable = Variable(config=self, name=variable_name)
         try:
-            response = client.connection.api_request(
-                method='GET', path=variable.path, _target_object=variable)
-            # NOTE: We assume response.get('name') matches `variable_name`.
-            variable._set_properties(api_response=response)
+            variable.reload(fields=fields, client=client)
             return variable
         except NotFound:
             return None

runtimeconfig/google/cloud/runtimeconfig/variable.py

@@ -63,7 +63,7 @@ def path(self):
         :rtype: string
         :returns: The URL path based on config and variable names.
         """
-        return '/%s' % (self.full_name)
+        return '/%s' % (self.full_name,)
 
     @property
     def client(self):
@@ -110,7 +110,7 @@ def update_time(self):
                   ``None`` if the property is not set locally.
         """
         value = self._properties.get('updateTime')
-        if value:
+        if value is not None:
             value = _rfc3339_to_datetime(value)
         return value
 
@@ -166,24 +166,24 @@ def exists(self, client=None):
         except NotFound:
             return False
 
-    def reload(self, client=None, fields=None):
+    def reload(self, fields=None, client=None):
         """API call:  reload the variable via a ``GET`` request.
 
         This method will reload the newest data for the variable.
 
         See:
         https://cloud.google.com/deployment-manager/runtime-configurator/reference/rest/v1beta1/projects.configs/get
 
-        :type client: :class:`google.cloud.runtimeconfig.client.Client` or
-                      :data:`NoneType <types.NoneType>`
-        :param client: the client to use.  If not passed, falls back to
-                       the client stored on the current config.
-
         :type fields: string or ``NoneType``
         :param fields: Selector specifying which fields to include in a
                        partial response. Must be a list of fields. For example
                        to get a partial response with just the name and value:
                        'name,value'
+
+        :type client: :class:`google.cloud.runtimeconfig.client.Client` or
+                      :data:`NoneType <types.NoneType>`
+        :param client: the client to use.  If not passed, falls back to
+                       the client stored on the current config.
         """
         client = self._require_client(client)
         query_params = {}

runtimeconfig/unit_tests/test__helpers.py

@@ -36,6 +36,11 @@ def test_w_name_w_all_extras(self):
         config_name = self._callFUT(PATH)
         self.assertEqual(config_name, CONFIG_NAME)
 
+    def test_w_bad_format(self):
+        PATH = 'definitley/not/a/resource-name'
+        with self.assertRaises(ValueError):
+            self._callFUT(PATH)
+
 
 class Test_variable_name_from_full_name(unittest.TestCase):
 
@@ -61,3 +66,8 @@ def test_w_name_w_all_extras(self):
             PROJECT, CONFIG_NAME, VARIABLE_NAME)
         variable_name = self._callFUT(PATH)
         self.assertEqual(variable_name, VARIABLE_NAME)
+
+    def test_w_bad_format(self):
+        PATH = 'definitley/not/a/resource/name/for/a/variable'
+        with self.assertRaises(ValueError):
+            self._callFUT(PATH)

runtimeconfig/unit_tests/test_config.py

@@ -50,7 +50,8 @@ def test_ctor(self):
     def test_ctor_w_no_name(self):
         client = _Client(project=self.PROJECT)
         config = self._makeOne(name=None, client=client)
-        self.assertRaises(ValueError, lambda: config.full_name)
+        with self.assertRaises(ValueError):
+            _ = config.full_name
 
     def test_exists_miss_w_bound_client(self):
         conn = _Connection()
@@ -62,7 +63,7 @@ def test_exists_miss_w_bound_client(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.CONFIG_PATH)
+        self.assertEqual(req['path'], '/%s' % (self.CONFIG_PATH,))
         self.assertEqual(req['query_params'], {'fields': 'name'})
 
     def test_exists_hit_w_alternate_client(self):
@@ -78,7 +79,7 @@ def test_exists_hit_w_alternate_client(self):
         self.assertEqual(len(conn2._requested), 1)
         req = conn2._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.CONFIG_PATH)
+        self.assertEqual(req['path'], '/%s' % (self.CONFIG_PATH,))
         self.assertEqual(req['query_params'], {'fields': 'name'})
 
     def test_reload_w_no_fields(self):
@@ -94,7 +95,7 @@ def test_reload_w_no_fields(self):
         # Name should not be overwritten if not in the response.
         self.assertEqual(self.CONFIG_NAME, config.name)
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.CONFIG_PATH)
+        self.assertEqual(req['path'], '/%s' % (self.CONFIG_PATH,))
         self._verifyResourceProperties(config, RESOURCE)
 
     def test_reload_w_bound_client(self):
@@ -108,7 +109,7 @@ def test_reload_w_bound_client(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.CONFIG_PATH)
+        self.assertEqual(req['path'], '/%s' % (self.CONFIG_PATH,))
         self._verifyResourceProperties(config, RESOURCE)
 
     def test_reload_w_alternate_client(self):
@@ -125,7 +126,7 @@ def test_reload_w_alternate_client(self):
         self.assertEqual(len(conn2._requested), 1)
         req = conn2._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.CONFIG_PATH)
+        self.assertEqual(req['path'], '/%s' % (self.CONFIG_PATH,))
         self._verifyResourceProperties(config, RESOURCE)
 
     def test_get_variable_w_bound_client(self):
@@ -153,7 +154,7 @@ def test_get_variable_w_bound_client(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % VARIABLE_PATH)
+        self.assertEqual(req['path'], '/%s' % (VARIABLE_PATH,))
 
     def test_get_variable_w_notfound(self):
         VARIABLE_NAME = 'my-variable/abcd'
@@ -191,7 +192,7 @@ def test_get_variable_w_alternate_client(self):
         self.assertEqual(len(conn2._requested), 1)
         req = conn2._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % VARIABLE_PATH)
+        self.assertEqual(req['path'], '/%s' % (VARIABLE_PATH,))
 
     def test_list_variables_empty(self):
         conn = _Connection({})
@@ -205,7 +206,7 @@ def test_list_variables_empty(self):
         self.assertEqual(req['method'], 'GET')
         PATH = 'projects/%s/configs/%s/variables' % (
             self.PROJECT, self.CONFIG_NAME)
-        self.assertEqual(req['path'], '/%s' % PATH)
+        self.assertEqual(req['path'], '/%s' % (PATH,))
 
     def test_list_variables_defaults(self):
         from google.cloud.runtimeconfig.variable import Variable
@@ -243,7 +244,7 @@ def test_list_variables_defaults(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % PATH)
+        self.assertEqual(req['path'], '/%s' % (PATH,))
         self.assertNotIn('filter', req['query_params'])
 
     def test_list_variables_explicit(self):
@@ -288,7 +289,7 @@ def test_list_variables_explicit(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % PATH)
+        self.assertEqual(req['path'], '/%s' % (PATH,))
         self.assertEqual(
             req['query_params'],
             {

runtimeconfig/unit_tests/test_variable.py

@@ -59,14 +59,15 @@ def test_ctor(self):
         variable = self._makeOne(name=self.VARIABLE_NAME, config=config)
         self.assertEqual(variable.name, self.VARIABLE_NAME)
         self.assertEqual(variable.full_name, self.PATH)
-        self.assertEqual(variable.path, '/%s' % self.PATH)
+        self.assertEqual(variable.path, '/%s' % (self.PATH,))
         self.assertIs(variable.client, client)
 
     def test_ctor_w_no_name(self):
         client = _Client(project=self.PROJECT)
         config = Config(name=self.CONFIG_NAME, client=client)
         variable = self._makeOne(name=None, config=config)
-        self.assertRaises(ValueError, lambda: variable.full_name)
+        with self.assertRaises(ValueError):
+            _ = variable.full_name
 
     def test_exists_miss_w_bound_client(self):
         conn = _Connection()
@@ -79,7 +80,7 @@ def test_exists_miss_w_bound_client(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.PATH)
+        self.assertEqual(req['path'], '/%s' % (self.PATH,))
         self.assertEqual(req['query_params'], {'fields': 'name'})
 
     def test_exists_hit_w_alternate_client(self):
@@ -96,7 +97,7 @@ def test_exists_hit_w_alternate_client(self):
         self.assertEqual(len(conn2._requested), 1)
         req = conn2._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.PATH)
+        self.assertEqual(req['path'], '/%s' % (self.PATH,))
         self.assertEqual(req['query_params'], {'fields': 'name'})
 
     def test_reload_w_bound_client(self):
@@ -116,7 +117,7 @@ def test_reload_w_bound_client(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.PATH)
+        self.assertEqual(req['path'], '/%s' % (self.PATH,))
         self._verifyResourceProperties(variable, RESOURCE)
 
     def test_reload_w_no_fields(self):
@@ -134,7 +135,7 @@ def test_reload_w_no_fields(self):
         self.assertEqual(len(conn._requested), 1)
         req = conn._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.PATH)
+        self.assertEqual(req['path'], '/%s' % (self.PATH,))
         self._verifyResourceProperties(variable, RESOURCE)
 
     def test_reload_w_alternate_client(self):
@@ -157,7 +158,7 @@ def test_reload_w_alternate_client(self):
         self.assertEqual(len(conn2._requested), 1)
         req = conn2._requested[0]
         self.assertEqual(req['method'], 'GET')
-        self.assertEqual(req['path'], '/%s' % self.PATH)
+        self.assertEqual(req['path'], '/%s' % (self.PATH,))
         self._verifyResourceProperties(variable, RESOURCE)