From 5340d580ce0dd434cd4aabfbbf3c47b452ac28de Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sun, 13 Jul 2025 14:20:22 +0200 Subject: [PATCH] Optimized filter functions --- filter_plugins/csp_filters.py | 2 +- filter_plugins/get_app_conf.py | 14 +++--- .../test_no_applications_variable_usage.py | 47 +++++++++++++++++++ .../test_configuration_filters.py | 7 +-- tests/unit/filter_plugins/test_csp_filters.py | 6 +-- .../unit/filter_plugins/test_get_app_conf.py | 11 +++-- .../unit/lookup_plugins/test_docker_cards.py | 4 +- 7 files changed, 69 insertions(+), 22 deletions(-) create mode 100644 tests/integration/test_no_applications_variable_usage.py diff --git a/filter_plugins/csp_filters.py b/filter_plugins/csp_filters.py index ae6dc214..8b3d4869 100644 --- a/filter_plugins/csp_filters.py +++ b/filter_plugins/csp_filters.py @@ -124,7 +124,7 @@ class FilterModule(object): # Enable loading via ancestors if ( - self.is_feature_enabled(applications, 'portfolio_iframe', application_id) + self.is_feature_enabled(applications, 'port-ui-desktop', application_id) and directive == 'frame-ancestors' ): domain = domains.get('web-app-port-ui')[0] diff --git a/filter_plugins/get_app_conf.py b/filter_plugins/get_app_conf.py index 7735510c..9b259da9 100644 --- a/filter_plugins/get_app_conf.py +++ b/filter_plugins/get_app_conf.py @@ -72,14 +72,12 @@ def get_app_conf(applications, application_id, config_path, strict=True): try: obj = applications[application_id] except KeyError: - if strict: - raise AppConfigKeyError( - f"Application ID '{application_id}' not found in applications dict.\n" - f"path_trace: {path_trace}\n" - f"applications keys: {list(applications.keys())}\n" - f"config_path: {config_path}" - ) - return False + raise AppConfigKeyError( + f"Application ID '{application_id}' not found in applications dict.\n" + f"path_trace: {path_trace}\n" + f"applications keys: {list(applications.keys())}\n" + f"config_path: {config_path}" + ) for part in config_path.split("."): path_trace.append(part) diff --git a/tests/integration/test_no_applications_variable_usage.py b/tests/integration/test_no_applications_variable_usage.py new file mode 100644 index 00000000..2ac9ee40 --- /dev/null +++ b/tests/integration/test_no_applications_variable_usage.py @@ -0,0 +1,47 @@ +# tests/integration/test_no_applications_variable_usage.py + +import os +import re +import unittest + +class TestNoApplicationsVariableUsage(unittest.TestCase): + """ + This test ensures that the pattern `applications[some_variable]` is not used anywhere + under the roles/ directory. Instead, the usage of get_app_conf should be preferred. + """ + + APPLICATIONS_VARIABLE_PATTERN = re.compile(r"applications\[\s*[a-zA-Z_][a-zA-Z0-9_]*\s*\]") + + def test_no_applications_variable_usage(self): + roles_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', 'roles')) + found = [] + + for root, dirs, files in os.walk(roles_dir): + # Skip __pycache__ folders + dirs[:] = [d for d in dirs if d != '__pycache__'] + for file in files: + if file.endswith('.pyc'): + continue + file_path = os.path.join(root, file) + # Skip this test file itself (so it can contain the pattern in docstrings) + if os.path.abspath(file_path) == os.path.abspath(__file__): + continue + try: + with open(file_path, 'r', encoding='utf-8') as f: + for lineno, line in enumerate(f, 1): + match = self.APPLICATIONS_VARIABLE_PATTERN.search(line) + if match: + found.append(f"{file_path}:{lineno}: {line.strip()}") + except Exception: + # Binary or unreadable file, skip + continue + + if found: + self.fail( + "Found illegal usages of 'applications[variable]' in the following locations:\n" + + "\n".join(found) + + "\n\nPlease use get_app_conf instead." + ) + +if __name__ == '__main__': + unittest.main() diff --git a/tests/unit/filter_plugins/test_configuration_filters.py b/tests/unit/filter_plugins/test_configuration_filters.py index f813285c..e7c90001 100644 --- a/tests/unit/filter_plugins/test_configuration_filters.py +++ b/tests/unit/filter_plugins/test_configuration_filters.py @@ -1,5 +1,3 @@ -# tests/unit/test_configuration_filters.py - import unittest import sys import os @@ -15,6 +13,8 @@ from filter_plugins.configuration_filters import ( is_feature_enabled, ) +from filter_plugins.get_app_conf import AppConfigKeyError + class TestConfigurationFilters(unittest.TestCase): def setUp(self): @@ -38,7 +38,8 @@ class TestConfigurationFilters(unittest.TestCase): self.assertFalse(is_feature_enabled(self.applications, 'nonexistent', 'app1')) def test_is_feature_enabled_false_missing_app(self): - self.assertFalse(is_feature_enabled(self.applications, 'oauth2', 'unknown_app')) + with self.assertRaises(Exception): + is_feature_enabled(self.applications, 'oauth2', 'unknown_app') if __name__ == '__main__': unittest.main() \ No newline at end of file diff --git a/tests/unit/filter_plugins/test_csp_filters.py b/tests/unit/filter_plugins/test_csp_filters.py index da34d6d8..84b43ba6 100644 --- a/tests/unit/filter_plugins/test_csp_filters.py +++ b/tests/unit/filter_plugins/test_csp_filters.py @@ -174,10 +174,10 @@ class TestCspFilters(unittest.TestCase): def test_build_csp_header_frame_ancestors(self): """ frame-ancestors should include the wildcarded SLD+TLD when - 'portfolio_iframe' is enabled, and omit it when disabled. + 'port-ui-desktop' is enabled, and omit it when disabled. """ # Ensure feature enabled and domain set - self.apps['app1']['features']['portfolio_iframe'] = True + self.apps['app1']['features']['port-ui-desktop'] = True # simulate a subdomain for the application self.domains['web-app-port-ui'] = ['domain-example.com'] @@ -189,7 +189,7 @@ class TestCspFilters(unittest.TestCase): ) # Now disable the feature and rebuild - self.apps['app1']['features']['portfolio_iframe'] = False + self.apps['app1']['features']['port-ui-desktop'] = False header_no = self.filter.build_csp_header(self.apps, 'app1', self.domains, web_protocol='https') # Should no longer contain the wildcarded sld.tld self.assertNotIn("*.domain-example.com", header_no) diff --git a/tests/unit/filter_plugins/test_get_app_conf.py b/tests/unit/filter_plugins/test_get_app_conf.py index 3536d3c8..ca45aeaa 100644 --- a/tests/unit/filter_plugins/test_get_app_conf.py +++ b/tests/unit/filter_plugins/test_get_app_conf.py @@ -4,7 +4,7 @@ import os sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '../../../filter_plugins'))) -from get_app_conf import get_app_conf +from get_app_conf import get_app_conf, AppConfigKeyError from ansible.errors import AnsibleFilterError class TestGetAppConf(unittest.TestCase): @@ -99,11 +99,12 @@ class TestGetAppConf(unittest.TestCase): get_app_conf(apps, "app", "foo[1].bar", strict=True) def test_application_id_not_found(self): - """Test with unknown application_id: should raise error in strict mode.""" - with self.assertRaises(AnsibleFilterError): + """Test with unknown application_id: should always raise error now.""" + with self.assertRaises(AppConfigKeyError): get_app_conf(self.applications, "unknown", "features.foo", strict=True) - # Non-strict: returns False - self.assertFalse(get_app_conf(self.applications, "unknown", "features.foo", strict=False)) + with self.assertRaises(AppConfigKeyError): + get_app_conf(self.applications, "unknown", "features.foo", strict=False) + if __name__ == '__main__': unittest.main() diff --git a/tests/unit/lookup_plugins/test_docker_cards.py b/tests/unit/lookup_plugins/test_docker_cards.py index cc51c88b..b641b809 100644 --- a/tests/unit/lookup_plugins/test_docker_cards.py +++ b/tests/unit/lookup_plugins/test_docker_cards.py @@ -57,7 +57,7 @@ galaxy_info: "applications": { "portfolio": { "features": { - "portfolio_iframe": True + "port-ui-desktop": True } } }, @@ -90,7 +90,7 @@ galaxy_info: "applications": { "portfolio": { "features": { - "portfolio_iframe": True + "port-ui-desktop": True } } },