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)