From a7ec465031c3429acfb91e5e4e105726dea607c7 Mon Sep 17 00:00:00 2001 From: Steven Hardy Date: Mon, 21 Sep 2015 18:25:08 +0100 Subject: [PATCH] Don't pass defaults as parameters for TemplateResource The child_params calculation doesn't differentiate parameters provided via the properties vs those provided via the template defaults (defined in the nested stack template), which leads to confusing results when you look at the parameters for the nested stack, as the defaults are exposed as parameters passed in to the stack, when the parent didn't pass anything via properties. Closes-Bug: #1498107 Change-Id: Ic58cdcd15db09cd8d8d3d52fadfb68aa5709e7d8 --- heat/engine/properties.py | 13 +++++++++---- heat/engine/resources/template_resource.py | 2 +- heat/tests/test_properties.py | 16 ++++++++++++++++ heat/tests/test_provider_template.py | 9 +++++---- 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/heat/engine/properties.py b/heat/engine/properties.py index 8376f8caf2..1147005ef9 100644 --- a/heat/engine/properties.py +++ b/heat/engine/properties.py @@ -413,12 +413,11 @@ class Properties(collections.Mapping): if any(res.action == res.INIT for res in deps): return True - def _get_property_value(self, key, validate=False): + def get_user_value(self, key, validate=False): if key not in self: raise KeyError(_('Invalid Property %s') % key) prop = self.props[key] - if key in self.data: try: unresolved_value = self.data[key] @@ -438,12 +437,18 @@ class Properties(collections.Mapping): # so handle this generically except Exception as e: raise ValueError(six.text_type(e)) + + def _get_property_value(self, key, validate=False): + if key not in self: + raise KeyError(_('Invalid Property %s') % key) + + prop = self.props[key] + if key in self.data: + return self.get_user_value(key, validate) elif prop.has_default(): return prop.get_value(None, validate) elif prop.required(): raise ValueError(_('Property %s not assigned') % key) - else: - return None def __getitem__(self, key): return self._get_property_value(key) diff --git a/heat/engine/resources/template_resource.py b/heat/engine/resources/template_resource.py index 556369e8cb..e3396aad72 100644 --- a/heat/engine/resources/template_resource.py +++ b/heat/engine/resources/template_resource.py @@ -133,7 +133,7 @@ class TemplateResource(stack_resource.StackResource): continue try: - val = self.properties[pname] + val = self.properties.get_user_value(pname) except ValueError: if self.action == self.INIT: prop = self.properties.props[pname] diff --git a/heat/tests/test_properties.py b/heat/tests/test_properties.py index 4d6ae1fa22..b478749432 100644 --- a/heat/tests/test_properties.py +++ b/heat/tests/test_properties.py @@ -1038,9 +1038,25 @@ class PropertiesTest(common.HeatTestCase): def test_default_override(self): self.assertEqual(42, self.props['default_override']) + def test_get_user_value(self): + self.assertIsNone(self.props.get_user_value('defaulted')) + self.assertEqual(42, self.props.get_user_value('default_override')) + + def test_get_user_value_key_error(self): + ex = self.assertRaises(KeyError, self.props.get_user_value, 'foo') + # Note we have to use args here: https://bugs.python.org/issue2651 + self.assertEqual('Invalid Property foo', + six.text_type(ex.args[0])) + def test_bad_key(self): self.assertEqual('wibble', self.props.get('foo', 'wibble')) + def test_key_error(self): + ex = self.assertRaises(KeyError, self.props.__getitem__, 'foo') + # Note we have to use args here: https://bugs.python.org/issue2651 + self.assertEqual('Invalid Property foo', + six.text_type(ex.args[0])) + def test_none_string(self): schema = {'foo': {'Type': 'String'}} props = properties.Properties(schema, {'foo': None}) diff --git a/heat/tests/test_provider_template.py b/heat/tests/test_provider_template.py index 9a0e585023..72ccedc063 100644 --- a/heat/tests/test_provider_template.py +++ b/heat/tests/test_provider_template.py @@ -178,7 +178,8 @@ class ProviderTemplateTest(common.HeatTestCase): # verify Map conversion self.assertEqual(map_prop_val, converted_params.get("AMap")) - with mock.patch.object(properties.Properties, '__getitem__') as m_get: + with mock.patch.object(properties.Properties, + 'get_user_value') as m_get: m_get.side_effect = ValueError('boom') # If the property doesn't exist on INIT, return default value @@ -984,7 +985,7 @@ class TemplateResourceCrudTest(common.HeatTestCase): self.res.handle_create() self.res.create_with_template.assert_called_once_with( - self.provider, {'Foo': 'bar', 'Blarg': 'wibble'}) + self.provider, {'Foo': 'bar'}) def test_handle_adopt(self): self.res.create_with_template = mock.Mock(return_value=None) @@ -992,7 +993,7 @@ class TemplateResourceCrudTest(common.HeatTestCase): self.res.handle_adopt(resource_data={'resource_id': 'fred'}) self.res.create_with_template.assert_called_once_with( - self.provider, {'Foo': 'bar', 'Blarg': 'wibble'}, + self.provider, {'Foo': 'bar'}, adopt_data={'resource_id': 'fred'}) def test_handle_update(self): @@ -1001,7 +1002,7 @@ class TemplateResourceCrudTest(common.HeatTestCase): self.res.handle_update(self.defn, None, None) self.res.update_with_template.assert_called_once_with( - self.provider, {'Foo': 'bar', 'Blarg': 'wibble'}) + self.provider, {'Foo': 'bar'}) def test_handle_delete(self): self.res.rpc_client = mock.MagicMock()