From f5a6aa8497a8126717e305b4e46131925bf97c8e Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Fri, 10 Dec 2021 15:54:45 +0000 Subject: [PATCH 01/18] Initial changes to support pack override of enable parameter Supports pack.yaml files in /opt/stackstorm/configs/overrides of following format: --- actions: defaults: enabled: false exceptions: delete_row: enabled: true --- .../st2common/bootstrap/actionsregistrar.py | 3 + .../st2common/bootstrap/aliasesregistrar.py | 3 + st2common/st2common/bootstrap/base.py | 2 + .../st2common/bootstrap/rulesregistrar.py | 3 + .../st2common/bootstrap/sensorsregistrar.py | 3 + st2common/st2common/content/loader.py | 63 ++++++++++++++++++- 6 files changed, 76 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/bootstrap/actionsregistrar.py b/st2common/st2common/bootstrap/actionsregistrar.py index 4349812f05..074a5cec24 100644 --- a/st2common/st2common/bootstrap/actionsregistrar.py +++ b/st2common/st2common/bootstrap/actionsregistrar.py @@ -142,6 +142,9 @@ def _register_action(self, pack, action): ) content["metadata_file"] = metadata_file + # Pass override information + self._override_loader.override(pack, "actions", content) + action_api = ActionAPI(**content) try: diff --git a/st2common/st2common/bootstrap/aliasesregistrar.py b/st2common/st2common/bootstrap/aliasesregistrar.py index d961b32e0c..849dcd6ad8 100644 --- a/st2common/st2common/bootstrap/aliasesregistrar.py +++ b/st2common/st2common/bootstrap/aliasesregistrar.py @@ -144,6 +144,9 @@ def _get_action_alias_db( else: content["metadata_file"] = metadata_file + # Pass override information + self._override_loader.override(pack, "aliases", content) + action_alias_api = ActionAliasAPI(**content) action_alias_api.validate() action_alias_db = ActionAliasAPI.to_model(action_alias_api) diff --git a/st2common/st2common/bootstrap/base.py b/st2common/st2common/bootstrap/base.py index 1070a3af38..275b93f1dc 100644 --- a/st2common/st2common/bootstrap/base.py +++ b/st2common/st2common/bootstrap/base.py @@ -22,6 +22,7 @@ from st2common import log as logging from st2common.constants.pack import CONFIG_SCHEMA_FILE_NAME from st2common.content.loader import MetaLoader +from st2common.content.loader import OverrideLoader from st2common.content.loader import ContentPackLoader from st2common.models.api.pack import PackAPI from st2common.models.api.pack import ConfigSchemaAPI @@ -68,6 +69,7 @@ def __init__( self._fail_on_failure = fail_on_failure self._meta_loader = MetaLoader() + self._override_loader = OverrideLoader() self._pack_loader = ContentPackLoader() # Maps runner name -> RunnerTypeDB diff --git a/st2common/st2common/bootstrap/rulesregistrar.py b/st2common/st2common/bootstrap/rulesregistrar.py index a58c34e162..b1b739e002 100644 --- a/st2common/st2common/bootstrap/rulesregistrar.py +++ b/st2common/st2common/bootstrap/rulesregistrar.py @@ -128,6 +128,9 @@ def _register_rules_from_pack(self, pack, rules): ) content["metadata_file"] = metadata_file + # Pass override information + self._override_loader.override(pack, "rules", content) + rule_api = RuleAPI(**content) rule_api.validate() rule_db = RuleAPI.to_model(rule_api) diff --git a/st2common/st2common/bootstrap/sensorsregistrar.py b/st2common/st2common/bootstrap/sensorsregistrar.py index 7476041da1..e22bef4ecd 100644 --- a/st2common/st2common/bootstrap/sensorsregistrar.py +++ b/st2common/st2common/bootstrap/sensorsregistrar.py @@ -167,6 +167,9 @@ def _register_sensor_from_pack(self, pack, sensor): ) content["metadata_file"] = metadata_file + # Pass override information + self._override_loader.override(pack, "sensors", content) + sensors_dir = os.path.dirname(sensor_metadata_file_path) sensor_file_path = os.path.join(sensors_dir, entry_point) artifact_uri = "file://%s" % (sensor_file_path) diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 3eed301bda..ec70ea37db 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -20,6 +20,7 @@ from yaml.parser import ParserError import six +from oslo_config import cfg from st2common import log as logging from st2common.constants.meta import ALLOWED_EXTS from st2common.constants.meta import PARSER_FUNCS @@ -28,7 +29,7 @@ if six.PY2: from io import open -__all__ = ["ContentPackLoader", "MetaLoader"] +__all__ = ["ContentPackLoader", "MetaLoader", "OverrideLoader"] LOG = logging.getLogger(__name__) @@ -268,3 +269,63 @@ def _load(self, parser_func, file_path): except ParserError: LOG.exception("Failed loading content from %s.", file_path) raise + +class OverrideLoader(object): + """ + Class for loading pack override data + """ + ALLOWED_OVERRIDE_TYPES = [ + "sensors", + "actions", + "rules", + "aliases", + ] + + def override(self, pack_name, type, content): + + """ + Loads override content for pack, and updates content + + :param pack: Name of pack + :type pack: ``str`` + + """ + + if type not in self.ALLOWED_OVERRIDE_TYPES: + LOG.warning(f"Invalid override type of {type} will be ignored for pack {pack_name}") + override_dir = os.path.join(cfg.CONF.system.base_path, "configs/overrides") + override_file = os.path.join(override_dir, f"{pack_name}.yaml") + if not os.path.exists(override_file): + # No override file for pack + return content + + # Read override file + file_name, file_ext = os.path.splitext(override_file) + overrides = self._load(PARSER_FUNCS[file_ext], override_file) + + # Apply overrides + if type in overrides.keys(): + type_override = overrides[type] + name = content["name"] + if "defaults" in type_override.keys(): + if "enabled" in type_override["defaults"]: + content["enabled"] = type_override["defaults"]["enabled"] + LOG.info(f'Overridden {type} {pack_name}.{name} enabled to default value of {content["enabled"]}') + if "exceptions" in type_override.keys(): + if name in type_override["exceptions"]: + if "enabled" in type_override["exceptions"][name]: + content["enabled"] = type_override["exceptions"][name]["enabled"] + LOG.info(f'Overridden {type} {pack_name}.{name} enabled to exception value of {content["enabled"]}') + + return content + + def _load(self, parser_func, file_path): + with open(file_path, "r", encoding="utf-8") as fd: + try: + return parser_func(fd) + except ValueError: + LOG.exception("Failed loading content from %s.", file_path) + raise + except ParserError: + LOG.exception("Failed loading content from %s.", file_path) + raise From af455f6c4677d6dda51dc1471af682e1760f81a1 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Fri, 10 Dec 2021 17:43:59 +0000 Subject: [PATCH 02/18] Add UTs and make more generic --- st2common/st2common/content/loader.py | 33 +++-- .../configs/overrides/overpack1.yaml | 7 ++ .../configs/overrides/overpack2.yaml | 8 ++ .../configs/overrides/overpack3.yaml | 7 ++ .../configs/overrides/overpack4.yaml | 4 + st2common/tests/unit/test_content_loader.py | 115 ++++++++++++++++++ 6 files changed, 164 insertions(+), 10 deletions(-) create mode 100644 st2common/tests/resources/configs/overrides/overpack1.yaml create mode 100644 st2common/tests/resources/configs/overrides/overpack2.yaml create mode 100644 st2common/tests/resources/configs/overrides/overpack3.yaml create mode 100644 st2common/tests/resources/configs/overrides/overpack4.yaml diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index ec70ea37db..657e62c524 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -281,18 +281,26 @@ class OverrideLoader(object): "aliases", ] + ALLOWED_OVERRIDE_NAMES = [ + "enabled", + ] + def override(self, pack_name, type, content): """ Loads override content for pack, and updates content - :param pack: Name of pack - :type pack: ``str`` + :param pack_name: Name of pack + :type pack_name: ``str`` + :param type: Type of resource loading + :type type: ``str`` + :param content: Content as loaded from meta information + :type content: ``object`` """ if type not in self.ALLOWED_OVERRIDE_TYPES: - LOG.warning(f"Invalid override type of {type} will be ignored for pack {pack_name}") + raise ValueError(f"Invalid override type of {type} attempted for pack {pack_name}") override_dir = os.path.join(cfg.CONF.system.base_path, "configs/overrides") override_file = os.path.join(override_dir, f"{pack_name}.yaml") if not os.path.exists(override_file): @@ -302,20 +310,25 @@ def override(self, pack_name, type, content): # Read override file file_name, file_ext = os.path.splitext(override_file) overrides = self._load(PARSER_FUNCS[file_ext], override_file) - # Apply overrides if type in overrides.keys(): type_override = overrides[type] name = content["name"] if "defaults" in type_override.keys(): - if "enabled" in type_override["defaults"]: - content["enabled"] = type_override["defaults"]["enabled"] - LOG.info(f'Overridden {type} {pack_name}.{name} enabled to default value of {content["enabled"]}') + for key in type_override["defaults"].keys(): + if key in self.ALLOWED_OVERRIDE_NAMES: + content[key] = type_override["defaults"][key] + LOG.info(f'Overridden {type} {pack_name}.{name} {key} to default value of {content[key]}') + else: + raise ValueError(f'Override attempted with invalid default key {key} in pack {pack_name}' ) if "exceptions" in type_override.keys(): if name in type_override["exceptions"]: - if "enabled" in type_override["exceptions"][name]: - content["enabled"] = type_override["exceptions"][name]["enabled"] - LOG.info(f'Overridden {type} {pack_name}.{name} enabled to exception value of {content["enabled"]}') + for key in type_override["exceptions"][name].keys(): + if key in self.ALLOWED_OVERRIDE_NAMES: + content[key] = type_override["exceptions"][name][key] + LOG.info(f'Overridden {type} {pack_name}.{name} {key} to exception value of {content[key]}') + else: + raise ValueError(f'Override attempted with invalid exceptions key {key} in pack {pack_name}' ) return content diff --git a/st2common/tests/resources/configs/overrides/overpack1.yaml b/st2common/tests/resources/configs/overrides/overpack1.yaml new file mode 100644 index 0000000000..236107cf0a --- /dev/null +++ b/st2common/tests/resources/configs/overrides/overpack1.yaml @@ -0,0 +1,7 @@ +--- +actions: + defaults: + enabled: False + exceptions: + action2: + enabled: True diff --git a/st2common/tests/resources/configs/overrides/overpack2.yaml b/st2common/tests/resources/configs/overrides/overpack2.yaml new file mode 100644 index 0000000000..6650c314cf --- /dev/null +++ b/st2common/tests/resources/configs/overrides/overpack2.yaml @@ -0,0 +1,8 @@ +--- +actions: + defaults: + enabled: False + rubbish: False + exceptions: + action2: + enabled: True diff --git a/st2common/tests/resources/configs/overrides/overpack3.yaml b/st2common/tests/resources/configs/overrides/overpack3.yaml new file mode 100644 index 0000000000..9cd543814f --- /dev/null +++ b/st2common/tests/resources/configs/overrides/overpack3.yaml @@ -0,0 +1,7 @@ +--- +actions: + defaults: + enabled: False + exceptions: + action2: + rubbish: True diff --git a/st2common/tests/resources/configs/overrides/overpack4.yaml b/st2common/tests/resources/configs/overrides/overpack4.yaml new file mode 100644 index 0000000000..f4f132aa20 --- /dev/null +++ b/st2common/tests/resources/configs/overrides/overpack4.yaml @@ -0,0 +1,4 @@ +--- +actions: + defaults: + enabled: False diff --git a/st2common/tests/unit/test_content_loader.py b/st2common/tests/unit/test_content_loader.py index 279d04dc4a..1803bdd401 100644 --- a/st2common/tests/unit/test_content_loader.py +++ b/st2common/tests/unit/test_content_loader.py @@ -15,6 +15,7 @@ from __future__ import absolute_import +from oslo_config import cfg import os import unittest2 @@ -31,8 +32,10 @@ from mock import Mock from st2common.content.loader import ContentPackLoader +from st2common.content.loader import OverrideLoader from st2common.content.loader import LOG from st2common.constants.meta import yaml_safe_load +from st2tests import config CURRENT_DIR = os.path.dirname(os.path.realpath(__file__)) RESOURCES_DIR = os.path.abspath(os.path.join(CURRENT_DIR, "../resources")) @@ -114,6 +117,118 @@ def test_get_content_from_pack_no_sensors(self): ) self.assertEqual(result, None) + def test_get_override_action_from_default(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"name":"action1","enabled": True} + loader.override("overpack1", "actions", content) + self.assertFalse(content["enabled"]) + content = {"name":"action1","enabled": False} + loader.override("overpack1", "actions", content) + self.assertFalse(content["enabled"]) + + def test_get_override_action_from_exception(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"name":"action2","enabled": True} + loader.override("overpack1", "actions", content) + self.assertTrue(content["enabled"]) + content = {"name":"action2","enabled": False} + loader.override("overpack1", "actions", content) + self.assertTrue(content["enabled"]) + + def test_get_override_action_from_default_no_exceptions(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"name":"action1","enabled": True} + loader.override("overpack4", "actions", content) + self.assertFalse(content["enabled"]) + content = {"name":"action2","enabled": True} + loader.override("overpack4", "actions", content) + self.assertFalse(content["enabled"]) + + def test_get_override_invalid_type(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"name":"action2","enabled": True} + self.assertRaises( + ValueError, + loader.override, + pack_name="overpack1", + type="wrongtype", + content=content + ) + + def test_get_override_invalid_default_key(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"name":"action1","enabled": True} + self.assertRaises( + ValueError, + loader.override, + pack_name="overpack2", + type="actions", + content=content + ) + + def test_get_override_invalid_exceptions_key(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"name":"action1","enabled": True} + loader.override("overpack1", "actions", content) + content = {"name":"action2","enabled": True} + self.assertRaises( + ValueError, + loader.override, + pack_name="overpack3", + type="actions", + content=content + ) + +class YamlLoaderTestCase(unittest2.TestCase): + def test_yaml_safe_load(self): + # Verify C version of yaml loader indeed doesn't load non-safe data + dumped = yaml.dump(Foo) + self.assertTrue("!!python" in dumped) + + # Regular full load should work, but safe wrapper should fail + result = yaml.load(dumped, Loader=FullLoader) + self.assertTrue(result) + + self.assertRaisesRegexp( + yaml.constructor.ConstructorError, + "could not determine a constructor", + yaml_safe_load, + dumped, + ) + + self.assertRaisesRegexp( + yaml.constructor.ConstructorError, + "could not determine a constructor", + yaml.load, + dumped, + Loader=SafeLoader, + ) + + if CSafeLoader: + self.assertRaisesRegexp( + yaml.constructor.ConstructorError, + "could not determine a constructor", + yaml.load, + dumped, + Loader=CSafeLoader, + ) + + +class Foo(object): + a = "1" + b = "c" class YamlLoaderTestCase(unittest2.TestCase): def test_yaml_safe_load(self): From 6592a7c8893a18055716cd1e7081b4a8042eab56 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Thu, 16 Dec 2021 14:24:42 +0000 Subject: [PATCH 03/18] Add global override --- st2common/st2common/content/loader.py | 55 +++++++++++++++---- .../resources/configs/overrides/global.yaml | 4 ++ .../configs/overrides/overpack2.yaml | 3 + .../configs/overrides/overpack3.yaml | 4 ++ st2common/tests/unit/test_content_loader.py | 24 ++++++++ 5 files changed, 80 insertions(+), 10 deletions(-) create mode 100644 st2common/tests/resources/configs/overrides/global.yaml diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 657e62c524..ad3c0c7014 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -274,12 +274,14 @@ class OverrideLoader(object): """ Class for loading pack override data """ - ALLOWED_OVERRIDE_TYPES = [ - "sensors", - "actions", - "rules", - "aliases", - ] + + # Mapping of permitted override types to resource name + ALLOWED_OVERRIDE_TYPES = { + "sensors": "class_name", + "actions": "name", + "rules": "name", + "aliases": "name", + } ALLOWED_OVERRIDE_NAMES = [ "enabled", @@ -299,12 +301,40 @@ def override(self, pack_name, type, content): """ - if type not in self.ALLOWED_OVERRIDE_TYPES: + if type not in self.ALLOWED_OVERRIDE_TYPES.keys(): raise ValueError(f"Invalid override type of {type} attempted for pack {pack_name}") + override_dir = os.path.join(cfg.CONF.system.base_path, "configs/overrides") + # Apply global overrides + global_file = os.path.join(override_dir, "global.yaml") + self._apply_override_file(global_file, pack_name, type, content, True) + + # Apply pack overrides override_file = os.path.join(override_dir, f"{pack_name}.yaml") + self._apply_override_file(override_file, pack_name, type, content, False) + + return content + + def _apply_override_file(self, override_file, pack_name, type, content, global_file): + + """ + Loads override content from override file + + :param override_file: Override filename + :type override_file: ``str`` + :param pack_name: Name of pack + :type pack_name: ``str`` + :param type: Type of resource loading + :type type: ``str`` + :param content: Content as loaded from meta information + :type content: ``object`` + :param global_file: Whether global file + :type global_file: ``bool`` + """ + if not os.path.exists(override_file): # No override file for pack + LOG.info(f"No override file {override_file} found") return content # Read override file @@ -313,20 +343,25 @@ def override(self, pack_name, type, content): # Apply overrides if type in overrides.keys(): type_override = overrides[type] - name = content["name"] + name = content[self.ALLOWED_OVERRIDE_TYPES[type]] if "defaults" in type_override.keys(): for key in type_override["defaults"].keys(): if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["defaults"][key] - LOG.info(f'Overridden {type} {pack_name}.{name} {key} to default value of {content[key]}') + LOG.info(f'Overridden {type} {pack_name}.{name} {key} to default value of {content[key]} from {override_file}') else: raise ValueError(f'Override attempted with invalid default key {key} in pack {pack_name}' ) + + if global_file: + # No exceptions required in global content file + return content + if "exceptions" in type_override.keys(): if name in type_override["exceptions"]: for key in type_override["exceptions"][name].keys(): if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["exceptions"][name][key] - LOG.info(f'Overridden {type} {pack_name}.{name} {key} to exception value of {content[key]}') + LOG.info(f'Overridden {type} {pack_name}.{name} {key} to exception value of {content[key]} from {override_file}') else: raise ValueError(f'Override attempted with invalid exceptions key {key} in pack {pack_name}' ) diff --git a/st2common/tests/resources/configs/overrides/global.yaml b/st2common/tests/resources/configs/overrides/global.yaml new file mode 100644 index 0000000000..d67d1f2ca0 --- /dev/null +++ b/st2common/tests/resources/configs/overrides/global.yaml @@ -0,0 +1,4 @@ +--- +sensors: + defaults: + enabled: false diff --git a/st2common/tests/resources/configs/overrides/overpack2.yaml b/st2common/tests/resources/configs/overrides/overpack2.yaml index 6650c314cf..620f843cbe 100644 --- a/st2common/tests/resources/configs/overrides/overpack2.yaml +++ b/st2common/tests/resources/configs/overrides/overpack2.yaml @@ -6,3 +6,6 @@ actions: exceptions: action2: enabled: True +sensors: + defaults: + enabled: True diff --git a/st2common/tests/resources/configs/overrides/overpack3.yaml b/st2common/tests/resources/configs/overrides/overpack3.yaml index 9cd543814f..6bf3532cc8 100644 --- a/st2common/tests/resources/configs/overrides/overpack3.yaml +++ b/st2common/tests/resources/configs/overrides/overpack3.yaml @@ -5,3 +5,7 @@ actions: exceptions: action2: rubbish: True +sensors: + exceptions: + sensor1: + enabled: True diff --git a/st2common/tests/unit/test_content_loader.py b/st2common/tests/unit/test_content_loader.py index 1803bdd401..5e8441f1de 100644 --- a/st2common/tests/unit/test_content_loader.py +++ b/st2common/tests/unit/test_content_loader.py @@ -150,6 +150,30 @@ def test_get_override_action_from_default_no_exceptions(self): loader.override("overpack4", "actions", content) self.assertFalse(content["enabled"]) + def test_get_override_action_from_global_default_no_exceptions(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"class_name":"sensor1","enabled": True} + loader.override("overpack1", "sensors", content) + self.assertFalse(content["enabled"]) + + def test_get_override_action_from_global_overridden_by_pack(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"class_name":"sensor1","enabled": True} + loader.override("overpack2", "sensors", content) + self.assertTrue(content["enabled"]) + + def test_get_override_action_from_global_overridden_by_pack_exception(self): + config.parse_args() + cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") + loader = OverrideLoader() + content = {"class_name":"sensor1","enabled": True} + loader.override("overpack3", "sensors", content) + self.assertTrue(content["enabled"]) + def test_get_override_invalid_type(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") From 8f4f4f88dc5880a6460600c45ac649db7ec66088 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Thu, 16 Dec 2021 14:35:34 +0000 Subject: [PATCH 04/18] Add changelog and black formatting --- CHANGELOG.rst | 4 +++ st2common/st2common/content/loader.py | 25 +++++++++++---- st2common/tests/unit/test_content_loader.py | 34 +++++++++++---------- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 74cc63ecdd..71c283ce74 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -11,6 +11,10 @@ Added Contributed by @khushboobhatia01 +* Added support to override enabled parameter of resources. #5506 + + Contributed by Amanda McGuinness (@amanda11 Intive) + Fixed ~~~~~ diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index ad3c0c7014..92ac9966c5 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -270,6 +270,7 @@ def _load(self, parser_func, file_path): LOG.exception("Failed loading content from %s.", file_path) raise + class OverrideLoader(object): """ Class for loading pack override data @@ -302,7 +303,9 @@ def override(self, pack_name, type, content): """ if type not in self.ALLOWED_OVERRIDE_TYPES.keys(): - raise ValueError(f"Invalid override type of {type} attempted for pack {pack_name}") + raise ValueError( + f"Invalid override type of {type} attempted for pack {pack_name}" + ) override_dir = os.path.join(cfg.CONF.system.base_path, "configs/overrides") # Apply global overrides @@ -315,7 +318,9 @@ def override(self, pack_name, type, content): return content - def _apply_override_file(self, override_file, pack_name, type, content, global_file): + def _apply_override_file( + self, override_file, pack_name, type, content, global_file + ): """ Loads override content from override file @@ -348,9 +353,13 @@ def _apply_override_file(self, override_file, pack_name, type, content, global_f for key in type_override["defaults"].keys(): if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["defaults"][key] - LOG.info(f'Overridden {type} {pack_name}.{name} {key} to default value of {content[key]} from {override_file}') + LOG.info( + f"Overridden {type} {pack_name}.{name} {key} to default value of {content[key]} from {override_file}" + ) else: - raise ValueError(f'Override attempted with invalid default key {key} in pack {pack_name}' ) + raise ValueError( + f"Override attempted with invalid default key {key} in pack {pack_name}" + ) if global_file: # No exceptions required in global content file @@ -361,9 +370,13 @@ def _apply_override_file(self, override_file, pack_name, type, content, global_f for key in type_override["exceptions"][name].keys(): if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["exceptions"][name][key] - LOG.info(f'Overridden {type} {pack_name}.{name} {key} to exception value of {content[key]} from {override_file}') + LOG.info( + f"Overridden {type} {pack_name}.{name} {key} to exception value of {content[key]} from {override_file}" + ) else: - raise ValueError(f'Override attempted with invalid exceptions key {key} in pack {pack_name}' ) + raise ValueError( + f"Override attempted with invalid exceptions key {key} in pack {pack_name}" + ) return content diff --git a/st2common/tests/unit/test_content_loader.py b/st2common/tests/unit/test_content_loader.py index 5e8441f1de..005462c70a 100644 --- a/st2common/tests/unit/test_content_loader.py +++ b/st2common/tests/unit/test_content_loader.py @@ -121,10 +121,10 @@ def test_get_override_action_from_default(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"name":"action1","enabled": True} + content = {"name": "action1", "enabled": True} loader.override("overpack1", "actions", content) self.assertFalse(content["enabled"]) - content = {"name":"action1","enabled": False} + content = {"name": "action1", "enabled": False} loader.override("overpack1", "actions", content) self.assertFalse(content["enabled"]) @@ -132,10 +132,10 @@ def test_get_override_action_from_exception(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"name":"action2","enabled": True} + content = {"name": "action2", "enabled": True} loader.override("overpack1", "actions", content) self.assertTrue(content["enabled"]) - content = {"name":"action2","enabled": False} + content = {"name": "action2", "enabled": False} loader.override("overpack1", "actions", content) self.assertTrue(content["enabled"]) @@ -143,10 +143,10 @@ def test_get_override_action_from_default_no_exceptions(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"name":"action1","enabled": True} + content = {"name": "action1", "enabled": True} loader.override("overpack4", "actions", content) self.assertFalse(content["enabled"]) - content = {"name":"action2","enabled": True} + content = {"name": "action2", "enabled": True} loader.override("overpack4", "actions", content) self.assertFalse(content["enabled"]) @@ -154,7 +154,7 @@ def test_get_override_action_from_global_default_no_exceptions(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"class_name":"sensor1","enabled": True} + content = {"class_name": "sensor1", "enabled": True} loader.override("overpack1", "sensors", content) self.assertFalse(content["enabled"]) @@ -162,7 +162,7 @@ def test_get_override_action_from_global_overridden_by_pack(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"class_name":"sensor1","enabled": True} + content = {"class_name": "sensor1", "enabled": True} loader.override("overpack2", "sensors", content) self.assertTrue(content["enabled"]) @@ -170,7 +170,7 @@ def test_get_override_action_from_global_overridden_by_pack_exception(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"class_name":"sensor1","enabled": True} + content = {"class_name": "sensor1", "enabled": True} loader.override("overpack3", "sensors", content) self.assertTrue(content["enabled"]) @@ -178,43 +178,44 @@ def test_get_override_invalid_type(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"name":"action2","enabled": True} + content = {"name": "action2", "enabled": True} self.assertRaises( ValueError, loader.override, pack_name="overpack1", type="wrongtype", - content=content + content=content, ) def test_get_override_invalid_default_key(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"name":"action1","enabled": True} + content = {"name": "action1", "enabled": True} self.assertRaises( ValueError, loader.override, pack_name="overpack2", type="actions", - content=content + content=content, ) def test_get_override_invalid_exceptions_key(self): config.parse_args() cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() - content = {"name":"action1","enabled": True} + content = {"name": "action1", "enabled": True} loader.override("overpack1", "actions", content) - content = {"name":"action2","enabled": True} + content = {"name": "action2", "enabled": True} self.assertRaises( ValueError, loader.override, pack_name="overpack3", type="actions", - content=content + content=content, ) + class YamlLoaderTestCase(unittest2.TestCase): def test_yaml_safe_load(self): # Verify C version of yaml loader indeed doesn't load non-safe data @@ -254,6 +255,7 @@ class Foo(object): a = "1" b = "c" + class YamlLoaderTestCase(unittest2.TestCase): def test_yaml_safe_load(self): # Verify C version of yaml loader indeed doesn't load non-safe data From d7a1435102d711b9ef9480cc7db3b62658f2a907 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Sun, 19 Dec 2021 12:08:36 +0000 Subject: [PATCH 05/18] Update test_content_loader.py --- st2common/tests/unit/test_content_loader.py | 40 --------------------- 1 file changed, 40 deletions(-) diff --git a/st2common/tests/unit/test_content_loader.py b/st2common/tests/unit/test_content_loader.py index 005462c70a..b964a95c1e 100644 --- a/st2common/tests/unit/test_content_loader.py +++ b/st2common/tests/unit/test_content_loader.py @@ -254,43 +254,3 @@ def test_yaml_safe_load(self): class Foo(object): a = "1" b = "c" - - -class YamlLoaderTestCase(unittest2.TestCase): - def test_yaml_safe_load(self): - # Verify C version of yaml loader indeed doesn't load non-safe data - dumped = yaml.dump(Foo) - self.assertTrue("!!python" in dumped) - - # Regular full load should work, but safe wrapper should fail - result = yaml.load(dumped, Loader=FullLoader) - self.assertTrue(result) - - self.assertRaisesRegexp( - yaml.constructor.ConstructorError, - "could not determine a constructor", - yaml_safe_load, - dumped, - ) - - self.assertRaisesRegexp( - yaml.constructor.ConstructorError, - "could not determine a constructor", - yaml.load, - dumped, - Loader=SafeLoader, - ) - - if CSafeLoader: - self.assertRaisesRegexp( - yaml.constructor.ConstructorError, - "could not determine a constructor", - yaml.load, - dumped, - Loader=CSafeLoader, - ) - - -class Foo(object): - a = "1" - b = "c" From e3c8352dae0380a82d192747cb05f3d9f25a3070 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Fri, 7 Jan 2022 15:14:21 +0000 Subject: [PATCH 06/18] Add review comments --- st2common/st2common/content/loader.py | 28 +++++++++---------- .../{configs => }/overrides/global.yaml | 0 .../{configs => }/overrides/overpack1.yaml | 0 .../{configs => }/overrides/overpack2.yaml | 0 .../{configs => }/overrides/overpack3.yaml | 0 .../{configs => }/overrides/overpack4.yaml | 0 st2common/tests/unit/test_content_loader.py | 6 ++-- 7 files changed, 17 insertions(+), 17 deletions(-) rename st2common/tests/resources/{configs => }/overrides/global.yaml (100%) rename st2common/tests/resources/{configs => }/overrides/overpack1.yaml (100%) rename st2common/tests/resources/{configs => }/overrides/overpack2.yaml (100%) rename st2common/tests/resources/{configs => }/overrides/overpack3.yaml (100%) rename st2common/tests/resources/{configs => }/overrides/overpack4.yaml (100%) diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 92ac9966c5..7878231635 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -288,38 +288,38 @@ class OverrideLoader(object): "enabled", ] - def override(self, pack_name, type, content): + def override(self, pack_name, resource_type, content): """ Loads override content for pack, and updates content :param pack_name: Name of pack :type pack_name: ``str`` - :param type: Type of resource loading + :param resource_type: Type of resource loading :type type: ``str`` :param content: Content as loaded from meta information :type content: ``object`` """ - if type not in self.ALLOWED_OVERRIDE_TYPES.keys(): + if resource_type not in self.ALLOWED_OVERRIDE_TYPES.keys(): raise ValueError( - f"Invalid override type of {type} attempted for pack {pack_name}" + f"Invalid override type of {resource_type} attempted for pack {pack_name}" ) - override_dir = os.path.join(cfg.CONF.system.base_path, "configs/overrides") + override_dir = os.path.join(cfg.CONF.system.base_path, "overrides") # Apply global overrides global_file = os.path.join(override_dir, "global.yaml") - self._apply_override_file(global_file, pack_name, type, content, True) + self._apply_override_file(global_file, pack_name, resource_type, content, True) # Apply pack overrides override_file = os.path.join(override_dir, f"{pack_name}.yaml") - self._apply_override_file(override_file, pack_name, type, content, False) + self._apply_override_file(override_file, pack_name, resource_type, content, False) return content def _apply_override_file( - self, override_file, pack_name, type, content, global_file + self, override_file, pack_name, resource_type, content, global_file ): """ @@ -329,7 +329,7 @@ def _apply_override_file( :type override_file: ``str`` :param pack_name: Name of pack :type pack_name: ``str`` - :param type: Type of resource loading + :param resource_type: Type of resource loading :type type: ``str`` :param content: Content as loaded from meta information :type content: ``object`` @@ -346,15 +346,15 @@ def _apply_override_file( file_name, file_ext = os.path.splitext(override_file) overrides = self._load(PARSER_FUNCS[file_ext], override_file) # Apply overrides - if type in overrides.keys(): - type_override = overrides[type] - name = content[self.ALLOWED_OVERRIDE_TYPES[type]] + if resource_type in overrides.keys(): + type_override = overrides[resource_type] + name = content[self.ALLOWED_OVERRIDE_TYPES[resource_type]] if "defaults" in type_override.keys(): for key in type_override["defaults"].keys(): if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["defaults"][key] LOG.info( - f"Overridden {type} {pack_name}.{name} {key} to default value of {content[key]} from {override_file}" + f"Overridden {resource_type} {pack_name}.{name} {key} to default value of {content[key]} from {override_file}" ) else: raise ValueError( @@ -371,7 +371,7 @@ def _apply_override_file( if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["exceptions"][name][key] LOG.info( - f"Overridden {type} {pack_name}.{name} {key} to exception value of {content[key]} from {override_file}" + f"Overridden {resource_type} {pack_name}.{name} {key} to exception value of {content[key]} from {override_file}" ) else: raise ValueError( diff --git a/st2common/tests/resources/configs/overrides/global.yaml b/st2common/tests/resources/overrides/global.yaml similarity index 100% rename from st2common/tests/resources/configs/overrides/global.yaml rename to st2common/tests/resources/overrides/global.yaml diff --git a/st2common/tests/resources/configs/overrides/overpack1.yaml b/st2common/tests/resources/overrides/overpack1.yaml similarity index 100% rename from st2common/tests/resources/configs/overrides/overpack1.yaml rename to st2common/tests/resources/overrides/overpack1.yaml diff --git a/st2common/tests/resources/configs/overrides/overpack2.yaml b/st2common/tests/resources/overrides/overpack2.yaml similarity index 100% rename from st2common/tests/resources/configs/overrides/overpack2.yaml rename to st2common/tests/resources/overrides/overpack2.yaml diff --git a/st2common/tests/resources/configs/overrides/overpack3.yaml b/st2common/tests/resources/overrides/overpack3.yaml similarity index 100% rename from st2common/tests/resources/configs/overrides/overpack3.yaml rename to st2common/tests/resources/overrides/overpack3.yaml diff --git a/st2common/tests/resources/configs/overrides/overpack4.yaml b/st2common/tests/resources/overrides/overpack4.yaml similarity index 100% rename from st2common/tests/resources/configs/overrides/overpack4.yaml rename to st2common/tests/resources/overrides/overpack4.yaml diff --git a/st2common/tests/unit/test_content_loader.py b/st2common/tests/unit/test_content_loader.py index 005462c70a..1dee6f65ce 100644 --- a/st2common/tests/unit/test_content_loader.py +++ b/st2common/tests/unit/test_content_loader.py @@ -183,7 +183,7 @@ def test_get_override_invalid_type(self): ValueError, loader.override, pack_name="overpack1", - type="wrongtype", + resource_type="wrongtype", content=content, ) @@ -196,7 +196,7 @@ def test_get_override_invalid_default_key(self): ValueError, loader.override, pack_name="overpack2", - type="actions", + resource_type="actions", content=content, ) @@ -211,7 +211,7 @@ def test_get_override_invalid_exceptions_key(self): ValueError, loader.override, pack_name="overpack3", - type="actions", + resource_type="actions", content=content, ) From 6200478c2376e00febb75a450d0a1dd666e254f4 Mon Sep 17 00:00:00 2001 From: amanda Date: Mon, 31 Jan 2022 11:27:12 +0000 Subject: [PATCH 07/18] Change level of logging so doesn't appear in output when do a st2ctl reload --- st2common/st2common/content/loader.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 7878231635..7692ebc94a 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -339,7 +339,7 @@ def _apply_override_file( if not os.path.exists(override_file): # No override file for pack - LOG.info(f"No override file {override_file} found") + LOG.debug(f"No override file {override_file} found") return content # Read override file @@ -353,7 +353,7 @@ def _apply_override_file( for key in type_override["defaults"].keys(): if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["defaults"][key] - LOG.info( + LOG.debug( f"Overridden {resource_type} {pack_name}.{name} {key} to default value of {content[key]} from {override_file}" ) else: @@ -370,7 +370,7 @@ def _apply_override_file( for key in type_override["exceptions"][name].keys(): if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["exceptions"][name][key] - LOG.info( + LOG.debug( f"Overridden {resource_type} {pack_name}.{name} {key} to exception value of {content[key]} from {override_file}" ) else: From 24150568b8966d411058eff3faa92708f7a22aaa Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Mon, 7 Feb 2022 13:42:53 +0000 Subject: [PATCH 08/18] Add output to reload to indicate number of resources where metadata is overridden, and also to result output on packs.load --- .../tests/unit/test_actions_registrar.py | 2 +- st2api/st2api/controllers/v1/packs.py | 15 ++++++-- .../st2common/bootstrap/actionsregistrar.py | 30 +++++++++------ .../st2common/bootstrap/aliasesregistrar.py | 37 +++++++++++-------- .../st2common/bootstrap/rulesregistrar.py | 28 ++++++++------ .../st2common/bootstrap/sensorsregistrar.py | 24 +++++++----- st2common/st2common/content/bootstrap.py | 16 ++++++-- st2common/st2common/content/loader.py | 31 +++++++++++----- st2common/tests/unit/test_content_loader.py | 18 ++++----- 9 files changed, 129 insertions(+), 72 deletions(-) diff --git a/st2actions/tests/unit/test_actions_registrar.py b/st2actions/tests/unit/test_actions_registrar.py index cc9da33299..d01b83a05f 100644 --- a/st2actions/tests/unit/test_actions_registrar.py +++ b/st2actions/tests/unit/test_actions_registrar.py @@ -112,7 +112,7 @@ def test_register_action_with_no_params(self): "generic", "actions", "action-with-no-parameters.yaml" ) - self.assertEqual(registrar._register_action("dummy", action_file), None) + self.assertEqual(registrar._register_action("dummy", action_file), False) @mock.patch.object( action_validator, "_is_valid_pack", mock.MagicMock(return_value=True) diff --git a/st2api/st2api/controllers/v1/packs.py b/st2api/st2api/controllers/v1/packs.py index 7aacae7471..6d963ce704 100644 --- a/st2api/st2api/controllers/v1/packs.py +++ b/st2api/st2api/controllers/v1/packs.py @@ -201,10 +201,19 @@ def post(self, pack_register_request): pack_path = content_utils.get_pack_base_path(pack) try: - registered_count = registrar.register_from_pack( + res = registrar.register_from_pack( pack_dir=pack_path ) - result[name] += registered_count + # Where overridding is supported return is tuple of + # (registered,overridden) else its just registered + # count return + if isinstance(res, tuple): + result[name] += res[0] + if res[1] != 0: + result[f"{name}(overridden)"] = res[1] + else: + result[name] += res + except ValueError as e: # Throw more user-friendly exception if requsted pack doesn't exist if re.match( @@ -219,7 +228,7 @@ def post(self, pack_register_request): raise e else: packs_base_paths = content_utils.get_packs_base_paths() - registered_count = registrar.register_from_packs( + registered_count, overridden_count = registrar.register_from_packs( base_dirs=packs_base_paths ) result[name] += registered_count diff --git a/st2common/st2common/bootstrap/actionsregistrar.py b/st2common/st2common/bootstrap/actionsregistrar.py index 074a5cec24..a575e2cec4 100644 --- a/st2common/st2common/bootstrap/actionsregistrar.py +++ b/st2common/st2common/bootstrap/actionsregistrar.py @@ -43,13 +43,14 @@ def register_from_packs(self, base_dirs): Discover all the packs in the provided directory and register actions from all of the discovered packs. - :return: Number of actions registered. - :rtype: ``int`` + :return: Number of actions registered, Number of actions overridden + :rtype: ``tuple`` """ # Register packs first self.register_packs(base_dirs=base_dirs) registered_count = 0 + overridden_count = 0 content = self._pack_loader.get_content( base_dirs=base_dirs, content_type="actions" ) @@ -63,8 +64,9 @@ def register_from_packs(self, base_dirs): "Registering actions from pack %s:, dir: %s", pack, actions_dir ) actions = self._get_actions_from_pack(actions_dir) - count = self._register_actions_from_pack(pack=pack, actions=actions) + count, overridden = self._register_actions_from_pack(pack=pack, actions=actions) registered_count += count + overridden_count += overridden except Exception as e: if self._fail_on_failure: raise e @@ -73,14 +75,14 @@ def register_from_packs(self, base_dirs): "Failed registering all actions from pack: %s", actions_dir ) - return registered_count + return registered_count, overridden_count def register_from_pack(self, pack_dir): """ Register all the actions from the provided pack. - :return: Number of actions registered. - :rtype: ``int`` + :return: Number of actions registered, Number of actions overridden + :rtype: ``tuple`` """ pack_dir = pack_dir[:-1] if pack_dir.endswith("/") else pack_dir _, pack = os.path.split(pack_dir) @@ -92,6 +94,7 @@ def register_from_pack(self, pack_dir): self.register_pack(pack_name=pack, pack_dir=pack_dir) registered_count = 0 + overridden_count = 0 if not actions_dir: return registered_count @@ -99,7 +102,7 @@ def register_from_pack(self, pack_dir): try: actions = self._get_actions_from_pack(actions_dir=actions_dir) - registered_count = self._register_actions_from_pack( + registered_count, overridden_count = self._register_actions_from_pack( pack=pack, actions=actions ) except Exception as e: @@ -108,7 +111,7 @@ def register_from_pack(self, pack_dir): LOG.exception("Failed registering all actions from pack: %s", actions_dir) - return registered_count + return registered_count, overridden_count def _get_actions_from_pack(self, actions_dir): actions = self.get_resources_from_pack(resources_dir=actions_dir) @@ -143,7 +146,7 @@ def _register_action(self, pack, action): content["metadata_file"] = metadata_file # Pass override information - self._override_loader.override(pack, "actions", content) + altered = self._override_loader.override(pack, "actions", content) action_api = ActionAPI(**content) @@ -217,12 +220,17 @@ def _register_action(self, pack, action): LOG.exception("Failed to write action to db %s.", model.name) raise + return altered + def _register_actions_from_pack(self, pack, actions): registered_count = 0 + overridden_count = 0 for action in actions: try: LOG.debug("Loading action from %s.", action) - self._register_action(pack=pack, action=action) + altered = self._register_action(pack=pack, action=action) + if altered: + overridden_count += 1 except Exception as e: if self._fail_on_failure: msg = 'Failed to register action "%s" from pack "%s": %s' % ( @@ -237,7 +245,7 @@ def _register_actions_from_pack(self, pack, actions): else: registered_count += 1 - return registered_count + return registered_count, overridden_count def register_actions( diff --git a/st2common/st2common/bootstrap/aliasesregistrar.py b/st2common/st2common/bootstrap/aliasesregistrar.py index 849dcd6ad8..2103a695d3 100644 --- a/st2common/st2common/bootstrap/aliasesregistrar.py +++ b/st2common/st2common/bootstrap/aliasesregistrar.py @@ -40,13 +40,14 @@ def register_from_packs(self, base_dirs): Discover all the packs in the provided directory and register aliases from all of the discovered packs. - :return: Number of aliases registered. - :rtype: ``int`` + :return: Tuple, Number of aliases registered, overridden. + :rtype: ``tuple`` """ # Register packs first self.register_packs(base_dirs=base_dirs) registered_count = 0 + overridden_count = 0 content = self._pack_loader.get_content( base_dirs=base_dirs, content_type="aliases" ) @@ -60,8 +61,9 @@ def register_from_packs(self, base_dirs): "Registering aliases from pack %s:, dir: %s", pack, aliases_dir ) aliases = self._get_aliases_from_pack(aliases_dir) - count = self._register_aliases_from_pack(pack=pack, aliases=aliases) + count, overridden = self._register_aliases_from_pack(pack=pack, aliases=aliases) registered_count += count + overridden_count += overridden except Exception as e: if self._fail_on_failure: raise e @@ -70,14 +72,14 @@ def register_from_packs(self, base_dirs): "Failed registering all aliases from pack: %s", aliases_dir ) - return registered_count + return registered_count, overridden_count def register_from_pack(self, pack_dir): """ Register all the aliases from the provided pack. - :return: Number of aliases registered. - :rtype: ``int`` + :return: Tuple, Number of aliases registered, overridden + :rtype: ``tuple`` """ pack_dir = pack_dir[:-1] if pack_dir.endswith("/") else pack_dir _, pack = os.path.split(pack_dir) @@ -89,14 +91,15 @@ def register_from_pack(self, pack_dir): self.register_pack(pack_name=pack, pack_dir=pack_dir) registered_count = 0 + overridden_count = 0 if not aliases_dir: - return registered_count + return registered_count, overridden_count LOG.debug("Registering aliases from pack %s:, dir: %s", pack, aliases_dir) try: aliases = self._get_aliases_from_pack(aliases_dir=aliases_dir) - registered_count = self._register_aliases_from_pack( + registered_count, overridden_count = self._register_aliases_from_pack( pack=pack, aliases=aliases ) except Exception as e: @@ -104,9 +107,9 @@ def register_from_pack(self, pack_dir): raise e LOG.exception("Failed registering all aliases from pack: %s", aliases_dir) - return registered_count + return registered_count, overridden_count - return registered_count + return registered_count, overridden_count def _get_aliases_from_pack(self, aliases_dir): return self.get_resources_from_pack(resources_dir=aliases_dir) @@ -145,16 +148,16 @@ def _get_action_alias_db( content["metadata_file"] = metadata_file # Pass override information - self._override_loader.override(pack, "aliases", content) + altered = self._override_loader.override(pack, "aliases", content) action_alias_api = ActionAliasAPI(**content) action_alias_api.validate() action_alias_db = ActionAliasAPI.to_model(action_alias_api) - return action_alias_db + return action_alias_db, altered def _register_action_alias(self, pack, action_alias): - action_alias_db = self._get_action_alias_db( + action_alias_db, altered = self._get_action_alias_db( pack=pack, action_alias=action_alias ) @@ -184,14 +187,18 @@ def _register_action_alias(self, pack, action_alias): except Exception: LOG.exception("Failed to create action alias %s.", action_alias_db.name) raise + return altered def _register_aliases_from_pack(self, pack, aliases): registered_count = 0 + overridden_count = 0 for alias in aliases: try: LOG.debug("Loading alias from %s.", alias) - self._register_action_alias(pack, alias) + altered = self._register_action_alias(pack, alias) + if altered: + overridden_count += 1 except Exception as e: if self._fail_on_failure: msg = 'Failed to register alias "%s" from pack "%s": %s' % ( @@ -206,7 +213,7 @@ def _register_aliases_from_pack(self, pack, aliases): else: registered_count += 1 - return registered_count + return registered_count, overridden_count def register_aliases( diff --git a/st2common/st2common/bootstrap/rulesregistrar.py b/st2common/st2common/bootstrap/rulesregistrar.py index b1b739e002..6fb5e66880 100644 --- a/st2common/st2common/bootstrap/rulesregistrar.py +++ b/st2common/st2common/bootstrap/rulesregistrar.py @@ -42,13 +42,14 @@ class RulesRegistrar(ResourceRegistrar): def register_from_packs(self, base_dirs): """ - :return: Number of rules registered. - :rtype: ``int`` + :return: Tuple, Number of rules registered, overridden + :rtype: ``tuple`` """ # Register packs first self.register_packs(base_dirs=base_dirs) registered_count = 0 + overridden_count = 0 content = self._pack_loader.get_content( base_dirs=base_dirs, content_type="rules" ) @@ -59,22 +60,23 @@ def register_from_packs(self, base_dirs): try: LOG.debug("Registering rules from pack: %s", pack) rules = self._get_rules_from_pack(rules_dir) - count = self._register_rules_from_pack(pack, rules) + count, override = self._register_rules_from_pack(pack, rules) registered_count += count + overridden_count += override except Exception as e: if self._fail_on_failure: raise e LOG.exception("Failed registering all rules from pack: %s", rules_dir) - return registered_count + return registered_count, overridden_count def register_from_pack(self, pack_dir): """ Register all the rules from the provided pack. - :return: Number of rules registered. - :rtype: ``int`` + :return: Number of rules registered, Number of rules overridden + :rtype: ``tuple`` """ pack_dir = pack_dir[:-1] if pack_dir.endswith("/") else pack_dir _, pack = os.path.split(pack_dir) @@ -86,27 +88,29 @@ def register_from_pack(self, pack_dir): self.register_pack(pack_name=pack, pack_dir=pack_dir) registered_count = 0 + overridden_count = 0 if not rules_dir: - return registered_count + return registered_count, overridden_count LOG.debug("Registering rules from pack %s:, dir: %s", pack, rules_dir) try: rules = self._get_rules_from_pack(rules_dir=rules_dir) - registered_count = self._register_rules_from_pack(pack=pack, rules=rules) + registered_count, overridden_count = self._register_rules_from_pack(pack=pack, rules=rules) except Exception as e: if self._fail_on_failure: raise e LOG.exception("Failed registering all rules from pack: %s", rules_dir) - return registered_count + return registered_count, overridden_count def _get_rules_from_pack(self, rules_dir): return self.get_resources_from_pack(resources_dir=rules_dir) def _register_rules_from_pack(self, pack, rules): registered_count = 0 + overridden_count = 0 # TODO: Refactor this monstrosity for rule in rules: @@ -129,7 +133,7 @@ def _register_rules_from_pack(self, pack, rules): content["metadata_file"] = metadata_file # Pass override information - self._override_loader.override(pack, "rules", content) + altered = self._override_loader.override(pack, "rules", content) rule_api = RuleAPI(**content) rule_api.validate() @@ -201,8 +205,10 @@ def _register_rules_from_pack(self, pack, rules): LOG.exception("Failed registering rule from %s.", rule) else: registered_count += 1 + if altered: + overridden_count += 1 - return registered_count + return registered_count, overridden_count def register_rules( diff --git a/st2common/st2common/bootstrap/sensorsregistrar.py b/st2common/st2common/bootstrap/sensorsregistrar.py index e22bef4ecd..b0c3903a83 100644 --- a/st2common/st2common/bootstrap/sensorsregistrar.py +++ b/st2common/st2common/bootstrap/sensorsregistrar.py @@ -48,6 +48,7 @@ def register_from_packs(self, base_dirs): self.register_packs(base_dirs=base_dirs) registered_count = 0 + overridden_count = 0 content = self._pack_loader.get_content( base_dirs=base_dirs, content_type="sensors" ) @@ -61,8 +62,9 @@ def register_from_packs(self, base_dirs): "Registering sensors from pack %s:, dir: %s", pack, sensors_dir ) sensors = self._get_sensors_from_pack(sensors_dir) - count = self._register_sensors_from_pack(pack=pack, sensors=sensors) + count, overridden = self._register_sensors_from_pack(pack=pack, sensors=sensors) registered_count += count + overridden_count += overridden except Exception as e: if self._fail_on_failure: raise e @@ -73,7 +75,7 @@ def register_from_packs(self, base_dirs): six.text_type(e), ) - return registered_count + return registered_count, overridden_count def register_from_pack(self, pack_dir): """ @@ -92,14 +94,15 @@ def register_from_pack(self, pack_dir): self.register_pack(pack_name=pack, pack_dir=pack_dir) registered_count = 0 + overridden_count = 0 if not sensors_dir: - return registered_count + return registered_count, overridden_count LOG.debug("Registering sensors from pack %s:, dir: %s", pack, sensors_dir) try: sensors = self._get_sensors_from_pack(sensors_dir=sensors_dir) - registered_count = self._register_sensors_from_pack( + registered_count, overridden_count = self._register_sensors_from_pack( pack=pack, sensors=sensors ) except Exception as e: @@ -112,16 +115,19 @@ def register_from_pack(self, pack_dir): six.text_type(e), ) - return registered_count + return registered_count, overridden_count def _get_sensors_from_pack(self, sensors_dir): return self.get_resources_from_pack(resources_dir=sensors_dir) def _register_sensors_from_pack(self, pack, sensors): registered_count = 0 + overridden_count = 0 for sensor in sensors: try: - self._register_sensor_from_pack(pack=pack, sensor=sensor) + _, altered = self._register_sensor_from_pack(pack=pack, sensor=sensor) + if altered: + overridden_count = overridden_count + 1 except Exception as e: if self._fail_on_failure: msg = 'Failed to register sensor "%s" from pack "%s": %s' % ( @@ -138,7 +144,7 @@ def _register_sensors_from_pack(self, pack, sensors): LOG.debug('Sensor "%s" successfully registered', sensor) registered_count += 1 - return registered_count + return (registered_count, overridden_count) def _register_sensor_from_pack(self, pack, sensor): sensor_metadata_file_path = sensor @@ -168,7 +174,7 @@ def _register_sensor_from_pack(self, pack, sensor): content["metadata_file"] = metadata_file # Pass override information - self._override_loader.override(pack, "sensors", content) + altered = self._override_loader.override(pack, "sensors", content) sensors_dir = os.path.dirname(sensor_metadata_file_path) sensor_file_path = os.path.join(sensors_dir, entry_point) @@ -194,7 +200,7 @@ def _register_sensor_from_pack(self, pack, sensor): except: LOG.exception("Failed creating sensor model for %s", sensor) - return sensor_model + return sensor_model, altered def register_sensors( diff --git a/st2common/st2common/content/bootstrap.py b/st2common/st2common/content/bootstrap.py index 9ae04664ad..888d33c8ef 100644 --- a/st2common/st2common/content/bootstrap.py +++ b/st2common/st2common/content/bootstrap.py @@ -193,13 +193,14 @@ def register_sensors(): fail_on_failure = not cfg.CONF.register.no_fail_on_failure registered_count = 0 + overridden_count = 0 try: LOG.info("=========================================================") LOG.info("############## Registering sensors ######################") LOG.info("=========================================================") with Timer(key="st2.register.sensors"): - registered_count = sensors_registrar.register_sensors( + (registered_count, overridden_count) = sensors_registrar.register_sensors( pack_dir=pack_dir, fail_on_failure=fail_on_failure ) except Exception as e: @@ -210,6 +211,7 @@ def register_sensors(): raise e LOG.info("Registered %s sensors." % (registered_count)) + LOG.info("%s sensors had their metadata overridden." % (overridden_count)) def register_runners(): @@ -245,13 +247,14 @@ def register_actions(): fail_on_failure = not cfg.CONF.register.no_fail_on_failure registered_count = 0 + overridden_count = 0 try: LOG.info("=========================================================") LOG.info("############## Registering actions ######################") LOG.info("=========================================================") with Timer(key="st2.register.actions"): - registered_count = actions_registrar.register_actions( + registered_count, overridden_count = actions_registrar.register_actions( pack_dir=pack_dir, fail_on_failure=fail_on_failure, use_runners_cache=True, @@ -264,6 +267,7 @@ def register_actions(): raise e LOG.info("Registered %s actions." % (registered_count)) + LOG.info("%s actions had their metadata overridden." % (overridden_count)) def register_rules(): @@ -272,6 +276,7 @@ def register_rules(): fail_on_failure = not cfg.CONF.register.no_fail_on_failure registered_count = 0 + overridden_count = 0 try: LOG.info("=========================================================") @@ -284,7 +289,7 @@ def register_rules(): try: with Timer(key="st2.register.rules"): - registered_count = rules_registrar.register_rules( + registered_count, overridden_count = rules_registrar.register_rules( pack_dir=pack_dir, fail_on_failure=fail_on_failure ) except Exception as e: @@ -295,6 +300,7 @@ def register_rules(): raise e LOG.info("Registered %s rules.", registered_count) + LOG.info("%s rules had their metadata overridden." % (overridden_count)) def register_aliases(): @@ -302,13 +308,14 @@ def register_aliases(): fail_on_failure = not cfg.CONF.register.no_fail_on_failure registered_count = 0 + overridden_count = 0 try: LOG.info("=========================================================") LOG.info("############## Registering aliases ######################") LOG.info("=========================================================") with Timer(key="st2.register.aliases"): - registered_count = aliases_registrar.register_aliases( + registered_count, overridden_count = aliases_registrar.register_aliases( pack_dir=pack_dir, fail_on_failure=fail_on_failure ) except Exception as e: @@ -318,6 +325,7 @@ def register_aliases(): LOG.warning("Failed to register aliases.", exc_info=True) LOG.info("Registered %s aliases.", registered_count) + LOG.info("%s aliases had their metadata overridden." % (overridden_count)) def register_policies(): diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 7692ebc94a..86df010a0b 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -288,6 +288,10 @@ class OverrideLoader(object): "enabled", ] + DEFAULT_OVERRIDE_VALUES = { + "enabled": True + } + def override(self, pack_name, resource_type, content): """ @@ -299,9 +303,12 @@ def override(self, pack_name, resource_type, content): :type type: ``str`` :param content: Content as loaded from meta information :type content: ``object`` + :return: Whether data was overridden + :rtype: ``bool`` + """ - + orig_content = content.copy() if resource_type not in self.ALLOWED_OVERRIDE_TYPES.keys(): raise ValueError( f"Invalid override type of {resource_type} attempted for pack {pack_name}" @@ -315,8 +322,18 @@ def override(self, pack_name, resource_type, content): # Apply pack overrides override_file = os.path.join(override_dir, f"{pack_name}.yaml") self._apply_override_file(override_file, pack_name, resource_type, content, False) - - return content + if content == orig_content: + overridden = False + else: + # Need to account for defaults that might not have been set + for key in self.ALLOWED_OVERRIDE_NAMES: + if not key in orig_content.keys() and key in content.keys(): + orig_content[key] = self.DEFAULT_OVERRIDE_VALUES[key] + if content == orig_content: + overridden = False + else: + overridden = True + return overridden def _apply_override_file( self, override_file, pack_name, resource_type, content, global_file @@ -333,14 +350,12 @@ def _apply_override_file( :type type: ``str`` :param content: Content as loaded from meta information :type content: ``object`` - :param global_file: Whether global file - :type global_file: ``bool`` """ if not os.path.exists(override_file): # No override file for pack LOG.debug(f"No override file {override_file} found") - return content + return # Read override file file_name, file_ext = os.path.splitext(override_file) @@ -363,7 +378,7 @@ def _apply_override_file( if global_file: # No exceptions required in global content file - return content + return if "exceptions" in type_override.keys(): if name in type_override["exceptions"]: @@ -378,8 +393,6 @@ def _apply_override_file( f"Override attempted with invalid exceptions key {key} in pack {pack_name}" ) - return content - def _load(self, parser_func, file_path): with open(file_path, "r", encoding="utf-8") as fd: try: diff --git a/st2common/tests/unit/test_content_loader.py b/st2common/tests/unit/test_content_loader.py index b41b56a79c..663d5f7844 100644 --- a/st2common/tests/unit/test_content_loader.py +++ b/st2common/tests/unit/test_content_loader.py @@ -122,10 +122,10 @@ def test_get_override_action_from_default(self): cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() content = {"name": "action1", "enabled": True} - loader.override("overpack1", "actions", content) + self.assertTrue(loader.override("overpack1", "actions", content)) self.assertFalse(content["enabled"]) content = {"name": "action1", "enabled": False} - loader.override("overpack1", "actions", content) + self.assertFalse(loader.override("overpack1", "actions", content)) self.assertFalse(content["enabled"]) def test_get_override_action_from_exception(self): @@ -133,10 +133,10 @@ def test_get_override_action_from_exception(self): cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() content = {"name": "action2", "enabled": True} - loader.override("overpack1", "actions", content) + self.assertFalse(loader.override("overpack1", "actions", content)) self.assertTrue(content["enabled"]) content = {"name": "action2", "enabled": False} - loader.override("overpack1", "actions", content) + self.assertTrue(loader.override("overpack1", "actions", content)) self.assertTrue(content["enabled"]) def test_get_override_action_from_default_no_exceptions(self): @@ -144,10 +144,10 @@ def test_get_override_action_from_default_no_exceptions(self): cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() content = {"name": "action1", "enabled": True} - loader.override("overpack4", "actions", content) + self.assertTrue(loader.override("overpack4", "actions", content)) self.assertFalse(content["enabled"]) content = {"name": "action2", "enabled": True} - loader.override("overpack4", "actions", content) + self.assertTrue(loader.override("overpack4", "actions", content)) self.assertFalse(content["enabled"]) def test_get_override_action_from_global_default_no_exceptions(self): @@ -155,7 +155,7 @@ def test_get_override_action_from_global_default_no_exceptions(self): cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() content = {"class_name": "sensor1", "enabled": True} - loader.override("overpack1", "sensors", content) + self.assertTrue(loader.override("overpack1", "sensors", content)) self.assertFalse(content["enabled"]) def test_get_override_action_from_global_overridden_by_pack(self): @@ -163,7 +163,7 @@ def test_get_override_action_from_global_overridden_by_pack(self): cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() content = {"class_name": "sensor1", "enabled": True} - loader.override("overpack2", "sensors", content) + self.assertFalse(loader.override("overpack2", "sensors", content)) self.assertTrue(content["enabled"]) def test_get_override_action_from_global_overridden_by_pack_exception(self): @@ -171,7 +171,7 @@ def test_get_override_action_from_global_overridden_by_pack_exception(self): cfg.CONF.set_override(name="base_path", override=RESOURCES_DIR, group="system") loader = OverrideLoader() content = {"class_name": "sensor1", "enabled": True} - loader.override("overpack3", "sensors", content) + self.assertFalse(loader.override("overpack3", "sensors", content)) self.assertTrue(content["enabled"]) def test_get_override_invalid_type(self): From df920abc1c9167ca1b564fdb2f5cf1197cb695ca Mon Sep 17 00:00:00 2001 From: amanda Date: Mon, 14 Feb 2022 13:50:01 +0000 Subject: [PATCH 09/18] Fix black error --- st2api/st2api/controllers/v1/packs.py | 6 ++---- st2common/st2common/bootstrap/actionsregistrar.py | 6 ++++-- st2common/st2common/bootstrap/aliasesregistrar.py | 4 +++- st2common/st2common/bootstrap/rulesregistrar.py | 4 +++- st2common/st2common/bootstrap/sensorsregistrar.py | 4 +++- st2common/st2common/content/loader.py | 10 +++++----- 6 files changed, 20 insertions(+), 14 deletions(-) diff --git a/st2api/st2api/controllers/v1/packs.py b/st2api/st2api/controllers/v1/packs.py index 6d963ce704..3d886bee6f 100644 --- a/st2api/st2api/controllers/v1/packs.py +++ b/st2api/st2api/controllers/v1/packs.py @@ -201,9 +201,7 @@ def post(self, pack_register_request): pack_path = content_utils.get_pack_base_path(pack) try: - res = registrar.register_from_pack( - pack_dir=pack_path - ) + res = registrar.register_from_pack(pack_dir=pack_path) # Where overridding is supported return is tuple of # (registered,overridden) else its just registered # count return @@ -213,7 +211,7 @@ def post(self, pack_register_request): result[f"{name}(overridden)"] = res[1] else: result[name] += res - + except ValueError as e: # Throw more user-friendly exception if requsted pack doesn't exist if re.match( diff --git a/st2common/st2common/bootstrap/actionsregistrar.py b/st2common/st2common/bootstrap/actionsregistrar.py index a575e2cec4..a514bf0138 100644 --- a/st2common/st2common/bootstrap/actionsregistrar.py +++ b/st2common/st2common/bootstrap/actionsregistrar.py @@ -64,7 +64,9 @@ def register_from_packs(self, base_dirs): "Registering actions from pack %s:, dir: %s", pack, actions_dir ) actions = self._get_actions_from_pack(actions_dir) - count, overridden = self._register_actions_from_pack(pack=pack, actions=actions) + count, overridden = self._register_actions_from_pack( + pack=pack, actions=actions + ) registered_count += count overridden_count += overridden except Exception as e: @@ -230,7 +232,7 @@ def _register_actions_from_pack(self, pack, actions): LOG.debug("Loading action from %s.", action) altered = self._register_action(pack=pack, action=action) if altered: - overridden_count += 1 + overridden_count += 1 except Exception as e: if self._fail_on_failure: msg = 'Failed to register action "%s" from pack "%s": %s' % ( diff --git a/st2common/st2common/bootstrap/aliasesregistrar.py b/st2common/st2common/bootstrap/aliasesregistrar.py index 2103a695d3..b2c5d3b306 100644 --- a/st2common/st2common/bootstrap/aliasesregistrar.py +++ b/st2common/st2common/bootstrap/aliasesregistrar.py @@ -61,7 +61,9 @@ def register_from_packs(self, base_dirs): "Registering aliases from pack %s:, dir: %s", pack, aliases_dir ) aliases = self._get_aliases_from_pack(aliases_dir) - count, overridden = self._register_aliases_from_pack(pack=pack, aliases=aliases) + count, overridden = self._register_aliases_from_pack( + pack=pack, aliases=aliases + ) registered_count += count overridden_count += overridden except Exception as e: diff --git a/st2common/st2common/bootstrap/rulesregistrar.py b/st2common/st2common/bootstrap/rulesregistrar.py index 6fb5e66880..ac66008194 100644 --- a/st2common/st2common/bootstrap/rulesregistrar.py +++ b/st2common/st2common/bootstrap/rulesregistrar.py @@ -96,7 +96,9 @@ def register_from_pack(self, pack_dir): try: rules = self._get_rules_from_pack(rules_dir=rules_dir) - registered_count, overridden_count = self._register_rules_from_pack(pack=pack, rules=rules) + registered_count, overridden_count = self._register_rules_from_pack( + pack=pack, rules=rules + ) except Exception as e: if self._fail_on_failure: raise e diff --git a/st2common/st2common/bootstrap/sensorsregistrar.py b/st2common/st2common/bootstrap/sensorsregistrar.py index b0c3903a83..ee7f61b4a0 100644 --- a/st2common/st2common/bootstrap/sensorsregistrar.py +++ b/st2common/st2common/bootstrap/sensorsregistrar.py @@ -62,7 +62,9 @@ def register_from_packs(self, base_dirs): "Registering sensors from pack %s:, dir: %s", pack, sensors_dir ) sensors = self._get_sensors_from_pack(sensors_dir) - count, overridden = self._register_sensors_from_pack(pack=pack, sensors=sensors) + count, overridden = self._register_sensors_from_pack( + pack=pack, sensors=sensors + ) registered_count += count overridden_count += overridden except Exception as e: diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 86df010a0b..3d81914b5b 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -288,9 +288,7 @@ class OverrideLoader(object): "enabled", ] - DEFAULT_OVERRIDE_VALUES = { - "enabled": True - } + DEFAULT_OVERRIDE_VALUES = {"enabled": True} def override(self, pack_name, resource_type, content): @@ -305,7 +303,7 @@ def override(self, pack_name, resource_type, content): :type content: ``object`` :return: Whether data was overridden :rtype: ``bool`` - + """ orig_content = content.copy() @@ -321,7 +319,9 @@ def override(self, pack_name, resource_type, content): # Apply pack overrides override_file = os.path.join(override_dir, f"{pack_name}.yaml") - self._apply_override_file(override_file, pack_name, resource_type, content, False) + self._apply_override_file( + override_file, pack_name, resource_type, content, False + ) if content == orig_content: overridden = False else: From e8ea7a419c773731b0c1ec4f09712571346fc923 Mon Sep 17 00:00:00 2001 From: amanda Date: Mon, 14 Feb 2022 14:02:12 +0000 Subject: [PATCH 10/18] Fix flake8 --- st2common/st2common/content/loader.py | 2 +- st2tests/st2tests/action_aliases.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 3d81914b5b..e37a80bed0 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -327,7 +327,7 @@ def override(self, pack_name, resource_type, content): else: # Need to account for defaults that might not have been set for key in self.ALLOWED_OVERRIDE_NAMES: - if not key in orig_content.keys() and key in content.keys(): + if key not in orig_content.keys() and key in content.keys(): orig_content[key] = self.DEFAULT_OVERRIDE_VALUES[key] if content == orig_content: overridden = False diff --git a/st2tests/st2tests/action_aliases.py b/st2tests/st2tests/action_aliases.py index 88f02f9642..3fd3dee80e 100644 --- a/st2tests/st2tests/action_aliases.py +++ b/st2tests/st2tests/action_aliases.py @@ -130,7 +130,7 @@ def _get_action_alias_db_by_name(self, name): ) aliases = registrar._get_aliases_from_pack(aliases_dir=aliases_path) for alias_path in aliases: - action_alias_db = registrar._get_action_alias_db( + action_alias_db, altered = registrar._get_action_alias_db( pack=pack, action_alias=alias_path, ignore_metadata_file_error=True ) From 7938dceb3f296f55be0648e53216bba162a5f371 Mon Sep 17 00:00:00 2001 From: amanda Date: Mon, 14 Feb 2022 14:35:24 +0000 Subject: [PATCH 11/18] Fix UT --- st2api/st2api/controllers/v1/packs.py | 14 ++++++++++---- st2common/st2common/bootstrap/sensorsregistrar.py | 4 ++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/st2api/st2api/controllers/v1/packs.py b/st2api/st2api/controllers/v1/packs.py index 3d886bee6f..2f9706acdf 100644 --- a/st2api/st2api/controllers/v1/packs.py +++ b/st2api/st2api/controllers/v1/packs.py @@ -226,10 +226,16 @@ def post(self, pack_register_request): raise e else: packs_base_paths = content_utils.get_packs_base_paths() - registered_count, overridden_count = registrar.register_from_packs( - base_dirs=packs_base_paths - ) - result[name] += registered_count + res = registrar.register_from_packs(base_dirs=packs_base_paths) + # Where overridding is supported return is tuple of + # (registered,overridden) else its just registered + # count return + if isinstance(res, tuple): + result[name] += res[0] + if res[1] != 0: + result[f"{name}(overridden)"] = res[1] + else: + result[name] += res return result diff --git a/st2common/st2common/bootstrap/sensorsregistrar.py b/st2common/st2common/bootstrap/sensorsregistrar.py index ee7f61b4a0..6105f1c13e 100644 --- a/st2common/st2common/bootstrap/sensorsregistrar.py +++ b/st2common/st2common/bootstrap/sensorsregistrar.py @@ -41,8 +41,8 @@ def register_from_packs(self, base_dirs): Discover all the packs in the provided directory and register sensors from all of the discovered packs. - :return: Number of sensors registered. - :rtype: ``int`` + :return: Number of sensors registered, overridde + :rtype: ``tuple`` """ # Register packs first self.register_packs(base_dirs=base_dirs) From 27da72275a8bb2f0f061c45b34ce621215f1643e Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Mon, 14 Feb 2022 15:04:23 +0000 Subject: [PATCH 12/18] Fix UT --- st2api/st2api/controllers/v1/packs.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2api/st2api/controllers/v1/packs.py b/st2api/st2api/controllers/v1/packs.py index 2f9706acdf..0642c2f90e 100644 --- a/st2api/st2api/controllers/v1/packs.py +++ b/st2api/st2api/controllers/v1/packs.py @@ -234,8 +234,8 @@ def post(self, pack_register_request): result[name] += res[0] if res[1] != 0: result[f"{name}(overridden)"] = res[1] - else: - result[name] += res + else: + result[name] += res return result From 3635f4cff2ed19ed4e0fe34f5d55cf589cfadb37 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Mon, 14 Feb 2022 15:19:47 +0000 Subject: [PATCH 13/18] Fix UT --- st2common/tests/unit/test_aliasesregistrar.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2common/tests/unit/test_aliasesregistrar.py b/st2common/tests/unit/test_aliasesregistrar.py index 4f17246dcf..79995a98be 100644 --- a/st2common/tests/unit/test_aliasesregistrar.py +++ b/st2common/tests/unit/test_aliasesregistrar.py @@ -33,7 +33,7 @@ class TestAliasRegistrar(DbTestCase): def test_alias_registration(self): - count = aliasesregistrar.register_aliases(pack_dir=ALIASES_FIXTURE_PACK_PATH) + count, overridden = aliasesregistrar.register_aliases(pack_dir=ALIASES_FIXTURE_PACK_PATH) # expect all files to contain be aliases self.assertEqual(count, len(os.listdir(ALIASES_FIXTURE_PATH))) From 70035fc7f960ee35e7f1f032c5b502a258e8c49f Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Mon, 14 Feb 2022 15:32:53 +0000 Subject: [PATCH 14/18] Fix Black UT --- st2common/tests/unit/test_aliasesregistrar.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/st2common/tests/unit/test_aliasesregistrar.py b/st2common/tests/unit/test_aliasesregistrar.py index 79995a98be..714a0c072a 100644 --- a/st2common/tests/unit/test_aliasesregistrar.py +++ b/st2common/tests/unit/test_aliasesregistrar.py @@ -33,7 +33,9 @@ class TestAliasRegistrar(DbTestCase): def test_alias_registration(self): - count, overridden = aliasesregistrar.register_aliases(pack_dir=ALIASES_FIXTURE_PACK_PATH) + count, overridden = aliasesregistrar.register_aliases( + pack_dir=ALIASES_FIXTURE_PACK_PATH + ) # expect all files to contain be aliases self.assertEqual(count, len(os.listdir(ALIASES_FIXTURE_PATH))) From 3810862412944f619b439daf4133028ad7ac5745 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Thu, 17 Feb 2022 16:33:34 +0000 Subject: [PATCH 15/18] Apply suggestions from code review Co-authored-by: Jacob Floyd --- st2common/st2common/content/loader.py | 12 +++++------- st2common/tests/resources/overrides/global.yaml | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index e37a80bed0..50bfe27486 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -303,8 +303,6 @@ def override(self, pack_name, resource_type, content): :type content: ``object`` :return: Whether data was overridden :rtype: ``bool`` - - """ orig_content = content.copy() if resource_type not in self.ALLOWED_OVERRIDE_TYPES.keys(): @@ -361,11 +359,11 @@ def _apply_override_file( file_name, file_ext = os.path.splitext(override_file) overrides = self._load(PARSER_FUNCS[file_ext], override_file) # Apply overrides - if resource_type in overrides.keys(): + if resource_type in overrides: type_override = overrides[resource_type] name = content[self.ALLOWED_OVERRIDE_TYPES[resource_type]] - if "defaults" in type_override.keys(): - for key in type_override["defaults"].keys(): + if "defaults" in type_override: + for key in type_override["defaults"]: if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["defaults"][key] LOG.debug( @@ -380,9 +378,9 @@ def _apply_override_file( # No exceptions required in global content file return - if "exceptions" in type_override.keys(): + if "exceptions" in type_override: if name in type_override["exceptions"]: - for key in type_override["exceptions"][name].keys(): + for key in type_override["exceptions"][name]: if key in self.ALLOWED_OVERRIDE_NAMES: content[key] = type_override["exceptions"][name][key] LOG.debug( diff --git a/st2common/tests/resources/overrides/global.yaml b/st2common/tests/resources/overrides/global.yaml index d67d1f2ca0..c90008b58b 100644 --- a/st2common/tests/resources/overrides/global.yaml +++ b/st2common/tests/resources/overrides/global.yaml @@ -1,4 +1,4 @@ --- sensors: - defaults: - enabled: false + defaults: + enabled: false From 455f890dcfe9d11a52dc98c1fa96223dfae457e8 Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Fri, 4 Mar 2022 16:05:16 +0000 Subject: [PATCH 16/18] Add reserved pack list name - currently just global --- st2common/st2common/constants/pack.py | 4 ++++ st2common/st2common/util/pack.py | 5 +++++ st2common/st2common/util/virtualenvs.py | 6 ++++++ st2common/tests/unit/test_aliasesregistrar.py | 2 ++ st2common/tests/unit/test_util_pack.py | 19 +++++++++++++++++++ st2common/tests/unit/test_virtualenvs.py | 14 ++++++++++++++ 6 files changed, 50 insertions(+) diff --git a/st2common/st2common/constants/pack.py b/st2common/st2common/constants/pack.py index f782a6920c..99259e75fc 100644 --- a/st2common/st2common/constants/pack.py +++ b/st2common/st2common/constants/pack.py @@ -16,6 +16,7 @@ __all__ = [ "PACKS_PACK_NAME", "PACK_REF_WHITELIST_REGEX", + "RESERVED_PACK_LIST", "PACK_RESERVED_CHARACTERS", "PACK_VERSION_SEPARATOR", "PACK_VERSION_REGEX", @@ -37,6 +38,9 @@ # A list of allowed characters for the pack name PACK_REF_WHITELIST_REGEX = r"^[a-z0-9_]+$" +# A list of reserved pack names that cannot be used +RESERVED_PACK_LIST = ["global"] + # Check for a valid semver string PACK_VERSION_REGEX = r"^(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-[\da-z\-]+(?:\.[\da-z\-]+)*)?(?:\+[\da-z\-]+(?:\.[\da-z\-]+)*)?$" # noqa diff --git a/st2common/st2common/util/pack.py b/st2common/st2common/util/pack.py index 4dadba87cb..0c13ccf347 100644 --- a/st2common/st2common/util/pack.py +++ b/st2common/st2common/util/pack.py @@ -24,6 +24,7 @@ from st2common.util import schema as util_schema from st2common.constants.pack import MANIFEST_FILE_NAME from st2common.constants.pack import PACK_REF_WHITELIST_REGEX +from st2common.constants.pack import RESERVED_PACK_LIST from st2common.content.loader import MetaLoader from st2common.persistence.pack import Pack from st2common.exceptions.apivalidation import ValueValidationException @@ -88,6 +89,10 @@ def get_pack_ref_from_metadata(metadata, pack_directory_name=None): ) raise ValueError(msg % (metadata["name"])) + if pack_ref in RESERVED_PACK_LIST: + raise ValueError( + f"{pack_ref} is a reserved name, and cannot be used as a pack name" + ) return pack_ref diff --git a/st2common/st2common/util/virtualenvs.py b/st2common/st2common/util/virtualenvs.py index ad64f1e3af..20100369b0 100644 --- a/st2common/st2common/util/virtualenvs.py +++ b/st2common/st2common/util/virtualenvs.py @@ -29,6 +29,7 @@ from st2common import log as logging from st2common.constants.pack import PACK_REF_WHITELIST_REGEX from st2common.constants.pack import BASE_PACK_REQUIREMENTS +from st2common.constants.pack import RESERVED_PACK_LIST from st2common.util.shell import run_command from st2common.util.shell import quote_unix from st2common.util.compat import to_ascii @@ -74,6 +75,11 @@ def setup_pack_virtualenv( if not re.match(PACK_REF_WHITELIST_REGEX, pack_name): raise ValueError('Invalid pack name "%s"' % (pack_name)) + if pack_name in RESERVED_PACK_LIST: + raise ValueError( + f"Pack name {pack_name} is a reserved name, and cannot be used" + ) + base_virtualenvs_path = os.path.join(cfg.CONF.system.base_path, "virtualenvs/") virtualenv_path = os.path.join(base_virtualenvs_path, quote_unix(pack_name)) diff --git a/st2common/tests/unit/test_aliasesregistrar.py b/st2common/tests/unit/test_aliasesregistrar.py index 714a0c072a..38d86a2f9f 100644 --- a/st2common/tests/unit/test_aliasesregistrar.py +++ b/st2common/tests/unit/test_aliasesregistrar.py @@ -38,6 +38,8 @@ def test_alias_registration(self): ) # expect all files to contain be aliases self.assertEqual(count, len(os.listdir(ALIASES_FIXTURE_PATH))) + # Nothing overridden + self.assertEqual(0, overridden) action_alias_dbs = ActionAlias.get_all() self.assertEqual(action_alias_dbs[0].metadata_file, "aliases/alias1.yaml") diff --git a/st2common/tests/unit/test_util_pack.py b/st2common/tests/unit/test_util_pack.py index 0b476b7336..275fdf72c0 100644 --- a/st2common/tests/unit/test_util_pack.py +++ b/st2common/tests/unit/test_util_pack.py @@ -19,6 +19,7 @@ from st2common.models.db.pack import PackDB from st2common.util.pack import get_pack_common_libs_path_for_pack_db from st2common.util.pack import get_pack_warnings +from st2common.util.pack import get_pack_ref_from_metadata class PackUtilsTestCase(unittest2.TestCase): @@ -66,3 +67,21 @@ def test_get_pack_warnings_no_python(self): pack_metadata = {"name": "PackNone"} warning = get_pack_warnings(pack_metadata) self.assertEqual(None, warning) + + def test_get_pack_ref_from_meta_name_valid(self): + pack_metadata = {"name": "pack1"} + pack_ref = get_pack_ref_from_metadata(pack_metadata) + self.assertEqual("pack1", pack_ref) + + def test_get_pack_ref_from_meta_ref_valid(self): + pack_metadata = {"name": "Pack1", "ref": "pack1"} + pack_ref = get_pack_ref_from_metadata(pack_metadata) + self.assertEqual("pack1", pack_ref) + + def test_get_pack_ref_from_meta_ref_global(self): + pack_metadata = {"name": "Pack1", "ref": "global"} + self.assertRaises(ValueError, get_pack_ref_from_metadata, pack_metadata) + + def test_get_pack_ref_from_meta_name_global(self): + pack_metadata = {"name": "global"} + self.assertRaises(ValueError, get_pack_ref_from_metadata, pack_metadata) diff --git a/st2common/tests/unit/test_virtualenvs.py b/st2common/tests/unit/test_virtualenvs.py index 439801f67a..90c159f2dc 100644 --- a/st2common/tests/unit/test_virtualenvs.py +++ b/st2common/tests/unit/test_virtualenvs.py @@ -378,3 +378,17 @@ def assertVirtualenvExists(self, virtualenv_dir): self.assertTrue(os.path.isdir(os.path.join(virtualenv_dir, "bin/"))) return True + + def test_setup_virtualenv_reserved_packname(self): + # Test a virtualenv update with pack which has global name + pack_name = "global" + pack_virtualenv_dir = os.path.join(self.virtualenvs_path, pack_name) + + self.assertRaises( + ValueError, + setup_pack_virtualenv, + pack_name=pack_name, + update=False, + include_setuptools=False, + include_wheel=False, + ) From 03671cf3f1861c857a44f7d769a2ee86f089f3aa Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Fri, 4 Mar 2022 16:16:17 +0000 Subject: [PATCH 17/18] Fix flake8 UT error --- st2common/tests/unit/test_virtualenvs.py | 1 - 1 file changed, 1 deletion(-) diff --git a/st2common/tests/unit/test_virtualenvs.py b/st2common/tests/unit/test_virtualenvs.py index 90c159f2dc..c93261a615 100644 --- a/st2common/tests/unit/test_virtualenvs.py +++ b/st2common/tests/unit/test_virtualenvs.py @@ -382,7 +382,6 @@ def assertVirtualenvExists(self, virtualenv_dir): def test_setup_virtualenv_reserved_packname(self): # Test a virtualenv update with pack which has global name pack_name = "global" - pack_virtualenv_dir = os.path.join(self.virtualenvs_path, pack_name) self.assertRaises( ValueError, From 45c6bc9c213031999c1045eb9121deed7ea4e5bf Mon Sep 17 00:00:00 2001 From: Amanda McGuinness Date: Mon, 7 Mar 2022 13:28:54 +0000 Subject: [PATCH 18/18] Rename overrides global to _global --- st2common/st2common/constants/pack.py | 2 +- st2common/st2common/content/loader.py | 2 +- .../tests/resources/overrides/{global.yaml => _global.yaml} | 0 st2common/tests/unit/test_util_pack.py | 4 ++-- st2common/tests/unit/test_virtualenvs.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) rename st2common/tests/resources/overrides/{global.yaml => _global.yaml} (100%) diff --git a/st2common/st2common/constants/pack.py b/st2common/st2common/constants/pack.py index 99259e75fc..c0a68abc07 100644 --- a/st2common/st2common/constants/pack.py +++ b/st2common/st2common/constants/pack.py @@ -39,7 +39,7 @@ PACK_REF_WHITELIST_REGEX = r"^[a-z0-9_]+$" # A list of reserved pack names that cannot be used -RESERVED_PACK_LIST = ["global"] +RESERVED_PACK_LIST = ["_global"] # Check for a valid semver string PACK_VERSION_REGEX = r"^(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)\.(?:0|[1-9]\d*)(?:-[\da-z\-]+(?:\.[\da-z\-]+)*)?(?:\+[\da-z\-]+(?:\.[\da-z\-]+)*)?$" # noqa diff --git a/st2common/st2common/content/loader.py b/st2common/st2common/content/loader.py index 50bfe27486..7e57ef6ec0 100644 --- a/st2common/st2common/content/loader.py +++ b/st2common/st2common/content/loader.py @@ -312,7 +312,7 @@ def override(self, pack_name, resource_type, content): override_dir = os.path.join(cfg.CONF.system.base_path, "overrides") # Apply global overrides - global_file = os.path.join(override_dir, "global.yaml") + global_file = os.path.join(override_dir, "_global.yaml") self._apply_override_file(global_file, pack_name, resource_type, content, True) # Apply pack overrides diff --git a/st2common/tests/resources/overrides/global.yaml b/st2common/tests/resources/overrides/_global.yaml similarity index 100% rename from st2common/tests/resources/overrides/global.yaml rename to st2common/tests/resources/overrides/_global.yaml diff --git a/st2common/tests/unit/test_util_pack.py b/st2common/tests/unit/test_util_pack.py index 275fdf72c0..8e9dd59884 100644 --- a/st2common/tests/unit/test_util_pack.py +++ b/st2common/tests/unit/test_util_pack.py @@ -79,9 +79,9 @@ def test_get_pack_ref_from_meta_ref_valid(self): self.assertEqual("pack1", pack_ref) def test_get_pack_ref_from_meta_ref_global(self): - pack_metadata = {"name": "Pack1", "ref": "global"} + pack_metadata = {"name": "Pack1", "ref": "_global"} self.assertRaises(ValueError, get_pack_ref_from_metadata, pack_metadata) def test_get_pack_ref_from_meta_name_global(self): - pack_metadata = {"name": "global"} + pack_metadata = {"name": "_global"} self.assertRaises(ValueError, get_pack_ref_from_metadata, pack_metadata) diff --git a/st2common/tests/unit/test_virtualenvs.py b/st2common/tests/unit/test_virtualenvs.py index c93261a615..502f1df83c 100644 --- a/st2common/tests/unit/test_virtualenvs.py +++ b/st2common/tests/unit/test_virtualenvs.py @@ -381,7 +381,7 @@ def assertVirtualenvExists(self, virtualenv_dir): def test_setup_virtualenv_reserved_packname(self): # Test a virtualenv update with pack which has global name - pack_name = "global" + pack_name = "_global" self.assertRaises( ValueError,