Merge "Deploy templates: conductor and API nits"

3 files changed, 120 insertions, 45 deletions

diff --git a/ironic/tests/unit/api/controllers/v1/test_deploy_template.py b/ironic/tests/unit/api/controllers/v1/test_deploy_template.py
index 25499589b..eae07c5d3 100644
--- a/ironic/tests/unit/api/controllers/v1/test_deploy_template.py
+++ b/ironic/tests/unit/api/controllers/v1/test_deploy_template.py
@@ -175,19 +175,16 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest):
 
     def test_detail_query_false(self):
         obj_utils.create_test_deploy_template(self.context)
-        data1 = self.get_json(
-            '/deploy_templates',
-            headers={api_base.Version.string: str(api_v1.max_version())})
+        data1 = self.get_json('/deploy_templates', headers=self.headers)
         data2 = self.get_json(
-            '/deploy_templates?detail=False',
-            headers={api_base.Version.string: str(api_v1.max_version())})
+            '/deploy_templates?detail=False', headers=self.headers)
         self.assertEqual(data1['deploy_templates'], data2['deploy_templates'])
 
     def test_detail_using_query_false_and_fields(self):
         obj_utils.create_test_deploy_template(self.context)
         data = self.get_json(
             '/deploy_templates?detail=False&fields=steps',
-            headers={api_base.Version.string: str(api_v1.max_version())})
+            headers=self.headers)
         self.assertIn('steps', data['deploy_templates'][0])
         self.assertNotIn('uuid', data['deploy_templates'][0])
         self.assertNotIn('extra', data['deploy_templates'][0])
@@ -195,8 +192,7 @@ class TestListDeployTemplates(BaseDeployTemplatesAPITest):
     def test_detail_using_query_and_fields(self):
         obj_utils.create_test_deploy_template(self.context)
         response = self.get_json(
-            '/deploy_templates?detail=True&fields=name',
-            headers={api_base.Version.string: str(api_v1.max_version())},
+            '/deploy_templates?detail=True&fields=name', headers=self.headers,
             expect_errors=True)
         self.assertEqual(http_client.BAD_REQUEST, response.status_int)
 
@@ -397,7 +393,14 @@ class TestPatch(BaseDeployTemplatesAPITest):
     def test_update_name_standard_trait(self, mock_save):
         name = 'HW_CPU_X86_VMX'
         patch = [{'path': '/name', 'value': name, 'op': 'replace'}]
-        self._test_update_ok(mock_save, patch)
+        response = self._test_update_ok(mock_save, patch)
+        self.assertEqual(name, response.json['name'])
+
+    def test_update_name_custom_trait(self, mock_save):
+        name = 'CUSTOM_DT2'
+        patch = [{'path': '/name', 'value': name, 'op': 'replace'}]
+        response = self._test_update_ok(mock_save, patch)
+        self.assertEqual(name, response.json['name'])
 
     def test_update_invalid_name(self, mock_save):
         self._test_update_bad_request(
@@ -441,12 +444,6 @@ class TestPatch(BaseDeployTemplatesAPITest):
         self.assertTrue(response.json['error_message'])
         self.assertFalse(mock_save.called)
 
-    def test_replace_singular(self, mock_save):
-        name = 'CUSTOM_DT2'
-        patch = [{'path': '/name', 'value': name, 'op': 'replace'}]
-        response = self._test_update_ok(mock_save, patch)
-        self.assertEqual(name, response.json['name'])
-
     @mock.patch.object(notification_utils, '_emit_api_notification',
                        autospec=True)
     def test_replace_name_already_exist(self, mock_notify, mock_save):
@@ -582,7 +579,7 @@ class TestPatch(BaseDeployTemplatesAPITest):
         patch = []
         for i, step in enumerate(steps):
             patch.append({'path': '/steps/%s' % i,
-                          'value': steps[i],
+                          'value': step,
                           'op': 'replace'})
         response = self.patch_json('/deploy_templates/%s' % template.uuid,
                                    patch, headers=self.headers)
diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py
index 1feed997a..8b1924257 100644
--- a/ironic/tests/unit/api/controllers/v1/test_utils.py
+++ b/ironic/tests/unit/api/controllers/v1/test_utils.py
@@ -86,6 +86,36 @@ class TestApiUtils(base.TestCase):
                                "spongebob",
                                utils.validate_trait, "invalid", "spongebob")
 
