Merge branch 'tickets/DM-33665' into main

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/
+)

src/admin/python/lsst/qserv/admin/cli/entrypoint.py

@@ -624,16 +624,17 @@ 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(
     "--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,
 )

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

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.",

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()