Remove support for parameters section

It hasn't been recommended to pass settings in via the parameters
section for quite a while now, and in 2.0 let's remove it completely.
This simplifies a lot of the logic around id and role processing.

openstack_virtual_baremetal/build_nodes_json.py

@@ -71,13 +71,6 @@ def _parse_args():
     return args
 
 
-def _get_from_env(env, name):
-    try:
-        return env['parameters'][name]
-    except KeyError:
-        return env['parameter_defaults'][name]
-
-
 def _get_names(args):
     if args.env is None:
         bmc_base = args.bmc_prefix
@@ -87,8 +80,8 @@ def _get_names(args):
     else:
         with open(args.env) as f:
             e = yaml.safe_load(f)
-        bmc_base = _get_from_env(e, 'bmc_prefix')
-        baremetal_base = _get_from_env(e, 'baremetal_prefix')
+        bmc_base = e['parameter_defaults']['bmc_prefix']
+        baremetal_base = e['parameter_defaults']['baremetal_prefix']
         role = e.get('parameter_defaults', {}).get('role')
         if role and baremetal_base.endswith('-' + role):
             baremetal_base = baremetal_base[:-len(role) - 1]

openstack_virtual_baremetal/deploy.py

@@ -103,32 +103,16 @@ def _process_args(args):
 def _add_identifier(env_data, name, identifier, default=None):
     """Append identifier to the end of parameter name in env_data
 
-    Look for ``name`` in either the ``parameters`` or ``parameter_defaults``
-    key of ``env_data`` and append '-``identifier``' to it.
+    Look for ``name`` in the ``parameter_defaults`` key of ``env_data`` and
+    append '-``identifier``' to it.
     """
-    # We require both sections for id environments
-    if not env_data.get('parameters'):
-        env_data['parameters'] = {}
-    if not env_data.get('parameter_defaults'):
-        env_data['parameter_defaults'] = {}
-    parameter = False
-    try:
-        value = env_data['parameters'][name]
-        parameter = True
-    except KeyError:
-        value = env_data['parameter_defaults'].get(name)
+    value = env_data['parameter_defaults'].get(name)
     if value is None:
         value = default
     if value is None:
         raise RuntimeError('No base value found when adding id')
     if identifier:
         value = '%s-%s' % (value, identifier)
-
-    # If it was passed in as a parameter we need to set it in the parameters
-    # section or it will be overridden by the original value.  We can't always
-    # do that though because some parameters are not exposed at the top-level.
-    if parameter:
-        env_data['parameters'][name] = value
     env_data['parameter_defaults'][name] = value
 
 
@@ -201,10 +185,7 @@ def _validate_env(args, env_paths):
     if not args.id:
         env_data = _build_env_data(env_paths)
         role = env_data.get('parameter_defaults', {}).get('role')