+    def test_apply_jsonpatch(self):
+        doc = {"foo": {"bar": "baz"}}
+        patch = [{"op": "add", "path": "/foo/answer", "value": 42}]
+        result = utils.apply_jsonpatch(doc, patch)
+        expected = {"foo": {"bar": "baz", "answer": 42}}
+        self.assertEqual(expected, result)
+
+    def test_apply_jsonpatch_no_add_root_attr(self):
+        doc = {}
+        patch = [{"op": "add", "path": "/foo", "value": 42}]
+        self.assertRaisesRegex(wsme.exc.ClientSideError,
+                               "Adding a new attribute",
+                               utils.apply_jsonpatch, doc, patch)
+
+    def test_apply_jsonpatch_remove_non_existent(self):
+        # Raises a KeyError.
+        doc = {}
+        patch = [{"op": "remove", "path": "/foo"}]
+        self.assertRaisesRegex(exception.PatchError,
+                               "can't remove non-existent object 'foo'",
+                               utils.apply_jsonpatch, doc, patch)
+
+    def test_apply_jsonpatch_replace_non_existent_list_item(self):
+        # Raises an IndexError.
+        doc = []
+        patch = [{"op": "replace", "path": "/0", "value": 42}]
+        self.assertRaisesRegex(exception.PatchError,
+                               "list assignment index out of range",
+                               utils.apply_jsonpatch, doc, patch)
+
     def test_get_patch_values_no_path(self):
         patch = [{'path': '/name', 'op': 'update', 'value': 'node-0'}]
         path = '/invalid'
diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py
index d49f73fee..3e39f94bb 100644
--- a/ironic/tests/unit/conductor/test_utils.py
+++ b/ironic/tests/unit/conductor/test_utils.py
@@ -1078,7 +1078,7 @@ class NodeDeployStepsTestCase(db_base.DbTestCase):
         traits = ['CUSTOM_DT1', 'CUSTOM_DT2']
         node = obj_utils.create_test_node(
             self.context, uuid=uuidutils.generate_uuid(),
-            driver='fake-hardware', instance_info={'traits': traits})
+            instance_info={'traits': traits})
         template1 = obj_utils.get_test_deploy_template(self.context)
         template2 = obj_utils.get_test_deploy_template(
             self.context, name='CUSTOM_DT2', uuid=uuidutils.generate_uuid(),
@@ -1092,15 +1092,12 @@ class NodeDeployStepsTestCase(db_base.DbTestCase):
             self.assertEqual(expected, templates)
             mock_list.assert_called_once_with(task.context, traits)
 
-    @mock.patch.object(conductor_utils, '_get_deployment_templates',
-                       autospec=True)
-    def test__get_steps_from_deployment_templates(self, mock_templates):
+    def test__get_steps_from_deployment_templates(self):
         template1 = obj_utils.get_test_deploy_template(self.context)
         template2 = obj_utils.get_test_deploy_template(
             self.context, name='CUSTOM_DT2', uuid=uuidutils.generate_uuid(),
             steps=[{'interface': 'bios', 'step': 'apply_configuration',
                     'args': {}, 'priority': 1}])
-        mock_templates.return_value = [template1, template2]
         step1 = template1.steps[0]
         step2 = template2.steps[0]
         expected = [
@@ -1119,24 +1116,25 @@ class NodeDeployStepsTestCase(db_base.DbTestCase):
         ]
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
-            steps = conductor_utils._get_steps_from_deployment_templates(task)
+            steps = conductor_utils._get_steps_from_deployment_templates(
+                task, [template1, template2])
             self.assertEqual(expected, steps)
-            mock_templates.assert_called_once_with(task)
 
-    @mock.patch.object(conductor_utils, '_get_steps_from_deployment_templates',
+    @mock.patch.object(conductor_utils, '_get_validated_steps_from_templates',
                        autospec=True)
     @mock.patch.object(conductor_utils, '_get_deployment_steps', autospec=True)
     def _test__get_all_deployment_steps(self, user_steps, driver_steps,
-                                        expected_steps, mock_gds, mock_gsfdt):
-        mock_gsfdt.return_value = user_steps
-        mock_gds.return_value = driver_steps
+                                        expected_steps, mock_steps,
+                                        mock_validated):
+        mock_validated.return_value = user_steps
+        mock_steps.return_value = driver_steps
 
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
             steps = conductor_utils._get_all_deployment_steps(task)
             self.assertEqual(expected_steps, steps)
-            mock_gsfdt.assert_called_once_with(task)
-            mock_gds.assert_called_once_with(task, enabled=True, sort=False)
+            mock_validated.assert_called_once_with(task)
+            mock_steps.assert_called_once_with(task, enabled=True, sort=False)
 
     def test__get_all_deployment_steps_no_steps(self):
         # Nothing in -> nothing out.
@@ -1206,6 +1204,19 @@ class NodeDeployStepsTestCase(db_base.DbTestCase):
         self._test__get_all_deployment_steps(user_steps, driver_steps,
                                              expected_steps)
 
+    @mock.patch.object(conductor_utils, '_get_validated_steps_from_templates',
+                       autospec=True)
+    @mock.patch.object(conductor_utils, '_get_deployment_steps', autospec=True)
+    def test__get_all_deployment_steps_error(self, mock_steps, mock_validated):
+        mock_validated.side_effect = exception.InvalidParameterValue('foo')
+
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+            self.assertRaises(exception.InvalidParameterValue,
+                              conductor_utils._get_all_deployment_steps, task)
+            mock_validated.assert_called_once_with(task)
+            self.assertFalse(mock_steps.called)
+
     @mock.patch.object(conductor_utils, '_get_all_deployment_steps',
                        autospec=True)
     def test_set_node_deployment_steps(self, mock_steps):
@@ -1280,7 +1291,7 @@ class NodeDeployStepsTestCase(db_base.DbTestCase):
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             self.assertRaisesRegex(exception.InvalidParameterValue,
-                                   "power_one.*invalid.*arg1",
+                                   "power_one.*unexpected.*arg1",
                                    conductor_utils._validate_user_deploy_steps,
                                    task, user_steps)
             mock_steps.assert_called_once_with(task, enabled=False, sort=False)
@@ -1356,7 +1367,7 @@ class NodeDeployStepsTestCase(db_base.DbTestCase):
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             self.assertRaisesRegex(exception.InvalidParameterValue,
-                                   "duplicate deploy steps for "
+                                   "Duplicate deploy steps for "
                                    "power.power_one",
                                    conductor_utils._validate_user_deploy_steps,
                                    task, user_steps)
@@ -1560,7 +1571,7 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
 
         with task_manager.acquire(self.context, node.uuid) as task:
             self.assertRaisesRegex(exception.InvalidParameterValue,
-                                   "update_firmware.*invalid.*arg1",
+                                   "update_firmware.*unexpected.*arg1",
                                    conductor_utils._validate_user_clean_steps,
                                    task, user_steps)
             mock_steps.assert_called_once_with(task, enabled=False, sort=False)
@@ -2455,37 +2466,74 @@ class ValidateInstanceInfoTraitsTestCase(tests_base.TestCase):
                                self.node)
 
 
