From 8eef21a7bbfdcba709515529a40fadc1f5386b70 Mon Sep 17 00:00:00 2001 From: Ondrej Mular Date: Fri, 8 Jul 2016 16:43:16 +0200 Subject: [PATCH 1/2] lib: use recipient id as identifier instead of its value --- pcs/common/report_codes.py | 3 +- pcs/lib/cib/alert.py | 129 +++++++---- pcs/lib/cib/test/test_alert.py | 449 +++++++++++++++++++++++++++++++----- pcs/lib/commands/alert.py | 45 +++- pcs/lib/commands/test/test_alert.py | 111 ++++++--- pcs/lib/reports.py | 29 ++- 6 files changed, 597 insertions(+), 169 deletions(-) diff --git a/pcs/common/report_codes.py b/pcs/common/report_codes.py index 2b39938..53f2ccb 100644 --- a/pcs/common/report_codes.py +++ b/pcs/common/report_codes.py @@ -7,6 +7,7 @@ from __future__ import ( # force cathegories FORCE_ACTIVE_RRP = "ACTIVE_RRP" +FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE = "FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE" FORCE_CONSTRAINT_DUPLICATE = "CONSTRAINT_DUPLICATE" FORCE_CONSTRAINT_MULTIINSTANCE_RESOURCE = "CONSTRAINT_MULTIINSTANCE_RESOURCE" FORCE_LOAD_THRESHOLD = "LOAD_THRESHOLD" @@ -22,7 +23,7 @@ AGENT_NOT_FOUND = "AGENT_NOT_FOUND" BAD_CLUSTER_STATE_FORMAT = 'BAD_CLUSTER_STATE_FORMAT' CIB_ALERT_NOT_FOUND = "CIB_ALERT_NOT_FOUND" CIB_ALERT_RECIPIENT_ALREADY_EXISTS = "CIB_ALERT_RECIPIENT_ALREADY_EXISTS" -CIB_ALERT_RECIPIENT_NOT_FOUND = "CIB_ALERT_RECIPIENT_NOT_FOUND" +CIB_ALERT_RECIPIENT_VALUE_INVALID = "CIB_ALERT_RECIPIENT_VALUE_INVALID" CIB_CANNOT_FIND_MANDATORY_SECTION = "CIB_CANNOT_FIND_MANDATORY_SECTION" CIB_LOAD_ERROR_BAD_FORMAT = "CIB_LOAD_ERROR_BAD_FORMAT" CIB_LOAD_ERROR = "CIB_LOAD_ERROR" diff --git a/pcs/lib/cib/alert.py b/pcs/lib/cib/alert.py index 6b72996..b5fe88c 100644 --- a/pcs/lib/cib/alert.py +++ b/pcs/lib/cib/alert.py @@ -7,14 +7,16 @@ from __future__ import ( from lxml import etree +from pcs.common import report_codes from pcs.lib import reports -from pcs.lib.errors import LibraryError +from pcs.lib.errors import LibraryError, ReportItemSeverity as Severities from pcs.lib.cib.nvpair import update_nvset, get_nvset from pcs.lib.cib.tools import ( check_new_id_applicable, get_sub_element, find_unique_id, get_alerts, + validate_id_does_not_exist, ) @@ -61,7 +63,7 @@ def _update_optional_attribute(element, attribute, value): def get_alert_by_id(tree, alert_id): """ Returns alert element with specified id. - Raises AlertNotFound if alert with specified id doesn't exist. + Raises LibraryError if alert with specified id doesn't exist. tree -- cib etree node alert_id -- id of alert @@ -72,25 +74,53 @@ def get_alert_by_id(tree, alert_id): return alert -def get_recipient(alert, recipient_value): +def get_recipient_by_id(tree, recipient_id): """ Returns recipient element with value recipient_value which belong to specified alert. - Raises RecipientNotFound if recipient doesn't exist. + Raises LibraryError if recipient doesn't exist. - alert -- parent element of required recipient - recipient_value -- value of recipient + tree -- cib etree node + recipient_id -- id of recipient """ - recipient = alert.find( - "./recipient[@value='{0}']".format(recipient_value) + recipient = get_alerts(tree).find( + "./alert/recipient[@id='{0}']".format(recipient_id) ) if recipient is None: - raise LibraryError(reports.cib_alert_recipient_not_found( - alert.get("id"), recipient_value - )) + raise LibraryError(reports.id_not_found(recipient_id, "Recipient")) return recipient +def ensure_recipient_value_is_unique( + reporter, alert, recipient_value, recipient_id="", allow_duplicity=False +): + """ + Ensures that recipient_value is unique in alert. + + reporter -- report processor + alert -- alert + recipient_value -- recipient value + recipient_id -- recipient id of to which value belongs to + allow_duplicity -- if True only warning will be shown if value already + exists + """ + recipient_list = alert.xpath( + "./recipient[@value='{value}' and @id!='{id}']".format( + value=recipient_value, id=recipient_id + ) + ) + if recipient_list: + reporter.process(reports.cib_alert_recipient_already_exists( + alert.get("id", None), + recipient_value, + Severities.WARNING if allow_duplicity else Severities.ERROR, + forceable=( + None if allow_duplicity + else report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE + ) + )) + + def create_alert(tree, alert_id, path, description=""): """ Create new alert element. Returns newly created element. @@ -116,7 +146,7 @@ def create_alert(tree, alert_id, path, description=""): def update_alert(tree, alert_id, path, description=None): """ Update existing alert. Return updated alert element. - Raises AlertNotFound if alert with specified id doesn't exist. + Raises LibraryError if alert with specified id doesn't exist. tree -- cib etree node alert_id -- id of alert to be updated @@ -134,7 +164,7 @@ def update_alert(tree, alert_id, path, description=None): def remove_alert(tree, alert_id): """ Remove alert with specified id. - Raises AlertNotFound if alert with specified id doesn't exist. + Raises LibraryError if alert with specified id doesn't exist. tree -- cib etree node alert_id -- id of alert which should be removed @@ -144,36 +174,38 @@ def remove_alert(tree, alert_id): def add_recipient( + reporter, tree, alert_id, recipient_value, - description="" + recipient_id=None, + description="", + allow_same_value=False ): """ Add recipient to alert with specified id. Returns added recipient element. - Raises AlertNotFound if alert with specified id doesn't exist. + Raises LibraryError if alert with specified recipient_id doesn't exist. Raises LibraryError if recipient already exists. + reporter -- report processor tree -- cib etree node alert_id -- id of alert which should be parent of new recipient recipient_value -- value of recipient + recipient_id -- id of new recipient, if None it will be generated description -- description of recipient + allow_same_value -- if True unique recipient value is not required """ - alert = get_alert_by_id(tree, alert_id) + if recipient_id is None: + recipient_id = find_unique_id(tree, "{0}-recipient".format(alert_id)) + else: + validate_id_does_not_exist(tree, recipient_id) - recipient = alert.find( - "./recipient[@value='{0}']".format(recipient_value) + alert = get_alert_by_id(tree, alert_id) + ensure_recipient_value_is_unique( + reporter, alert, recipient_value, allow_duplicity=allow_same_value ) - if recipient is not None: - raise LibraryError(reports.cib_alert_recipient_already_exists( - alert_id, recipient_value - )) - recipient = etree.SubElement( - alert, - "recipient", - id=find_unique_id(tree, "{0}-recipient".format(alert_id)), - value=recipient_value + alert, "recipient", id=recipient_id, value=recipient_value ) if description: @@ -182,38 +214,49 @@ def add_recipient( return recipient -def update_recipient(tree, alert_id, recipient_value, description): +def update_recipient( + reporter, + tree, + recipient_id, + recipient_value=None, + description=None, + allow_same_value=False +): """ Update specified recipient. Returns updated recipient element. - Raises AlertNotFound if alert with specified id doesn't exist. - Raises RecipientNotFound if recipient doesn't exist. + Raises LibraryError if recipient doesn't exist. + reporter -- report processor tree -- cib etree node - alert_id -- id of alert, parent element of recipient - recipient_value -- recipient value + recipient_id -- id of recipient to be updated + recipient_value -- recipient value, stay unchanged if None description -- description, if empty it will be removed, stay unchanged if None + allow_same_value -- if True unique recipient value is not required """ - recipient = get_recipient( - get_alert_by_id(tree, alert_id), recipient_value - ) + recipient = get_recipient_by_id(tree, recipient_id) + if recipient_value is not None: + ensure_recipient_value_is_unique( + reporter, + recipient.getparent(), + recipient_value, + recipient_id=recipient_id, + allow_duplicity=allow_same_value + ) + recipient.set("value", recipient_value) _update_optional_attribute(recipient, "description", description) return recipient -def remove_recipient(tree, alert_id, recipient_value): +def remove_recipient(tree, recipient_id): """ Remove specified recipient. - Raises AlertNotFound if alert with specified id doesn't exist. - Raises RecipientNotFound if recipient doesn't exist. + Raises LibraryError if recipient doesn't exist. tree -- cib etree node - alert_id -- id of alert, parent element of recipient - recipient_value -- recipient value + recipient_id -- id of recipient to be removed """ - recipient = get_recipient( - get_alert_by_id(tree, alert_id), recipient_value - ) + recipient = get_recipient_by_id(tree, recipient_id) recipient.getparent().remove(recipient) diff --git a/pcs/lib/cib/test/test_alert.py b/pcs/lib/cib/test/test_alert.py index c387aaf..50eaef6 100644 --- a/pcs/lib/cib/test/test_alert.py +++ b/pcs/lib/cib/test/test_alert.py @@ -15,8 +15,10 @@ from pcs.lib.errors import ReportItemSeverity as severities from pcs.test.tools.assertions import( assert_raise_library_error, assert_xml_equal, + assert_report_item_list_equal, ) from pcs.test.tools.pcs_mock import mock +from pcs.test.tools.custom_mock import MockLibraryReportProcessor @mock.patch("pcs.lib.cib.alert.update_nvset") @@ -129,54 +131,146 @@ class GetAlertByIdTest(TestCase): ) -class GetRecipientTest(TestCase): +class GetRecipientByIdTest(TestCase): def setUp(self): self.xml = etree.XML( """ - - - - - - - - + + + + + + + + + + + + + + + """ ) def test_exist(self): assert_xml_equal( - '', - etree.tostring(alert.get_recipient(self.xml, "value2")).decode() + '', + etree.tostring( + alert.get_recipient_by_id(self.xml, "rec-1") + ).decode() ) def test_different_place(self): assert_raise_library_error( - lambda: alert.get_recipient(self.xml, "value4"), + lambda: alert.get_recipient_by_id(self.xml, "rec-4"), ( severities.ERROR, - report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND, + report_codes.ID_NOT_FOUND, { - "alert": "alert-1", - "recipient": "value4" + "id": "rec-4", + "id_description": "Recipient" + } + ) + ) + + def test_not_in_alert(self): + assert_raise_library_error( + lambda: alert.get_recipient_by_id(self.xml, "rec-2"), + ( + severities.ERROR, + report_codes.ID_NOT_FOUND, + { + "id": "rec-2", + "id_description": "Recipient" } ) ) def test_not_recipient(self): assert_raise_library_error( - lambda: alert.get_recipient(self.xml, "value3"), + lambda: alert.get_recipient_by_id(self.xml, "rec-3"), ( severities.ERROR, - report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND, + report_codes.ID_NOT_FOUND, { - "alert": "alert-1", - "recipient": "value3" + "id": "rec-3", + "id_description": "Recipient" } ) ) +class EnsureRecipientValueIsUniqueTest(TestCase): + def setUp(self): + self.mock_reporter = MockLibraryReportProcessor() + self.alert = etree.Element("alert", id="alert-1") + self.recipient = etree.SubElement( + self.alert, "recipient", id="rec-1", value="value1" + ) + + def test_is_unique_no_duplicity_allowed(self): + alert.ensure_recipient_value_is_unique( + self.mock_reporter, self.alert, "value2" + ) + self.assertEqual(0, len(self.mock_reporter.report_item_list)) + + def test_same_recipient_no_duplicity_allowed(self): + alert.ensure_recipient_value_is_unique( + self.mock_reporter, self.alert, "value1", recipient_id="rec-1" + ) + self.assertEqual(0, len(self.mock_reporter.report_item_list)) + + def test_same_recipient_duplicity_allowed(self): + alert.ensure_recipient_value_is_unique( + self.mock_reporter, self.alert, "value1", recipient_id="rec-1", + allow_duplicity=True + ) + self.assertEqual(0, len(self.mock_reporter.report_item_list)) + + def test_not_unique_no_duplicity_allowed(self): + report_item = ( + severities.ERROR, + report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, + { + "alert": "alert-1", + "recipient": "value1" + }, + report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE + ) + assert_raise_library_error( + lambda: alert.ensure_recipient_value_is_unique( + self.mock_reporter, self.alert, "value1" + ), + report_item + ) + assert_report_item_list_equal( + self.mock_reporter.report_item_list, [report_item] + ) + + def test_is_unique_duplicity_allowed(self): + alert.ensure_recipient_value_is_unique( + self.mock_reporter, self.alert, "value2", allow_duplicity=True + ) + self.assertEqual(0, len(self.mock_reporter.report_item_list)) + + def test_not_unique_duplicity_allowed(self): + alert.ensure_recipient_value_is_unique( + self.mock_reporter, self.alert, "value1", allow_duplicity=True + ) + assert_report_item_list_equal( + self.mock_reporter.report_item_list, + [( + severities.WARNING, + report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, + { + "alert": "alert-1", + "recipient": "value1" + } + )] + ) + + class CreateAlertTest(TestCase): def setUp(self): self.tree = etree.XML( @@ -462,6 +556,7 @@ class RemoveAlertTest(TestCase): class AddRecipientTest(TestCase): def setUp(self): + self.mock_reporter = MockLibraryReportProcessor() self.tree = etree.XML( """ @@ -476,11 +571,40 @@ class AddRecipientTest(TestCase): """ ) - def test_success(self): + def test_with_id(self): + assert_xml_equal( + '', + etree.tostring( + alert.add_recipient( + self.mock_reporter, self.tree, "alert", "value1", + "my-recipient" + ) + ).decode() + ) + assert_xml_equal( + """ + + + + + + + + + + + """, + etree.tostring(self.tree).decode() + ) + self.assertEqual([], self.mock_reporter.report_item_list) + + def test_without_id(self): assert_xml_equal( '', etree.tostring( - alert.add_recipient(self.tree, "alert", "value1") + alert.add_recipient( + self.mock_reporter, self.tree, "alert", "value1" + ) ).decode() ) assert_xml_equal( @@ -498,23 +622,85 @@ class AddRecipientTest(TestCase): """, etree.tostring(self.tree).decode() ) + self.assertEqual([], self.mock_reporter.report_item_list) - def test_recipient_exist(self): + def test_id_exists(self): assert_raise_library_error( - lambda: alert.add_recipient(self.tree, "alert", "test_val"), + lambda: alert.add_recipient( + self.mock_reporter, self.tree, "alert", "value1", + "alert-recipient" + ), ( severities.ERROR, + report_codes.ID_ALREADY_EXISTS, + {"id": "alert-recipient"} + ) + ) + self.assertEqual([], self.mock_reporter.report_item_list) + + def test_duplicity_of_value_not_allowed(self): + report_item = ( + severities.ERROR, + report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, + { + "alert": "alert", + "recipient": "test_val" + }, + report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE + ) + assert_raise_library_error( + lambda: alert.add_recipient( + self.mock_reporter, self.tree, "alert", "test_val" + ), + report_item + ) + assert_report_item_list_equal( + self.mock_reporter.report_item_list, + [report_item] + ) + + def test_duplicity_of_value_allowed(self): + assert_xml_equal( + '', + etree.tostring( + alert.add_recipient( + self.mock_reporter, self.tree, "alert", "test_val", + allow_same_value=True + ) + ).decode() + ) + assert_xml_equal( + """ + + + + + + + + + + + """, + etree.tostring(self.tree).decode() + ) + assert_report_item_list_equal( + self.mock_reporter.report_item_list, + [( + severities.WARNING, report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, { - "recipient": "test_val", - "alert": "alert" + "alert": "alert", + "recipient": "test_val" } - ) + )] ) def test_alert_not_exist(self): assert_raise_library_error( - lambda: alert.add_recipient(self.tree, "alert1", "test_val"), + lambda: alert.add_recipient( + self.mock_reporter, self.tree, "alert1", "test_val" + ), ( severities.ERROR, report_codes.CIB_ALERT_NOT_FOUND, @@ -532,7 +718,8 @@ class AddRecipientTest(TestCase): /> """, etree.tostring(alert.add_recipient( - self.tree, "alert", "value1", "desc" + self.mock_reporter, self.tree, "alert", "value1", + description="desc" )).decode() ) assert_xml_equal( @@ -554,10 +741,12 @@ class AddRecipientTest(TestCase): """, etree.tostring(self.tree).decode() ) + self.assertEqual([], self.mock_reporter.report_item_list) class UpdateRecipientTest(TestCase): def setUp(self): + self.mock_reporter = MockLibraryReportProcessor() self.tree = etree.XML( """ @@ -577,6 +766,157 @@ class UpdateRecipientTest(TestCase): """ ) + def test_update_value(self): + assert_xml_equal( + """ + + """, + etree.tostring(alert.update_recipient( + self.mock_reporter, self.tree, "alert-recipient", + recipient_value="new_val" + )).decode() + ) + assert_xml_equal( + """ + + + + + + + + + + + """, + etree.tostring(self.tree).decode() + ) + self.assertEqual([], self.mock_reporter.report_item_list) + + def test_update_same_value_no_duplicity_allowed(self): + assert_xml_equal( + '', + etree.tostring(alert.update_recipient( + self.mock_reporter, self.tree, "alert-recipient", + recipient_value="test_val" + )).decode() + ) + assert_xml_equal( + """ + + + + + + + + + + + """, + etree.tostring(self.tree).decode() + ) + self.assertEqual([], self.mock_reporter.report_item_list) + + def test_update_same_value_duplicity_allowed(self): + assert_xml_equal( + '', + etree.tostring(alert.update_recipient( + self.mock_reporter, self.tree, "alert-recipient", + recipient_value="test_val", allow_same_value=True + )).decode() + ) + assert_xml_equal( + """ + + + + + + + + + + + """, + etree.tostring(self.tree).decode() + ) + self.assertEqual([], self.mock_reporter.report_item_list) + + def test_duplicity_of_value_not_allowed(self): + report_item = ( + severities.ERROR, + report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, + { + "alert": "alert", + "recipient": "value1" + }, + report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE + ) + assert_raise_library_error( + lambda: alert.update_recipient( + self.mock_reporter, self.tree, "alert-recipient", "value1" + ), + report_item + ) + assert_report_item_list_equal( + self.mock_reporter.report_item_list, + [report_item] + ) + + def test_duplicity_of_value_allowed(self): + assert_xml_equal( + """ + + """, + etree.tostring(alert.update_recipient( + self.mock_reporter, self.tree, "alert-recipient", + recipient_value="value1", allow_same_value=True + )).decode() + ) + assert_xml_equal( + """ + + + + + + + + + + + """, + etree.tostring(self.tree).decode() + ) + assert_report_item_list_equal( + self.mock_reporter.report_item_list, + [( + severities.WARNING, + report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, + { + "alert": "alert", + "recipient": "value1" + } + )] + ) + def test_add_description(self): assert_xml_equal( """ @@ -585,7 +925,8 @@ class UpdateRecipientTest(TestCase): /> """, etree.tostring(alert.update_recipient( - self.tree, "alert", "test_val", "description" + self.mock_reporter, self.tree, "alert-recipient", + description="description" )).decode() ) assert_xml_equal( @@ -611,6 +952,7 @@ class UpdateRecipientTest(TestCase): """, etree.tostring(self.tree).decode() ) + self.assertEqual([], self.mock_reporter.report_item_list) def test_update_description(self): assert_xml_equal( @@ -620,7 +962,8 @@ class UpdateRecipientTest(TestCase): /> """, etree.tostring(alert.update_recipient( - self.tree, "alert", "value1", "description" + self.mock_reporter, self.tree, "alert-recipient-1", + description="description" )).decode() ) assert_xml_equal( @@ -642,6 +985,7 @@ class UpdateRecipientTest(TestCase): """, etree.tostring(self.tree).decode() ) + self.assertEqual([], self.mock_reporter.report_item_list) def test_remove_description(self): assert_xml_equal( @@ -649,7 +993,10 @@ class UpdateRecipientTest(TestCase): """, etree.tostring( - alert.update_recipient(self.tree, "alert", "value1", "") + alert.update_recipient( + self.mock_reporter, self.tree, "alert-recipient-1", + description="" + ) ).decode() ) assert_xml_equal( @@ -667,26 +1014,18 @@ class UpdateRecipientTest(TestCase): """, etree.tostring(self.tree).decode() ) - - def test_alert_not_exists(self): - assert_raise_library_error( - lambda: alert.update_recipient(self.tree, "alert1", "test_val", ""), - ( - severities.ERROR, - report_codes.CIB_ALERT_NOT_FOUND, - {"alert": "alert1"} - ) - ) + self.assertEqual([], self.mock_reporter.report_item_list) def test_recipient_not_exists(self): assert_raise_library_error( - lambda: alert.update_recipient(self.tree, "alert", "unknown", ""), + lambda: alert.update_recipient( + self.mock_reporter, self.tree, "recipient"), ( severities.ERROR, - report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND, + report_codes.ID_NOT_FOUND, { - "alert": "alert", - "recipient": "unknown" + "id": "recipient", + "id_description": "Recipient" } ) ) @@ -710,7 +1049,7 @@ class RemoveRecipientTest(TestCase): ) def test_success(self): - alert.remove_recipient(self.tree, "alert", "val") + alert.remove_recipient(self.tree, "alert-recipient-2") assert_xml_equal( """ @@ -726,25 +1065,15 @@ class RemoveRecipientTest(TestCase): etree.tostring(self.tree).decode() ) - def test_alert_not_exists(self): - assert_raise_library_error( - lambda: alert.remove_recipient(self.tree, "alert1", "test_val"), - ( - severities.ERROR, - report_codes.CIB_ALERT_NOT_FOUND, - {"alert": "alert1"} - ) - ) - def test_recipient_not_exists(self): assert_raise_library_error( - lambda: alert.remove_recipient(self.tree, "alert", "unknown"), + lambda: alert.remove_recipient(self.tree, "recipient"), ( severities.ERROR, - report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND, + report_codes.ID_NOT_FOUND, { - "alert": "alert", - "recipient": "unknown" + "id": "recipient", + "id_description": "Recipient" } ) ) diff --git a/pcs/lib/commands/alert.py b/pcs/lib/commands/alert.py index 7371fbc..432d9d5 100644 --- a/pcs/lib/commands/alert.py +++ b/pcs/lib/commands/alert.py @@ -90,7 +90,9 @@ def add_recipient( recipient_value, instance_attribute_dict, meta_attribute_dict, - description=None + recipient_id=None, + description=None, + allow_same_value=False ): """ Add new recipient to alert witch id alert_id. @@ -100,7 +102,9 @@ def add_recipient( recipient_value -- value of new recipient instance_attribute_dict -- dictionary of instance attributes to update meta_attribute_dict -- dictionary of meta attributes to update + recipient_id -- id of new recipient, if None it will be generated description -- recipient description + allow_same_value -- if True unique recipient value is not required """ if not recipient_value: raise LibraryError( @@ -109,7 +113,13 @@ def add_recipient( cib = lib_env.get_cib(REQUIRED_CIB_VERSION) recipient = alert.add_recipient( - cib, alert_id, recipient_value, description + lib_env.report_processor, + cib, + alert_id, + recipient_value, + recipient_id=recipient_id, + description=description, + allow_same_value=allow_same_value ) alert.update_instance_attributes(cib, recipient, instance_attribute_dict) alert.update_meta_attributes(cib, recipient, meta_attribute_dict) @@ -119,26 +129,38 @@ def add_recipient( def update_recipient( lib_env, - alert_id, - recipient_value, + recipient_id, instance_attribute_dict, meta_attribute_dict, - description=None + recipient_value=None, + description=None, + allow_same_value=False ): """ Update existing recipient. lib_env -- LibraryEnvironment - alert_id -- id of alert to which recipient belong - recipient_value -- recipient to be updated + recipient_id -- id of recipient to be updated instance_attribute_dict -- dictionary of instance attributes to update meta_attribute_dict -- dictionary of meta attributes to update + recipient_value -- new recipient value, if None old value will stay + unchanged description -- new description, if empty string, old description will be deleted, if None old value will stay unchanged + allow_same_value -- if True unique recipient value is not required """ + if not recipient_value and recipient_value is not None: + raise LibraryError( + reports.cib_alert_recipient_invalid_value(recipient_value) + ) cib = lib_env.get_cib(REQUIRED_CIB_VERSION) recipient = alert.update_recipient( - cib, alert_id, recipient_value, description + lib_env.report_processor, + cib, + recipient_id, + recipient_value=recipient_value, + description=description, + allow_same_value=allow_same_value ) alert.update_instance_attributes(cib, recipient, instance_attribute_dict) alert.update_meta_attributes(cib, recipient, meta_attribute_dict) @@ -146,16 +168,15 @@ def update_recipient( lib_env.push_cib(cib) -def remove_recipient(lib_env, alert_id, recipient_value): +def remove_recipient(lib_env, recipient_id): """ Remove existing recipient. lib_env -- LibraryEnvironment - alert_id -- id of alert to which recipient belong - recipient_value -- recipient to be removed + recipient_id -- if of recipient to be removed """ cib = lib_env.get_cib(REQUIRED_CIB_VERSION) - alert.remove_recipient(cib, alert_id, recipient_value) + alert.remove_recipient(cib, recipient_id) lib_env.push_cib(cib) diff --git a/pcs/lib/commands/test/test_alert.py b/pcs/lib/commands/test/test_alert.py index 34813df..bced45e 100644 --- a/pcs/lib/commands/test/test_alert.py +++ b/pcs/lib/commands/test/test_alert.py @@ -361,19 +361,17 @@ class AddRecipientTest(TestCase): def test_recipient_already_exists(self): assert_raise_library_error( lambda: cmd_alert.add_recipient( - self.mock_env, "alert", "value1", {}, {} + self.mock_env, "alert", "value1", {}, {}, + recipient_id="alert-recipient" ), ( Severities.ERROR, - report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, - { - "recipient": "value1", - "alert": "alert" - } + report_codes.ID_ALREADY_EXISTS, + {"id": "alert-recipient"} ) ) - def test_success(self): + def test_without_id(self): cmd_alert.add_recipient( self.mock_env, "alert", @@ -424,6 +422,58 @@ class AddRecipientTest(TestCase): self.mock_env._get_cib_xml() ) + def test_with_id(self): + cmd_alert.add_recipient( + self.mock_env, + "alert", + "value", + {"attr1": "val1"}, + { + "attr2": "val2", + "attr1": "val1" + }, + recipient_id="my-recipient" + ) + assert_xml_equal( + """ + + + + + + + + + + + + + + + + + + + """, + self.mock_env._get_cib_xml() + ) + class UpdateRecipientTest(TestCase): def setUp(self): @@ -470,29 +520,29 @@ class UpdateRecipientTest(TestCase): self.mock_log, self.mock_rep, cib_data=cib ) - def test_alert_not_found(self): + def test_empty_value(self): assert_raise_library_error( lambda: cmd_alert.update_recipient( - self.mock_env, "unknown", "recipient", {}, {} + self.mock_env, "alert-recipient-1", {}, {}, recipient_value="" ), ( Severities.ERROR, - report_codes.CIB_ALERT_NOT_FOUND, - {"alert": "unknown"} + report_codes.CIB_ALERT_RECIPIENT_VALUE_INVALID, + {"recipient": ""} ) ) def test_recipient_not_found(self): assert_raise_library_error( lambda: cmd_alert.update_recipient( - self.mock_env, "alert", "recipient", {}, {} + self.mock_env, "recipient", {}, {} ), ( Severities.ERROR, - report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND, + report_codes.ID_NOT_FOUND, { - "recipient": "recipient", - "alert": "alert" + "id": "recipient", + "id_description": "Recipient" } ) ) @@ -500,14 +550,14 @@ class UpdateRecipientTest(TestCase): def test_update_all(self): cmd_alert.update_recipient( self.mock_env, - "alert", - "value", + "alert-recipient-1", {"attr1": "value"}, { "attr1": "", "attr3": "new_val" }, - "desc" + recipient_value="new_val", + description="desc" ) assert_xml_equal( """ @@ -518,7 +568,7 @@ class UpdateRecipientTest(TestCase): diff --git a/pcs/lib/reports.py b/pcs/lib/reports.py index 9ececf9..fc2670b 100644 --- a/pcs/lib/reports.py +++ b/pcs/lib/reports.py @@ -1654,40 +1654,39 @@ def cluster_restart_required_to_apply_changes(): ) -def cib_alert_recipient_already_exists(alert_id, recipient_value): +def cib_alert_recipient_already_exists( + alert_id, recipient_value, severity=ReportItemSeverity.ERROR, forceable=None +): """ - Error that recipient already exists. + Recipient with specified value already exists in alert with id 'alert_id' alert_id -- id of alert to which recipient belongs recipient_value -- value of recipient """ - return ReportItem.error( + return ReportItem( report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS, - "Recipient '{recipient}' in alert '{alert}' already exists.", + severity, + "Recipient '{recipient}' in alert '{alert}' already exists", info={ "recipient": recipient_value, "alert": alert_id - } + }, + forceable=forceable ) -def cib_alert_recipient_not_found(alert_id, recipient_value): +def cib_alert_recipient_invalid_value(recipient_value): """ - Specified recipient not found. + Invalid recipient value. - alert_id -- id of alert to which recipient should belong recipient_value -- recipient value """ return ReportItem.error( - report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND, - "Recipient '{recipient}' not found in alert '{alert}'.", - info={ - "recipient": recipient_value, - "alert": alert_id - } + report_codes.CIB_ALERT_RECIPIENT_VALUE_INVALID, + "Recipient value '{recipient}' is not valid.", + info={"recipient": recipient_value} ) - def cib_alert_not_found(alert_id): """ Alert with specified id doesn't exist. -- 1.8.3.1 From b8155a2bfa79bb71429953eb756e393c18926e4c Mon Sep 17 00:00:00 2001 From: Ondrej Mular Date: Sat, 9 Jul 2016 13:54:00 +0200 Subject: [PATCH 2/2] cli: use recipient id as identifier instead of its value --- pcs/alert.py | 32 +++++----- pcs/pcs.8 | 8 +-- pcs/test/test_alert.py | 165 ++++++++++++++++++++++++++++++++++++++++--------- pcs/usage.py | 14 +++-- 4 files changed, 164 insertions(+), 55 deletions(-) diff --git a/pcs/alert.py b/pcs/alert.py index d3a6e28..4786f57 100644 --- a/pcs/alert.py +++ b/pcs/alert.py @@ -139,42 +139,44 @@ def recipient_add(lib, argv, modifiers): sections = parse_cmd_sections(argv[2:], ["options", "meta"]) main_args = prepare_options(sections["main"]) - ensure_only_allowed_options(main_args, ["description"]) + ensure_only_allowed_options(main_args, ["description", "id"]) lib.alert.add_recipient( alert_id, recipient_value, prepare_options(sections["options"]), prepare_options(sections["meta"]), - main_args.get("description", None) + recipient_id=main_args.get("id", None), + description=main_args.get("description", None), + allow_same_value=modifiers["force"] ) def recipient_update(lib, argv, modifiers): - if len(argv) < 2: + if len(argv) < 1: raise CmdLineInputError() - alert_id = argv[0] - recipient_value = argv[1] + recipient_id = argv[0] - sections = parse_cmd_sections(argv[2:], ["options", "meta"]) + sections = parse_cmd_sections(argv[1:], ["options", "meta"]) main_args = prepare_options(sections["main"]) - ensure_only_allowed_options(main_args, ["description"]) + ensure_only_allowed_options(main_args, ["description", "value"]) lib.alert.update_recipient( - alert_id, - recipient_value, + recipient_id, prepare_options(sections["options"]), prepare_options(sections["meta"]), - main_args.get("description", None) + recipient_value=main_args.get("value", None), + description=main_args.get("description", None), + allow_same_value=modifiers["force"] ) def recipient_remove(lib, argv, modifiers): - if len(argv) != 2: + if len(argv) != 1: raise CmdLineInputError() - lib.alert.remove_recipient(argv[0], argv[1]) + lib.alert.remove_recipient(argv[0]) def _nvset_to_str(nvset_obj): @@ -219,9 +221,9 @@ def _alert_to_str(alert): def _recipient_to_str(recipient): - return ["Recipient: {value}".format(value=recipient["value"])] + indent( - __description_attributes_to_str(recipient), 1 - ) + return ["Recipient: {id} (value={value})".format( + value=recipient["value"], id=recipient["id"] + )] + indent(__description_attributes_to_str(recipient), 1) def print_alert_config(lib, argv, modifiers): diff --git a/pcs/pcs.8 b/pcs/pcs.8 index 4426444..223ef1b 100644 --- a/pcs/pcs.8 +++ b/pcs/pcs.8 @@ -666,13 +666,13 @@ Update existing alert with specified id. remove Remove alert with specified id. .TP -recipient add [description=] [options [