-        try:
-            prefix = env_data['parameters']['baremetal_prefix']
-        except KeyError:
-            prefix = env_data['parameter_defaults']['baremetal_prefix']
+        prefix = env_data['parameter_defaults']['baremetal_prefix']
         if role and prefix.endswith('-' + role):
             raise RuntimeError('baremetal_prefix ends with role name.  This '
                                'will break build-nodes-json.  Please choose '
@@ -312,15 +293,15 @@ def _process_role(role_file, base_envs, stack_name, args):
     allowed_registry_keys = ['OS::OVB::BaremetalPorts', 'OS::OVB::BMCPort',
                              'OS::OVB::UndercloudNetworks',
                              ]
+    # NOTE(bnemec): Not sure what purpose this serves. Can probably be removed.
     role_env = role_data
     # resource_registry is intentionally omitted as it should not be inherited
-    for section in ['parameters', 'parameter_defaults']:
-        role_env.setdefault(section, {}).update({
-            k: v for k, v in base_data.get(section, {}).items()
-            if k in inherited_keys and
-            (k not in role_env.get(section, {}) or
-             k not in allowed_parameter_keys)
-        })
+    role_env.setdefault('parameter_defaults', {}).update({
+        k: v for k, v in base_data.get('parameter_defaults', {}).items()
+        if k in inherited_keys and
+        (k not in role_env.get('parameter_defaults', {}) or
+         k not in allowed_parameter_keys)
+    })
     # Most of the resource_registry should not be included in role envs.
     # Only allow specific entries that may be needed.
     role_env.setdefault('resource_registry', {})
@@ -333,24 +314,20 @@ def _process_role(role_file, base_envs, stack_name, args):
         if k not in role_reg and k in base_reg:
             role_reg[k] = base_reg[k]
     # We need to start with the unmodified prefix
-    try:
-        base_prefix = orig_data['parameters']['baremetal_prefix']
-    except KeyError:
-        base_prefix = orig_data['parameter_defaults']['baremetal_prefix']
+    base_prefix = orig_data['parameter_defaults']['baremetal_prefix']
     # But we do need to add the id if one is in use
     if args.id:
         base_prefix += '-%s' % args.id
-    try:
-        bmc_prefix = base_data['parameters']['bmc_prefix']
-    except KeyError:
-        bmc_prefix = base_data['parameter_defaults']['bmc_prefix']
+    bmc_prefix = base_data['parameter_defaults']['bmc_prefix']
     role = role_data['parameter_defaults']['role']
     if '_' in role:
         raise RuntimeError('_ character not allowed in role name "%s".' % role)
-    role_env['parameters']['baremetal_prefix'] = '%s-%s' % (base_prefix, role)
-    role_env['parameters']['bmc_prefix'] = '%s-%s' % (bmc_prefix, role)
+    role_env['parameter_defaults']['baremetal_prefix'] = ('%s-%s' %
+                                                          (base_prefix, role))
+    role_env['parameter_defaults']['bmc_prefix'] = '%s-%s' % (bmc_prefix, role)
     # At this time roles are only attached to a single set of networks, so
     # we use just the primary network parameters.
+
     def maybe_add_id(role_env, name, args):
         """Add id only if one is not already present
 
@@ -364,6 +341,7 @@ def _process_role(role_file, base_envs, stack_name, args):
                 not role_env['parameter_defaults'].get(name, '')
                                                   .endswith('-' + args.id)):
             _add_identifier(role_env, name, args.id)
+
     maybe_add_id(role_env, 'provision_net', args)
     maybe_add_id(role_env, 'overcloud_internal_net', args)
     maybe_add_id(role_env, 'overcloud_storage_net', args)

openstack_virtual_baremetal/tests/test_build_nodes_json.py

@@ -89,26 +89,6 @@ class TestBuildNodesJson(testtools.TestCase):
         self.assertEqual('baremetal-foo', baremetal_base)
         self.assertEqual('undercloud', undercloud_name)
 
-    @mock.patch('openstack_virtual_baremetal.build_nodes_json.open',
-                create=True)
-    @mock.patch('yaml.safe_load')
-    def test_get_names_old_env(self, mock_load, mock_open):
-        args = mock.Mock()
-        args.env = 'foo.yaml'
-        args.add_undercloud = False
-        mock_env = {
-            'parameters': {
-                'bmc_prefix': 'bmc-foo',
-                'baremetal_prefix': 'baremetal-foo',
-            },
-        }
-        mock_load.return_value = mock_env
-        bmc_base, baremetal_base, undercloud_name = (
-            build_nodes_json._get_names(args))
-        self.assertEqual('bmc-foo', bmc_base)
-        self.assertEqual('baremetal-foo', baremetal_base)
-        self.assertIsNone(undercloud_name)
-
     @mock.patch('openstack_virtual_baremetal.build_nodes_json.open',
                 create=True)
     @mock.patch('yaml.safe_load')

openstack_virtual_baremetal/tests/test_deploy.py

@@ -89,14 +89,13 @@ class TestProcessArgs(unittest.TestCase):
         self.assertRaises(ValueError, deploy._process_args, mock_args)
 
 
-test_env = u"""parameters:
+test_env = u"""parameter_defaults:
   provision_net: provision
   public_net: public
   baremetal_prefix: baremetal
   bmc_prefix: bmc
 """
-test_env_param_defaults = u"""
-parameter_defaults:
+test_env_extra = u"""
   overcloud_internal_net: internalapi
   role: ''
 """
@@ -115,21 +114,13 @@ test_env_output = {
 
 class TestIdEnv(unittest.TestCase):
     def test_add_identifier(self):
-        env_data = {'parameters': {'foo': 'bar'}}
-        deploy._add_identifier(env_data, 'foo', 'baz')
-        self.assertEqual('bar-baz', env_data['parameters']['foo'])
-        self.assertEqual('bar-baz', env_data['parameter_defaults']['foo'])
-
-    def test_add_identifier_defaults(self):
         env_data = {'parameter_defaults': {'foo': 'bar'}}
         deploy._add_identifier(env_data, 'foo', 'baz')
-        self.assertNotIn('foo', env_data['parameters'])
         self.assertEqual('bar-baz', env_data['parameter_defaults']['foo'])
 
     def test_add_identifier_different_section(self):
         env_data = {'parameter_defaults': {'foo': 'bar'}}
         deploy._add_identifier(env_data, 'foo', 'baz')
-        self.assertNotIn('foo', env_data['parameters'])
         self.assertEqual('bar-baz', env_data['parameter_defaults']['foo'])
 
     @mock.patch('openstack_virtual_baremetal.deploy._build_env_data')
@@ -138,14 +129,11 @@ class TestIdEnv(unittest.TestCase):
         mock_args = mock.Mock()
         mock_args.id = 'foo'
         mock_args.env = ['foo.yaml']
-        env = test_env + 'parameter_defaults:'
-        mock_bed.return_value = yaml.safe_load(env)
+        mock_bed.return_value = yaml.safe_load(test_env)
         path = deploy._generate_id_env(mock_args)
         self.assertEqual(['foo.yaml', 'env-foo.yaml'], path)
         dumped_dict = mock_safe_dump.call_args_list[0][0][0]
         for k, v in test_env_output.items():
-            if k in mock_bed.return_value['parameters']:
-                self.assertEqual(v, dumped_dict['parameters'][k])
             self.assertEqual(v, dumped_dict['parameter_defaults'][k])
 
     @mock.patch('openstack_virtual_baremetal.deploy._build_env_data')
@@ -154,7 +142,7 @@ class TestIdEnv(unittest.TestCase):
         mock_args = mock.Mock()
         mock_args.id = 'foo'
         mock_args.env = ['foo.yaml']
-        env = (test_env + test_env_param_defaults +
+        env = (test_env + test_env_extra +
                '  undercloud_name: test-undercloud\n')
         mock_bed.return_value = yaml.safe_load(env)
         env_output = dict(test_env_output)
@@ -164,8 +152,6 @@ class TestIdEnv(unittest.TestCase):
         self.assertEqual(['foo.yaml', 'env-foo.yaml'], path)
         dumped_dict = mock_safe_dump.call_args_list[0][0][0]
         for k, v in env_output.items():
-            if k in mock_bed.return_value['parameters']:
-                self.assertEqual(v, dumped_dict['parameters'][k])
             self.assertEqual(v, dumped_dict['parameter_defaults'][k])
 
     @mock.patch('openstack_virtual_baremetal.deploy._build_env_data')
@@ -174,7 +160,7 @@ class TestIdEnv(unittest.TestCase):
         mock_args = mock.Mock()
         mock_args.id = 'foo'
         mock_args.env = ['foo.yaml']
-        env = (test_env + test_env_param_defaults)
+        env = (test_env + test_env_extra)
         mock_bed.return_value = yaml.safe_load(env)
         mock_bed.return_value['parameter_defaults']['role'] = 'compute'
         env_output = dict(test_env_output)
@@ -184,8 +170,6 @@ class TestIdEnv(unittest.TestCase):
         self.assertEqual(['foo.yaml', 'env-foo.yaml'], path)
         dumped_dict = mock_safe_dump.call_args_list[0][0][0]
         for k, v in env_output.items():
-            if k in mock_bed.return_value['parameters']:
-                self.assertEqual(v, dumped_dict['parameters'][k])
             self.assertEqual(v, dumped_dict['parameter_defaults'][k])
 
 
@@ -200,8 +184,6 @@ role_base_data = {
         'public_net': 'public-foo',
         'private_net': 'private',
         'role': 'control',
-    },
-    'parameters': {
         'os_user': 'admin',
         'key_name': 'default',
         'undercloud_name': 'undercloud-foo',
@@ -229,10 +211,9 @@ role_base_data = {
 role_specific_data = {
     'parameter_defaults': {
         'role': 'compute',
-    },
-    'parameters': {
         'key_name': 'default',
         'baremetal_flavor': 'baremetal',
+        'baremetal_image': 'centos',
         'bmc_image': 'bmc-base',
         'bmc_prefix': 'bmc',
         'node_count': 2,
@@ -250,8 +231,6 @@ role_original_data = {
         'public_net': 'public',
         'private_net': 'private',
         'provision_net': 'provision',
-    },
-    'parameters': {
         'os_user': 'admin',
         'key_name': 'default',
         'undercloud_name': 'undercloud',
@@ -288,8 +267,8 @@ class TestDeploy(testtools.TestCase):
             template_files, template
         )
         env_files = {'templates/resource_registry.yaml': {'bar': 'baz'},
-                     'env.yaml': {'parameters': {}}}
-        env = {'parameters': {}}
+                     'env.yaml': {'parameter_defaults': {}}}
+        env = {'parameter_defaults': {}}
         mock_tu.process_multiple_environments_and_files.return_value = (
             env_files, env
         )
@@ -393,11 +372,10 @@ class TestDeploy(testtools.TestCase):
         output = mock_write.call_args[0][0]
         # These values are computed in _process_role
         self.assertEqual('baremetal-foo-compute',
-                         output['parameters']['baremetal_prefix'])
+                         output['parameter_defaults']['baremetal_prefix'])
         self.assertEqual('bmc-foo-compute',
-                         output['parameters']['bmc_prefix'])
+                         output['parameter_defaults']['bmc_prefix'])
         # These should be inherited
-        self.assertEqual('ipxe-boot', output['parameters']['baremetal_image'])
         self.assertEqual('tenant-' + args.id,
                          output['parameter_defaults']['overcloud_tenant_net'])
         self.assertEqual('internal-' + args.id,
@@ -408,6 +386,9 @@ class TestDeploy(testtools.TestCase):
         self.assertEqual('storage_mgmt-' + args.id,
                          output['parameter_defaults'][
                              'overcloud_storage_mgmt_net'])
+        # This parameter should be overrideable
+        self.assertEqual('centos',
+                         output['parameter_defaults']['baremetal_image'])
         # This should not be present in a role env, even if set in the file
         self.assertNotIn('OS::OVB::BaremetalNetworks',
                          output['resource_registry'])
@@ -419,39 +400,6 @@ class TestDeploy(testtools.TestCase):
         self.assertEqual('templates/bmc-port-port-security.yaml',
                          output['resource_registry']['OS::OVB::BMCPort'])
 
-    @mock.patch('openstack_virtual_baremetal.deploy._write_role_file')
-    @mock.patch('openstack_virtual_baremetal.deploy._load_role_data')
-    def test_process_role_param_defaults(self, mock_load, mock_write):
-        def move_params_to_param_defaults(d):
-            data = copy.deepcopy(d)
-            for k, v in data['parameters'].items():
-                data['parameter_defaults'][k] = v
-            data.pop('parameters', None)
-            return data
-
-        pd_base_data = move_params_to_param_defaults(role_base_data)
-        pd_specific_data = move_params_to_param_defaults(role_specific_data)
-        pd_original_data = move_params_to_param_defaults(role_original_data)
-        pd_specific_data['parameter_defaults']['baremetal_image'] = 'centos'
-        mock_load.return_value = (pd_base_data, pd_specific_data,
-                                  pd_original_data)
-        args = mock.Mock()
-        args.id = 'foo'
-        role_file, role = deploy._process_role('foo-compute.yaml', 'foo.yaml',
-                                               'foo', args)
-        mock_load.assert_called_once_with('foo.yaml', 'foo-compute.yaml', args)
-        self.assertEqual('env-foo-compute.yaml', role_file)
-        self.assertEqual('compute', role)
-        output = mock_write.call_args[0][0]
-        # These values are computed in _process_role
-        self.assertEqual('baremetal-foo-compute',
-                         output['parameters']['baremetal_prefix'])
-        self.assertEqual('bmc-foo-compute',
-                         output['parameters']['bmc_prefix'])
-        # This parameter should be inherited (as tested above) but overrideable
-        self.assertEqual('centos',
-                         output['parameter_defaults']['baremetal_image'])
-
     @mock.patch('openstack_virtual_baremetal.deploy._load_role_data')
     def test_process_role_invalid_name(self, mock_load):
         bad_role_specific_data = copy.deepcopy(role_specific_data)
@@ -484,10 +432,10 @@ class TestDeploy(testtools.TestCase):
         deploy._deploy_roles('foo', args, 'foo.yaml')
         mock_process.assert_not_called()
 
-    def _test_validate_env_ends_with_profile(self, mock_id, mock_bed,
-                                             section='parameters'):
+    def _test_validate_env_ends_with_profile(self, mock_id, mock_bed):
         test_env = dict(role_original_data)
-        test_env[section]['baremetal_prefix'] = 'baremetal-control'
+        test_env['parameter_defaults']['baremetal_prefix'] = (
+            'baremetal-control')
         mock_bed.return_value = test_env
         args = mock.Mock()
         args.id = mock_id
@@ -501,11 +449,6 @@ class TestDeploy(testtools.TestCase):
     def test_validate_env_fails(self, mock_bed):
         self._test_validate_env_ends_with_profile(None, mock_bed)
 
-    @mock.patch('openstack_virtual_baremetal.deploy._build_env_data')
-    def test_validate_env_fails_param_defaults(self, mock_bed):
-        self._test_validate_env_ends_with_profile(None, mock_bed,
-                                                  'parameter_defaults')
-
     @mock.patch('openstack_virtual_baremetal.deploy._build_env_data')
     def test_validate_env_with_id(self, mock_bed):
         self._test_validate_env_ends_with_profile('foo', mock_bed)