+@mock.patch.object(conductor_utils, '_get_deployment_templates',
+                   autospec=True)
 @mock.patch.object(conductor_utils, '_get_steps_from_deployment_templates',
                    autospec=True)
 @mock.patch.object(conductor_utils, '_validate_user_deploy_steps',
                    autospec=True)
-class ValidateDeployTemplatesTestCase(db_base.DbTestCase):
+class GetValidatedStepsFromTemplatesTestCase(db_base.DbTestCase):
 
     def setUp(self):
-        super(ValidateDeployTemplatesTestCase, self).setUp()
+        super(GetValidatedStepsFromTemplatesTestCase, self).setUp()
         self.node = obj_utils.create_test_node(self.context,
                                                driver='fake-hardware')
+        self.template = obj_utils.get_test_deploy_template(self.context)
 
-    def test_validate_deploy_templates(self, mock_validate, mock_get):
+    def test_ok(self, mock_validate, mock_steps, mock_templates):
+        mock_templates.return_value = [self.template]
         steps = [db_utils.get_test_deploy_template_step()]
-        mock_get.return_value = steps
+        mock_steps.return_value = steps
+        mock_validate.return_value = steps
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
-            conductor_utils.validate_deploy_templates(task)
-            mock_validate.assert_called_once_with(task, steps)
+            result = conductor_utils._get_validated_steps_from_templates(task)
+            self.assertEqual(steps, result)
+            mock_templates.assert_called_once_with(task)
+            mock_steps.assert_called_once_with(task, [self.template])
+            mock_validate.assert_called_once_with(task, steps, mock.ANY)
 
-    def test_validate_deploy_templates_invalid_parameter_value(
-            self, mock_validate, mock_get):
+    def test_invalid_parameter_value(self, mock_validate, mock_steps,
+                                     mock_templates):
+        mock_templates.return_value = [self.template]
         mock_validate.side_effect = exception.InvalidParameterValue('fake')
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
-            self.assertRaises(exception.InvalidParameterValue,
-                              conductor_utils.validate_deploy_templates, task)
+            self.assertRaises(
+                exception.InvalidParameterValue,
+                conductor_utils._get_validated_steps_from_templates, task)
 
-    def test_validate_deploy_templates_instance_deploy_failure(
-            self, mock_validate, mock_get):
+    def test_instance_deploy_failure(self, mock_validate, mock_steps,
+                                     mock_templates):
+        mock_templates.return_value = [self.template]
         mock_validate.side_effect = exception.InstanceDeployFailure('foo')
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
-            self.assertRaises(exception.InstanceDeployFailure,
+            self.assertRaises(
+                exception.InstanceDeployFailure,
+                conductor_utils._get_validated_steps_from_templates, task)
+
+
+@mock.patch.object(conductor_utils, '_get_validated_steps_from_templates',
+                   autospec=True)
+class ValidateDeployTemplatesTestCase(db_base.DbTestCase):
+
+    def setUp(self):
+        super(ValidateDeployTemplatesTestCase, self).setUp()
+        self.node = obj_utils.create_test_node(self.context,
+                                               driver='fake-hardware')
+
+    def test_ok(self, mock_validated):
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+            result = conductor_utils.validate_deploy_templates(task)
+            self.assertIsNone(result)
+            mock_validated.assert_called_once_with(task)
+
+    def test_error(self, mock_validated):
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+            mock_validated.side_effect = exception.InvalidParameterValue('foo')
+            self.assertRaises(exception.InvalidParameterValue,
                               conductor_utils.validate_deploy_templates, task)
+            mock_validated.assert_called_once_with(task)