From 92df362f1db40c8af9cadffc4c58e21d8a04779a Mon Sep 17 00:00:00 2001
From: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Date: Thu, 24 Aug 2023 13:05:44 +0100
Subject: [PATCH] Allow top level  settings in CLI Config (#221)

allow top level sections in cli config

* Closes #217

---------

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
---
 cylc/rose/utilities.py         | 87 ++++++++++++++++++++++++++--------
 tests/unit/test_config_tree.py | 18 -------
 2 files changed, 66 insertions(+), 39 deletions(-)

diff --git a/cylc/rose/utilities.py b/cylc/rose/utilities.py
index ed90511d..da367b47 100644
--- a/cylc/rose/utilities.py
+++ b/cylc/rose/utilities.py
@@ -21,7 +21,7 @@
 from pathlib import Path
 import re
 import shlex
-from typing import TYPE_CHECKING, Union
+from typing import TYPE_CHECKING, Any, List, Tuple, Union
 
 from cylc.flow.hostuserutil import get_host
 from cylc.flow import LOG
@@ -327,6 +327,67 @@ def merge_rose_cylc_suite_install_conf(old, new):
     return old
 
 
+def parse_cli_defines(define: str) -> Union[
+    bool, Tuple[
+        List[Union[str, Any]],
+        Union[str, Any],
+        Union[str, Any]
+    ]
+]:
+    """Parse a define string.
+
+    Args:
+        define:
+            A string in one of two forms:
+            - `key = "value"`
+            - `[section]key = "value"`
+
+            With optional `!` and `!!` prepended, indicating an ignored state,
+            which should lead to a warning being logged.
+
+    Returns:
+        False: If state is ignored or trigger-ignored, otherwise...
+        (keys, value, state)
+
+    Examples:
+        # Top level key
+        >>> parse_cli_defines('root-dir = "foo"')
+        (['root-dir'], '"foo"', '')
+
+        # Marked as ignored
+        >>> parse_cli_defines('!root-dir = "foo"')
+        False
+
+        # Inside a section
+        >>> parse_cli_defines('[section]orange = "segment"')
+        (['section', 'orange'], '"segment"', '')
+    """
+    match = re.match(
+        (
+            r'^\[(?P<section>.*)\](?P<state>!{0,2})'
+            r'(?P<key>.*)\s*=\s*(?P<value>.*)'
+        ),
+        define
+    )
+    if match:
+        groupdict = match.groupdict()
+        keys = [groupdict['section'].strip(), groupdict['key'].strip()]
+    else:
+        # Doesn't have a section:
+        match = re.match(
+            r'^(?P<state>!{0,2})(?P<key>.*)\s*=\s*(?P<value>.*)', define)
+        if match and not match['state']:
+            groupdict = match.groupdict()
+            keys = [groupdict['key'].strip()]
+        else:
+            # This seems like it ought to be an error,
+            # But behaviour is consistent with Rose 2019
+            # See: https://github.com/cylc/cylc-rose/issues/217
+            return False
+
+    return (keys, match['value'], match['state'])
+
+
 def get_cli_opts_node(opts=None, srcdir=None):
     """Create a ConfigNode representing options set on the command line.
 
@@ -367,28 +428,12 @@ def get_cli_opts_node(opts=None, srcdir=None):
     defines.append(f'[env]ROSE_ORIG_HOST={rose_orig_host}')
     rose_template_vars.append(f'ROSE_ORIG_HOST={rose_orig_host}')
 
-    # Construct new ouput based on optional Configs:
+    # Construct new config node representing CLI config items:
     newconfig = ConfigNode()
-
-    # For each __define__ determine whether it is an env or template define.
     for define in defines:
-        match = re.match(
-            (
-                r'^\[(?P<key1>.*)\](?P<state>!{0,2})'
-                r'(?P<key2>.*)\s*=\s*(?P<value>.*)'
-            ),
-            define
-        ).groupdict()
-        if match['key1'] == '' and match['state'] in ['!', '!!']:
-            LOG.warning(
-                'CLI opts set to ignored or trigger-ignored will be ignored.'
-            )
-        else:
-            newconfig.set(
-                keys=[match['key1'], match['key2']],
-                value=match['value'],
-                state=match['state']
-            )
+        parsed_define = parse_cli_defines(define)
+        if parsed_define:
+            newconfig.set(*parsed_define)
 
     # For each __suite define__ add define.
     if srcdir is not None:
diff --git a/tests/unit/test_config_tree.py b/tests/unit/test_config_tree.py
index c93ddc2d..3718785b 100644
--- a/tests/unit/test_config_tree.py
+++ b/tests/unit/test_config_tree.py
@@ -467,24 +467,6 @@ def test_merge_opts(
     assert merge_opts(conf, opt_conf_keys) == expected
 
 
-@pytest.mark.parametrize(
-    'state',
-    ['!', '!!']
-)
-def test_cli_defines_ignored_are_ignored(
-    state, caplog
-):
-    opts = SimpleNamespace(
-        opt_confs='', defines=[f'[]{state}opts=ignore me'],
-        rose_template_vars=[]
-    )
-
-    get_cli_opts_node(opts)
-    assert (caplog.records[0].message ==
-            'CLI opts set to ignored or trigger-ignored will be ignored.'
-            )
-
-
 @pytest.mark.parametrize(
     'opt_confs, defines, rose_template_vars, expect',
     [