diff --git a/coco/client.py b/coco/client.py index 855087f..2214d7c 100644 --- a/coco/client.py +++ b/coco/client.py @@ -23,10 +23,10 @@ `CliGroup.init_coco` to fetch the coco config from the daemon, using the "--backend" and/or "--conf" options previously found (or envars) to determine the host/port of the daemon. Once the config is returned, - this function creates `EndpointCommand` instances for each endpoint - defined, and adds them all to the `CliGroup` instance. If an error - occurs trying to contact the daemon, the error is recorded for later, - but program flow continues. + this function creates `EndpointCommand` or `EndpointGroup` instances + for each endpoint/endpoint tree deefined, and adds them all to the + `CliGroup` instance. If an error occurs trying to contact the daemon, + the error is recorded for later, but program flow continues. 7. After creating all the endpoint commands, control passes to the normal `click.Group.list_commands` or `click.Group.get_command` and program flow now follows in the normal `click` way. @@ -51,6 +51,42 @@ from .endpoint import VALUE_TYPE +class EndpointGroup(click.Group): + """A click Group for an endpoint tree. + + NB: This is not used for the top-level of the endpoint tree. See the + CliGroup instead. + """ + + def __init__(self, tree): + self.tree = tree + name = tree["name"] + + # Figure out short help. + if "summary" in tree: + # If a there's a "summary" use that. + short_help = tree["summary"] + elif "description" in tree: + # If there's a description, use everything up to the first full stop. + short_help = tree["description"] + try: + short_help = short_help[: short_help.index(". ")] + except ValueError: + # description is at most once sentence. Use it all. + pass + + help_ = tree.get("description", tree.get("summary", "NO DESCRIPTION")) + + # Generate all the subcommands / subgroups + commands = [ + EndpointGroup(endpoint) if endpoint["tree"] else EndpointCommand(endpoint) + for endpoint in tree["endpoints"] + ] + + # click initialisation + super().__init__(name, short_help=short_help, help=help_, commands=commands) + + class EndpointCommand(click.Command): """A click Command for a single endpoint.""" @@ -76,11 +112,10 @@ def __init__(self, endpoint): params, param_help = self._endpoint_params() - help_ = ( - endpoint.get("description", endpoint.get("summary", "NO DESCRIPTION")) - + "\n\n\b\n" - + param_help - ) + help_ = endpoint.get("description", endpoint.get("summary", "NO DESCRIPTION")) + + if param_help: + help_ += "\n\n\b\n" + param_help # click initialisation super().__init__( @@ -314,7 +349,11 @@ def __init__(self, *args, **kwargs): def _init_endpoints(self, endpoints): """Update the click command list with endpoint definitions.""" for endpoint in endpoints: - self.add_command(EndpointCommand(endpoint)) + self.add_command( + EndpointGroup(endpoint) + if endpoint["tree"] + else EndpointCommand(endpoint) + ) def format_commands(self, ctx, formatter): """List commands and endpoints. @@ -330,7 +369,7 @@ def format_commands(self, ctx, formatter): if cmd is None or cmd.hidden: continue - if isinstance(cmd, EndpointCommand): + if isinstance(cmd, (EndpointCommand, EndpointGroup)): endpoints.append((subcommand, cmd)) else: commands.append((subcommand, cmd)) diff --git a/coco/config.py b/coco/config.py index c50a772..d994820 100644 --- a/coco/config.py +++ b/coco/config.py @@ -83,11 +83,9 @@ # These specify the logger path, the minimum level it applies to and the # slack channel the messages should go to. slack_rules: - - logger: coco level: WARNING channel: coco-alerts - - logger: coco.endpoint.update-pulsar-pointing-0 level: INFO channel: pulsar-timing-ops @@ -223,11 +221,16 @@ def load_config( if not any_exist: raise click.ClickException("No configuration files available.") + # Validate config _validate_and_resolve(config) # Local endpoints are not loaded in the CLI if not cli: - _load_endpoint_config(config) + # Load the endpoints + endpoint_tree = load_endpoint_tree(Path(config["endpoint_dir"])) + + # We only record the endpoint list per se, not the whole top-level group + config["endpoints"] = endpoint_tree["endpoints"] return config @@ -301,38 +304,83 @@ def _validate_and_resolve(config: dict) -> None: ) -def _load_endpoint_config(config: dict) -> None: - """Load the endpoint config. +def load_endpoint(path: Path) -> dict | None: + """Read an endpoint from `path`. - The config is injected into the passed in config object. + Returns the parsed endpoint config entry, or None if `path` wasn't an + endpoint file """ - config["endpoints"] = [] + # A file called "__meta.conf" (two underscores) can be used to set metadata + # about the containing endpoint group. + meta = path.name == "__meta.conf" - endpoint_dir = Path(config["endpoint_dir"]) + # Only accept files ending in .conf as endpoint configs. + # Endpoint config files starting with an underscore (_) are disabled (except + # a __meta.conf file). + if not meta and (path.suffix != ".conf" or path.name.startswith("_")): + # Not a valid endpoint file + logger.debug(f"Ignoring invalid/disabled endpoint {path}.") + return None - for endpoint_file in endpoint_dir.iterdir(): - # Only accept files ending in .conf as endpoint configs. - # Endpoint config files starting with an underscore (_) are disabled. - if endpoint_file.suffix == ".conf" and not endpoint_file.name.startswith("_"): - logger.debug(f"Loading endpoint config {endpoint_file}.") + logger.debug(f"Loading endpoint config {path}.") - # Remove .conf from the config file name to get the name of the endpoint - name = endpoint_file.stem + try: + with path.open("r", encoding="utf-8") as fh: + conf = yaml_load(fh) + except (OSError, ValueError, UnicodeDecodeError, yaml.YAMLError) as e: + raise click.ClickException(f"Failure reading endpoint {path}: {e}") from e - try: - with endpoint_file.open("r", encoding="utf-8") as fh: - conf = yaml_load(fh) - except (OSError, ValueError, UnicodeDecodeError, yaml.YAMLError) as e: - raise click.ClickException( - f"Failure reading endpoint {endpoint_file}: {e}" - ) from e + # Extra endpoint metadata + conf["meta"] = meta + conf["tree"] = False - # A "name" field in an endpoint file is not allowed - if "name" in conf: - raise click.ClickException( - f"spurious 'name' found in endpoint {name!r}" - ) - conf["name"] = name + # A "name" field in an endpoint file is not allowed + if "name" in conf: + raise click.ClickException(f"spurious 'name' found in endpoint {path}") - # Endpoint config will be validated after the state is loaded - config["endpoints"].append(conf) + # Remove .conf from the config file name to get the name of the endpoint + conf["name"] = path.stem + + return conf + + +def load_endpoint_tree(path): + """Iterate over a directory `path` in the endpoint tree. + + Returns an endpoint tree config for the directory, + containing the endpoints and subtrees found within. + """ + tree = { + "name": path.name, + "tree": True, + "description": "Unremarkable endpoint tree", + "endpoints": [], + } + for dir_entry in path.iterdir(): + if dir_entry.is_dir(): + # Parse the subtree + subtree = load_endpoint_tree(dir_entry) + + # The subtree is only added if it's not empty + if subtree["endpoints"]: + tree["endpoints"].append(subtree) + else: + endpoint = load_endpoint(dir_entry) + + # Skip if nothing was loaded + if not endpoint: + continue + + if endpoint["meta"]: + # If this was a __meta.conf file, merge (select) data into the + # tree config + for key in {"description", "summary"}: + if key in endpoint: + tree[key] = endpoint[key] + else: + # otherwise, delete the "meta" key and append endpoint + # to the tree's list + del endpoint["meta"] + tree["endpoints"].append(endpoint) + + return tree diff --git a/coco/core.py b/coco/core.py index ab430c3..aa7a9a3 100644 --- a/coco/core.py +++ b/coco/core.py @@ -107,11 +107,44 @@ def __init__( # Now that the state is loaded, validate all the endpoints defined by the config endpoints = [] groups = self.config["groups"] - all_endpoints = {endpoint["name"] for endpoint in self.config["endpoints"]} + + # dict of all endpoint upaths + upaths = {} + + def _list_all_endpoints(parent: str, endpoints: list) -> set: + """Generate a set of all endpoint names. + + Also ensures no two endpoints end up with the + same upath. + """ + all_ = set() + + if parent: + parent += "/" + + for endpoint in endpoints: + path = parent + endpoint["name"] + if endpoint["tree"]: # Descend through subtree + all_ |= _list_all_endpoints(path, endpoint["endpoints"]) + else: + # Having two endpoints called, say, "alpha_beta/gamma" and + # "alpha/beta_gamma" is not allowed, to keep the redis keys + # sensible. + upath = path.replace("/", "_") + if upath in upaths: + raise click.ClickException( + f"Endpoint clash: endpoints {path!r} and " + f"{upaths[upath]!r} can't both be defined." + ) + upaths[upath] = path + all_.add(path) + return all_ + + all_endpoints = _list_all_endpoints("", self.config["endpoints"]) for endpoint in self.config["endpoints"]: endpoints.append( - validate_endpoint(endpoint, groups, all_endpoints, self.state) + validate_endpoint(endpoint, "", groups, all_endpoints, self.state) ) self.config["endpoints"] = endpoints @@ -133,7 +166,8 @@ def __init__( self._config_slack_loggers() - self._load_endpoints() + self.endpoints = {} + self._load_endpoints("", self.config["endpoints"]) self._local_endpoints() self._check_endpoint_links() @@ -428,18 +462,26 @@ def _load_config(self, config_path: os.PathLike | None, testing: bool): f"Required key {key} missing from slack rule: {rdict}." ) - def _load_endpoints(self): - self.endpoints = {} + def _load_endpoints(self, parent, endpoints): + if parent: + parent += "/" - for conf in self.config["endpoints"]: + for conf in endpoints: name = conf["name"] - # Create the endpoint object - self.endpoints[name] = Endpoint(name, conf, self.forwarder, self.state) + # Descend through endpoint subtrees + if conf["tree"]: + self._load_endpoints(parent + name, conf["endpoints"]) + continue + + # Otherwise, create the endpoint object + self.endpoints[name] = Endpoint( + name, parent, conf, self.forwarder, self.state + ) if not self.endpoints[name].has_external_forwards: logger.debug( - f"Endpoint {name} has `call` set to 'null'. This means it " + f"Endpoint {parent}{name} has `call` set to 'null'. This means it " f"doesn't call external endpoints. It might check other coco " f"endpoints or return some part of coco's state." ) diff --git a/coco/endpoint.py b/coco/endpoint.py index 4396091..deb928b 100644 --- a/coco/endpoint.py +++ b/coco/endpoint.py @@ -46,9 +46,10 @@ class Endpoint: Does whatever the config says. """ - def __init__(self, name, conf, forwarder, state): + def __init__(self, name, parent, conf, forwarder, state): logger.debug(f"Loading {name}.conf") self.name = name + self.parent = parent if conf is None: conf = {} self.description = conf["description"] @@ -89,6 +90,19 @@ def __init__(self, name, conf, forwarder, state): self._load_internal_forward(conf.get("after"), self.after) self.timestamp_path = conf.get("timestamp") + @property + def path(self): + """Full name, including parent, if any.""" + + if self.parent: + return self.parent + "/" + self.name + return self.name + + @property + def upath(self): + """The underscorified path of this endpoint.""" + return self.path.replace("/", "_") + def _load_internal_forward(self, dict_, list_): """ Load Forwards from the config dictionary, generate objects and place in list. @@ -661,7 +675,9 @@ def _validate_bool(config: dict, param: str, default: bool, location: str) -> bo return config[param] -def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> dict: +def validate_endpoint( + config: dict, parent: str, groups: dict, all_endpoints: set, state +) -> dict: """Vet and rationalise endpoint config. If the endpoint can't be rationalized, leaving it invalid, @@ -671,6 +687,8 @@ def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> ---------- config: The endpoint config read from coco's config + parent: + The path in the endpoint tree for this endpoint groups: The cocod group config all_endpoints: @@ -685,11 +703,35 @@ def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> """ from .result import TYPES as RESULT_TYPES + if parent: + parent += "/" + + # If this is an endpoint subtree, iterate through it. + if config["tree"]: + endpoint_tree = { + "name": config["name"], + "tree": True, + "description": config["description"], + } + + if "summary" in endpoint_tree: + endpoint_tree["summary"] = config["summary"] + + endpoint_tree["endpoints"] = [ + validate_endpoint( + endpoint, parent + config["name"], groups, all_endpoints, state + ) + for endpoint in config["endpoints"] + ] + return endpoint_tree + + # If we got here, we have a normal endpoint. + # Used in error messages - location = f"endpoint {config['name']!r}" + location = f"endpoint {parent}{config['name']!r}" # The fixed-up endpoint config will end up here - endpoint = {"name": config["name"]} + endpoint = {"name": config["name"], "tree": False} # this is all the allowed endpoint parameters ENDPOINT_PARAMETERS = ( @@ -709,6 +751,7 @@ def validate_endpoint(config: dict, groups: dict, all_endpoints: set, state) -> "send_state", "set_state", "timestamp", + "tree", "type", "values", ) diff --git a/tests/coco_runner.py b/tests/coco_runner.py index cb9bed5..23c2614 100644 --- a/tests/coco_runner.py +++ b/tests/coco_runner.py @@ -27,6 +27,7 @@ def testN(coco_runner): import json import multiprocessing +import os import socket import threading from time import sleep @@ -226,8 +227,15 @@ def add_endpoint(self, name, endpoint_def): if self._daemon_proc: raise RuntimeError("called after start_daemon") + # Ensure directory exists + path = self.endpoint_dir / f"{name}.conf" + try: + os.mkdir(path.parent, mode=0o0700) + except FileExistsError: + pass + # Dump to endpoint file - with open(self.endpoint_dir / f"{name}.conf", "w") as f: + with open(path, "w") as f: yaml.dump(endpoint_def, f) def set_state(self, state): diff --git a/tests/test_client.py b/tests/test_client.py index 4c1c762..280a395 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -18,6 +18,7 @@ def test_help_no_endpoints(coco_runner): def test_help(coco_runner): + """Test --help output""" coco_runner.add_targets("cluster", 1) coco_runner.add_endpoint( "nop", @@ -26,12 +27,21 @@ def test_help(coco_runner): "call": {"forward": None}, }, ) + coco_runner.add_endpoint( + "subpoint/__meta", + {"description": "Endpoint group help", "summary": "Group summary"}, + ) + coco_runner.add_endpoint( + "subpoint/cmd", {"description": "sub-endpoint", "call": {"forward": None}} + ) # These should all complete successfully coco_runner.client("--help") coco_runner.client("blocklist", "--help") coco_runner.client("blocklist", "add", "--help") coco_runner.client("nop", "--help") + coco_runner.client("subpoint", "--help") + coco_runner.client("subpoint", "cmd", "--help") def test_style(coco_runner): diff --git a/tests/test_config.py b/tests/test_config.py index 2223de2..0724554 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -96,7 +96,8 @@ def test_default_config(coco_config): # But also the default endpoint expected_result = config.merge_dict_tree( - expected_result, {"endpoints": [{"group": "defgroup", "name": "endpoint"}]} + expected_result, + {"endpoints": [{"group": "defgroup", "name": "endpoint", "tree": False}]}, ) assert result == expected_result @@ -178,7 +179,7 @@ def test_yaml_endpoint_error(fs, coco_config): config.load_config() -def test_no_map_endpoing(fs, coco_config): +def test_no_map_endpoint(fs, coco_config): """Test reading a non-mapping YAML endpoint""" fs.create_file("/etc/coco/endpoints/test.conf", contents="---\njust_a_scalar\n") diff --git a/tests/test_endpoint.py b/tests/test_endpoint.py index 706c3d0..67f2b48 100644 --- a/tests/test_endpoint.py +++ b/tests/test_endpoint.py @@ -593,6 +593,9 @@ def test_endpoint_rewrite(coco_runner): name = endpoint["name"] del endpoint["name"] + # Delete the coco-added tree key + del endpoint["tree"] + # Should be one of the endpoints we expected assert name in endpoints