From 130daaab8047304f8272f9f04741af5d38ac0f6e Mon Sep 17 00:00:00 2001 From: Nathan Pease Date: Fri, 11 Feb 2022 11:19:38 -0600 Subject: [PATCH 1/4] add --no option to entrypoint & spawned flags --- src/admin/python/lsst/qserv/admin/qservCli/qserv_cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/admin/python/lsst/qserv/admin/qservCli/qserv_cli.py b/src/admin/python/lsst/qserv/admin/qservCli/qserv_cli.py index 1972f02a32..c73b2fcd74 100644 --- a/src/admin/python/lsst/qserv/admin/qservCli/qserv_cli.py +++ b/src/admin/python/lsst/qserv/admin/qservCli/qserv_cli.py @@ -609,13 +609,13 @@ def down( @click.argument("COMMAND", required=False) @qserv_image_option() @click.option( - "--entrypoint", + "--entrypoint/--no-entrypoint", is_flag=True, default=True, help="Show help output for the entrypoint command.", ) @click.option( - "--spawned", + "--spawned/--no-spawned", is_flag=True, default=True, help="Show help output for the spawned app.", From 004dd879283e048391776da8fd06a4a0952896ec Mon Sep 17 00:00:00 2001 From: Nathan Pease Date: Fri, 11 Feb 2022 19:28:35 -0600 Subject: [PATCH 2/4] fix help message --- src/admin/python/lsst/qserv/admin/cli/entrypoint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/admin/python/lsst/qserv/admin/cli/entrypoint.py b/src/admin/python/lsst/qserv/admin/cli/entrypoint.py index 1e31a0ed0f..0a0246ee8d 100644 --- a/src/admin/python/lsst/qserv/admin/cli/entrypoint.py +++ b/src/admin/python/lsst/qserv/admin/cli/entrypoint.py @@ -624,7 +624,7 @@ def worker_repl(ctx: click.Context, **kwargs: Any) -> None: required=True, ) @db_admin_uri_option( - help="The admin URI to the proxy's database, used for schema initialization. " + socket_option_help, + help="The admin URI to the replication controller's database, used for schema initialization. " + socket_option_help, required=True, ) @click.option( From 3fc5489a5fe344addc4cd09f28cfbdbedd530894 Mon Sep 17 00:00:00 2001 From: Nathan Pease Date: Fri, 11 Feb 2022 19:28:55 -0600 Subject: [PATCH 3/4] add details to help message --- src/admin/python/lsst/qserv/admin/cli/entrypoint.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/admin/python/lsst/qserv/admin/cli/entrypoint.py b/src/admin/python/lsst/qserv/admin/cli/entrypoint.py index 0a0246ee8d..32c0f69285 100644 --- a/src/admin/python/lsst/qserv/admin/cli/entrypoint.py +++ b/src/admin/python/lsst/qserv/admin/cli/entrypoint.py @@ -630,10 +630,11 @@ def worker_repl(ctx: click.Context, **kwargs: Any) -> None: @click.option( "--worker", "workers", - help=( - "The settings for each worker in the system. " - "The value must be in the form 'key1=val1,key2=val,...'" - f"\ntarg key name is {click.style('workers', bold=True)}" + help=("""The settings for each worker in the system. +The value must be in the form 'key1=val1,key2=val2,...' +These are used when initializing a fresh qserv to configure the replication controller. They become options passed to +qserv-replica-config, like 'qserv-replica-config ADD_WORKER --key1=val1 --key2=val2, ...'. +If using targs, name is plural; '{click.style('workers', bold=True)}'.""" ), multiple=True, ) From def14e93c8df183ccc55d031696d6a54c42cc180 Mon Sep 17 00:00:00 2001 From: Nathan Pease Date: Fri, 11 Feb 2022 19:39:33 -0600 Subject: [PATCH 4/4] fail if targs contains a self-reference --- src/admin/CMakeLists.txt | 5 +++ .../lsst/qserv/admin/cli/render_targs.py | 41 +++++++++++++++--- src/admin/tests/test_render_targs.py | 42 ++++++++++++++++++- 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/src/admin/CMakeLists.txt b/src/admin/CMakeLists.txt index b68423e491..47376a61f9 100644 --- a/src/admin/CMakeLists.txt +++ b/src/admin/CMakeLists.txt @@ -43,3 +43,8 @@ add_test(NAME test_qserv_log COMMAND python3 -m unittest tests.admin.test_qserv_log WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/python/ ) + +add_test(NAME test_render_targs + COMMAND python3 -m unittest tests.admin.test_render_targs + WORKING_DIRECTORY ${CMAKE_INSTALL_PREFIX}/python/ +) diff --git a/src/admin/python/lsst/qserv/admin/cli/render_targs.py b/src/admin/python/lsst/qserv/admin/cli/render_targs.py index f1d710f79c..290e85b077 100644 --- a/src/admin/python/lsst/qserv/admin/cli/render_targs.py +++ b/src/admin/python/lsst/qserv/admin/cli/render_targs.py @@ -25,6 +25,7 @@ from copy import copy import jinja2 +from typing import List from .utils import Targs @@ -35,6 +36,30 @@ class UnresolvableTemplate(RuntimeError): pass +def _format_targs(targs: Targs) -> str: + """Format targs for printing to an error message.""" + return ", ".join([f"{k}={v}" for k, v in targs.items()]) + +def _get_vars(val: str) -> List[str]: + """Get variable names from a value that contains template variables. + + Parameters + ---------- + val : str + The value from a targs entry. + + Returns + ------- + vars : list [ `str` ] + The variables (value inside braces) inside val. + """ + # Jinja variable names must be surrounded by double braces and may be + # surrounded by whitespace inside the braces (`{{foo}}` or `{{ foo }}`). + # Python variable names may not contain braces. So, find all the leading + # braces, and use the rest of the string up to the closing braces. + return [i[:i.find("}}")].strip() for i in val.split("{{") if "}}" in i] + + def render_targs(targs: Targs) -> Targs: """Go through a dict whose values may contain jinja templates that are other keys in the dict, and resolve the values. @@ -65,17 +90,21 @@ def render_targs(targs: Targs) -> Targs: if not isinstance(v, str): continue if "{{" in v: + if k in _get_vars(v): + raise UnresolvableTemplate( + "Template value may not refer to its own key, directly or as a circualr reference:" + + _format_targs(targs) + ) t = jinja2.Template(v, undefined=jinja2.StrictUndefined) try: - rendered[k] = t.render(rendered) - changed = True + r = t.render(rendered) + if r != rendered[k]: + rendered[k] = r + changed = True except jinja2.exceptions.UndefinedError as e: raise UnresolvableTemplate(f"Missing template value: {str(e)}") if not changed: break if any([isinstance(v, str) and "{{" in v for v in rendered.values()]): - raise UnresolvableTemplate( - "Could not resolve inputs: " - f"{', '.join([f'{k}={targs[k]}' for k, v in rendered.items() if '{{' in v])}" - ) + raise UnresolvableTemplate(f"Could not resolve inputs {targs}, they became: {rendered}") return rendered diff --git a/src/admin/tests/test_render_targs.py b/src/admin/tests/test_render_targs.py index b6b5117e87..3906f1ba85 100644 --- a/src/admin/tests/test_render_targs.py +++ b/src/admin/tests/test_render_targs.py @@ -19,9 +19,29 @@ # You should have received a copy of the GNU General Public License +import jinja2 import unittest -from lsst.qserv.admin.cli.render_targs import render_targs, UnresolvableTemplate +from lsst.qserv.admin.cli.render_targs import ( + _get_vars, + render_targs, + UnresolvableTemplate, +) + + +class GetVarsTestCase(unittest.TestCase): + + def test(self): + self.assertEqual(_get_vars("{{foo}}"), ["foo"]) + self.assertEqual(_get_vars("abc {{foo}}"), ["foo"]) + self.assertEqual(_get_vars("{{foo}} abc"), ["foo"]) + self.assertEqual(_get_vars("{{foo}} {{bar}} abc"), ["foo", "bar"]) + self.assertEqual(_get_vars("{{foo}} abc {{bar}}"), ["foo", "bar"]) + self.assertEqual(_get_vars("{{ foo }} abc {{ bar}} {{baz }}"), ["foo", "bar", "baz"]) + # "foo bar" is not a legal var name and will fail to resolve to anything + # in the jira template, but it should work fine when creating the "vars" + # that are used in the template string. + self.assertEqual(_get_vars("{{foo bar}} abc"), ["foo bar"]) class RenderTargsTestCase(unittest.TestCase): @@ -38,6 +58,17 @@ def testCircularReference(self): render_targs({"a": "{{b}}", "b": "{{c}}", "c": "{{a}}"}) self.assertIn("a={{b}}, b={{c}}, c={{a}}", str(r.exception)) + def testCircularReference_varWithText(self): + """Test for failure when there is a circular reference in targs.""" + with self.assertRaises(UnresolvableTemplate) as r: + render_targs({"a": "{{b}}", "b": "foo {{c}}", "c": "{{a}}"}) + self.assertIn("a={{b}}, b=foo {{c}}, c={{a}}", str(r.exception)) + + def testCirularReference_twoVarsWithText(self): + with self.assertRaises(UnresolvableTemplate) as r: + render_targs({"a": "the {{b}}", "b": "only {{a}}"}) + self.assertIn("a=the {{b}}, b=only {{a}}", str(r.exception)) + def testSelfReference(self): """Test for failure when a targ refers to itself.""" with self.assertRaises(UnresolvableTemplate) as r: @@ -60,7 +91,7 @@ def testList(self): def testResolves(self): """Verify that a dict with legal values resolves correctly.""" self.assertEqual( - render_targs({"a": "{{b}}", "b": "{{c}}", "c": "d"}), + render_targs({"a": "{{ b }}", "b": "{{c}}", "c": "d"}), {"a": "d", "b": "d", "c": "d"} ) @@ -78,6 +109,13 @@ def testBool(self): {"a": True} ) + def testVarWithSpace(self): + """Verify spaces are not allowed inside of jinja template variables. + + (and we don't expect it).""" + with self.assertRaises(jinja2.exceptions.TemplateSyntaxError): + render_targs({"abc def": "foo", "ghi jkl": "{{abc def}}"}) + if __name__ == "__main__": unittest.main()