From 818448b8e3b9c7bc69fa6879b74dfaa64e98bc6d Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 26 Oct 2023 14:26:03 +0100 Subject: [PATCH 01/36] WIP: Begin sketching context command --- data_safe_haven/commands/context.py | 101 ++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 data_safe_haven/commands/context.py diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py new file mode 100644 index 0000000000..b64bb8d69c --- /dev/null +++ b/data_safe_haven/commands/context.py @@ -0,0 +1,101 @@ +"""Command group and entrypoints for managing a DSH context""" +from typing import Annotated, Optional + +import typer + +from data_safe_haven.backend import Backend +from data_safe_haven.config import BackendSettings +from data_safe_haven.exceptions import ( + DataSafeHavenError, + DataSafeHavenInputError, +) +from data_safe_haven.functions import validate_aad_guid + +context_command_group = typer.Typer() + + +@context_command_group.command() +def add( + admin_group: Annotated[ + Optional[str], # noqa: UP007 + typer.Option( + "--admin-group", + "-a", + help="The ID of an Azure group containing all administrators.", + callback=validate_aad_guid, + ), + ] = None, + location: Annotated[ + Optional[str], # noqa: UP007 + typer.Option( + "--location", + "-l", + help="The Azure location to deploy resources into.", + ), + ] = None, + name: Annotated[ + Optional[str], # noqa: UP007 + typer.Option( + "--name", + "-n", + help="The name to give this Data Safe Haven deployment.", + ), + ] = None, + subscription: Annotated[ + Optional[str], # noqa: UP007 + typer.Option( + "--subscription", + "-s", + help="The name of an Azure subscription to deploy resources into.", + ), + ] = None, +) -> None: + settings = BackendSettings() + settings.add( + admin_group_id=admin_group, + location=location, + name=name, + subscription_name=subscription, + ) + + +@context_command_group.command() +def remove() -> None: + pass + + +@context_command_group.command() +def show() -> None: + settings = BackendSettings() + settings.summarise() + + +@context_command_group.command() +def switch( + name: Annotated[str, typer.Argument(help="Name of the context to switch to.")] +) -> None: + settings = BackendSettings() + settings.switch(name) + + +@context_command_group.command() +def create() -> None: + backend = Backend() # How does this get the config!?! + backend.create() + + backend.config.upload() # What does this do? + + +@context_command_group.command() +def teardown() -> None: + """Tear down a Data Safe Haven context""" + try: + try: + backend = Backend() + backend.teardown() + except Exception as exc: + msg = f"Unable to teardown Pulumi backend.\n{exc}" + raise DataSafeHavenInputError(msg) from exc # Input error? No input. + except DataSafeHavenError as exc: + msg = f"Could not teardown Data Safe Haven backend.\n{exc}" + raise DataSafeHavenError(msg) from exc From 5817e6592ba36343522807ad5fed0f9120582c8a Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 26 Oct 2023 15:59:06 +0100 Subject: [PATCH 02/36] WIP: Adjust context settings class --- data_safe_haven/config/backend_settings.py | 132 +++++++++------------ 1 file changed, 57 insertions(+), 75 deletions(-) diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index 61816839cf..5085fd9318 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -1,4 +1,5 @@ """Load global and local settings from dotfiles""" +from dataclasses import dataclass import pathlib import appdirs @@ -12,34 +13,72 @@ from data_safe_haven.utility import LoggingSingleton -class BackendSettings: +@dataclass +class Context(): + admin_group_id: str + location: str + name: str + subscription_name: str + + +class ContextSettings: """Load global and local settings from dotfiles with structure like the following - azure: - admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd - location: uksouth - subscription_name: Data Safe Haven (Acme) - current: - name: Acme Deployment + selected: acme_deployment + contexts: + acme_deployment: + name: Acme Deployment + azure: + admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd + location: uksouth + subscription_name: Data Safe Haven (Acme) + ... """ - def __init__( - self, - ) -> None: - # Define instance variables - self._admin_group_id: str | None = None - self._location: str | None = None - self._name: str | None = None - self._subscription_name: str | None = None + def __init__(self) -> None: self.logger = LoggingSingleton() - # Load previous backend settings (if any) - self.config_directory = pathlib.Path( + self._selected: str | None = None + self._context: Context | None = None + + config_directory = pathlib.Path( appdirs.user_config_dir(appname="data_safe_haven") ).resolve() - self.config_file_path = self.config_directory / "config.yaml" + self.config_file_path = config_directory / "config.yaml" self.read() + def read(self) -> None: + """Read settings from YAML file""" + try: + with open(self.config_file_path, encoding="utf-8") as f_yaml: + settings = yaml.safe_load(f_yaml) + if isinstance(settings, dict): + self.logger.info( + f"Reading project settings from '[green]{self.config_file_path}[/]'." + ) + self._selected = settings.get("selected") + self._context = Context(**settings.get("contexts").get(self._selected)) + except FileNotFoundError as exc: + msg = f"Could not find file {self.config_file_path}.\n{exc}" + raise DataSafeHavenConfigError(msg) from exc + except ParserError as exc: + msg = f"Could not load settings from {self.config_file_path}.\n{exc}" + raise DataSafeHavenConfigError(msg) from exc + + @property + def selected(self) -> str: + if not self._selected: + msg = f"Selected context is not defined in {self.config_file_path}." + raise DataSafeHavenParameterError(msg) + return self._selected + + @property + def context(self) -> Context: + if not self._context: + msg = f"Context {self._selected} is not defined in {self.config_file_path}." + raise DataSafeHavenParameterError(msg) + return self._context + def update( self, *, @@ -69,63 +108,6 @@ def update( # Write backend settings to disk (this will trigger errors for uninitialised parameters) self.write() - @property - def admin_group_id(self) -> str: - if not self._admin_group_id: - msg = "Azure administrator group not provided: use '[bright_cyan]--admin-group[/]' / '[green]-a[/]' to do so." - raise DataSafeHavenParameterError(msg) - return self._admin_group_id - - @property - def location(self) -> str: - if not self._location: - msg = "Azure location not provided: use '[bright_cyan]--location[/]' / '[green]-l[/]' to do so." - raise DataSafeHavenParameterError(msg) - return self._location - - @property - def name(self) -> str: - if not self._name: - msg = ( - "Data Safe Haven deployment name not provided:" - " use '[bright_cyan]--name[/]' / '[green]-n[/]' to do so." - ) - raise DataSafeHavenParameterError(msg) - return self._name - - @property - def subscription_name(self) -> str: - if not self._subscription_name: - msg = "Azure subscription not provided: use '[bright_cyan]--subscription[/]' / '[green]-s[/]' to do so." - raise DataSafeHavenParameterError(msg) - return self._subscription_name - - def read(self) -> None: - """Read settings from YAML file""" - try: - if self.config_file_path.exists(): - with open(self.config_file_path, encoding="utf-8") as f_yaml: - settings = yaml.safe_load(f_yaml) - if isinstance(settings, dict): - self.logger.info( - f"Reading project settings from '[green]{self.config_file_path}[/]'." - ) - if admin_group_id := settings.get("azure", {}).get( - "admin_group_id", None - ): - self._admin_group_id = admin_group_id - if location := settings.get("azure", {}).get("location", None): - self._location = location - if name := settings.get("current", {}).get("name", None): - self._name = name - if subscription_name := settings.get("azure", {}).get( - "subscription_name", None - ): - self._subscription_name = subscription_name - except ParserError as exc: - msg = f"Could not load settings from {self.config_file_path}.\n{exc}" - raise DataSafeHavenConfigError(msg) from exc - def write(self) -> None: """Write settings to YAML file""" settings = { From 5e6726dd07c254c995ce29f820e039f807bd852a Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 27 Oct 2023 13:48:03 +0100 Subject: [PATCH 03/36] WIP: Add/Update methods --- data_safe_haven/commands/context.py | 2 +- data_safe_haven/config/backend_settings.py | 105 +++++++++++---------- 2 files changed, 54 insertions(+), 53 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index b64bb8d69c..55aa778435 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -75,7 +75,7 @@ def switch( name: Annotated[str, typer.Argument(help="Name of the context to switch to.")] ) -> None: settings = BackendSettings() - settings.switch(name) + settings.context = name @context_command_group.command() diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index 5085fd9318..ac0858eead 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -1,6 +1,7 @@ """Load global and local settings from dotfiles""" -from dataclasses import dataclass import pathlib +from dataclasses import dataclass +from typing import Any import appdirs import yaml @@ -14,7 +15,7 @@ @dataclass -class Context(): +class Context: admin_group_id: str location: str name: str @@ -28,56 +29,67 @@ class ContextSettings: contexts: acme_deployment: name: Acme Deployment - azure: - admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd - location: uksouth - subscription_name: Data Safe Haven (Acme) + admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd + location: uksouth + subscription_name: Data Safe Haven (Acme) ... """ def __init__(self) -> None: self.logger = LoggingSingleton() - self._selected: str | None = None - self._context: Context | None = None + self._settings: dict[Any, Any] | None = None config_directory = pathlib.Path( appdirs.user_config_dir(appname="data_safe_haven") ).resolve() self.config_file_path = config_directory / "config.yaml" - self.read() - - def read(self) -> None: - """Read settings from YAML file""" - try: - with open(self.config_file_path, encoding="utf-8") as f_yaml: - settings = yaml.safe_load(f_yaml) - if isinstance(settings, dict): - self.logger.info( - f"Reading project settings from '[green]{self.config_file_path}[/]'." - ) - self._selected = settings.get("selected") - self._context = Context(**settings.get("contexts").get(self._selected)) - except FileNotFoundError as exc: - msg = f"Could not find file {self.config_file_path}.\n{exc}" - raise DataSafeHavenConfigError(msg) from exc - except ParserError as exc: - msg = f"Could not load settings from {self.config_file_path}.\n{exc}" - raise DataSafeHavenConfigError(msg) from exc + + @property + def settings(self) -> dict[Any, Any]: + if not self._settings: + try: + with open(self.config_file_path, encoding="utf-8") as f_yaml: + settings = yaml.safe_load(f_yaml) + if isinstance(settings, dict): + self.logger.info( + f"Reading project settings from '[green]{self.config_file_path}[/]'." + ) + self._settings = settings + self._context = Context(**settings.get("contexts").get(self._selected)) + except FileNotFoundError as exc: + msg = f"Could not find file {self.config_file_path}.\n{exc}" + raise DataSafeHavenConfigError(msg) from exc + except ParserError as exc: + msg = f"Could not load settings from {self.config_file_path}.\n{exc}" + raise DataSafeHavenConfigError(msg) from exc + + return self._settings @property def selected(self) -> str: - if not self._selected: + if selected := self.settings.get("selected"): + return selected + else: msg = f"Selected context is not defined in {self.config_file_path}." raise DataSafeHavenParameterError(msg) - return self._selected + + @selected.setter + def selected(self, context_name: str) -> None: + if context_name in self.settings["contexts"].keys(): + self.settings["selected"] = context_name + self.logger.info(f"Switched context to {context_name}.") + else: + msg = f"Context {context_name} is not defined in {self.config_file_path}." + raise DataSafeHavenParameterError(msg) @property def context(self) -> Context: - if not self._context: - msg = f"Context {self._selected} is not defined in {self.config_file_path}." + if context_dict := self.settings.get("contexts").get(self.selected): + return Context(**context_dict) + else: + msg = f"Context {self.selected} is not defined in {self.config_file_path}." raise DataSafeHavenParameterError(msg) - return self._context def update( self, @@ -87,43 +99,32 @@ def update( name: str | None = None, subscription_name: str | None = None, ) -> None: - """Overwrite defaults with provided parameters""" + context_dict = self.settings.get("contexts").get(self.selected) + if admin_group_id: self.logger.debug( f"Updating '[green]{admin_group_id}[/]' to '{admin_group_id}'." ) - self._admin_group_id = admin_group_id + context_dict["admin_group_id"] = admin_group_id if location: self.logger.debug(f"Updating '[green]{location}[/]' to '{location}'.") - self._location = location + context_dict["location"] = location if name: self.logger.debug(f"Updating '[green]{name}[/]' to '{name}'.") - self._name = name + context_dict["name"] = name if subscription_name: self.logger.debug( f"Updating '[green]{subscription_name}[/]' to '{subscription_name}'." ) - self._subscription_name = subscription_name - - # Write backend settings to disk (this will trigger errors for uninitialised parameters) - self.write() + context_dict["subscription_name"] = subscription_name def write(self) -> None: """Write settings to YAML file""" - settings = { - "azure": { - "admin_group_id": self.admin_group_id, - "location": self.location, - "subscription_name": self.subscription_name, - }, - "current": { - "name": self.name, - }, - } # Create the parent directory if it does not exist then write YAML self.config_file_path.parent.mkdir(parents=True, exist_ok=True) + with open(self.config_file_path, "w", encoding="utf-8") as f_yaml: - yaml.dump(settings, f_yaml, indent=2) + yaml.dump(self.settings, f_yaml, indent=2) self.logger.info( - f"Saved project settings to '[green]{self.config_file_path}[/]'." + f"Saved context settings to '[green]{self.config_file_path}[/]'." ) From 3c2ad438c028e3e0639aac1757e7a9c6672e62ab Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 27 Oct 2023 14:29:33 +0100 Subject: [PATCH 04/36] WIP: Add schema and from_file classmethod --- data_safe_haven/config/backend_settings.py | 83 ++++++++++++++-------- pyproject.toml | 1 + 2 files changed, 53 insertions(+), 31 deletions(-) diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index ac0858eead..3e7257b051 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -5,6 +5,7 @@ import appdirs import yaml +from schema import Schema, SchemaError from yaml.parser import ParserError from data_safe_haven.exceptions import ( @@ -14,6 +15,12 @@ from data_safe_haven.utility import LoggingSingleton +config_directory = pathlib.Path( + appdirs.user_config_dir(appname="data_safe_haven") +).resolve() +config_file_path = config_directory / "config.yaml" + + @dataclass class Context: admin_group_id: str @@ -35,35 +42,31 @@ class ContextSettings: ... """ - def __init__(self) -> None: + def __init__(self, settings_dict: dict[Any, Any]) -> None: self.logger = LoggingSingleton() - self._settings: dict[Any, Any] | None = None - - config_directory = pathlib.Path( - appdirs.user_config_dir(appname="data_safe_haven") - ).resolve() - self.config_file_path = config_directory / "config.yaml" + context_schema = Schema({ + "name": str, + "admin_group_id": str, + "location": str, + "subscription_name": str, + }) + + schema = Schema({ + "selected": str, + "contexts": Schema({ + str: context_schema, + }), + }) + + try: + self._settings: schema.validate(settings_dict) + except SchemaError as exc: + msg = f"Invalid context configuration file.\n{exc}" + raise DataSafeHavenParameterError(msg) @property def settings(self) -> dict[Any, Any]: - if not self._settings: - try: - with open(self.config_file_path, encoding="utf-8") as f_yaml: - settings = yaml.safe_load(f_yaml) - if isinstance(settings, dict): - self.logger.info( - f"Reading project settings from '[green]{self.config_file_path}[/]'." - ) - self._settings = settings - self._context = Context(**settings.get("contexts").get(self._selected)) - except FileNotFoundError as exc: - msg = f"Could not find file {self.config_file_path}.\n{exc}" - raise DataSafeHavenConfigError(msg) from exc - except ParserError as exc: - msg = f"Could not load settings from {self.config_file_path}.\n{exc}" - raise DataSafeHavenConfigError(msg) from exc - return self._settings @property @@ -71,7 +74,7 @@ def selected(self) -> str: if selected := self.settings.get("selected"): return selected else: - msg = f"Selected context is not defined in {self.config_file_path}." + msg = "Selected context is not defined." raise DataSafeHavenParameterError(msg) @selected.setter @@ -80,7 +83,7 @@ def selected(self, context_name: str) -> None: self.settings["selected"] = context_name self.logger.info(f"Switched context to {context_name}.") else: - msg = f"Context {context_name} is not defined in {self.config_file_path}." + msg = f"Context {context_name} is not defined." raise DataSafeHavenParameterError(msg) @property @@ -88,7 +91,7 @@ def context(self) -> Context: if context_dict := self.settings.get("contexts").get(self.selected): return Context(**context_dict) else: - msg = f"Context {self.selected} is not defined in {self.config_file_path}." + msg = f"Context {self.selected} is not defined." raise DataSafeHavenParameterError(msg) def update( @@ -118,13 +121,31 @@ def update( ) context_dict["subscription_name"] = subscription_name - def write(self) -> None: + def write(self, config_file_path: str = config_file_path) -> None: """Write settings to YAML file""" # Create the parent directory if it does not exist then write YAML - self.config_file_path.parent.mkdir(parents=True, exist_ok=True) + config_file_path.parent.mkdir(parents=True, exist_ok=True) - with open(self.config_file_path, "w", encoding="utf-8") as f_yaml: + with open(config_file_path, "w", encoding="utf-8") as f_yaml: yaml.dump(self.settings, f_yaml, indent=2) self.logger.info( - f"Saved context settings to '[green]{self.config_file_path}[/]'." + f"Saved context settings to '[green]{config_file_path}[/]'." ) + + @classmethod + def from_file(cls, config_file_path: str = config_file_path) -> None: + logger = LoggingSingleton() + try: + with open(config_file_path, encoding="utf-8") as f_yaml: + settings = yaml.safe_load(f_yaml) + if isinstance(settings, dict): + logger.info( + f"Reading project settings from '[green]{config_file_path}[/]'." + ) + return cls(settings) + except FileNotFoundError as exc: + msg = f"Could not find file {config_file_path}.\n{exc}" + raise DataSafeHavenConfigError(msg) from exc + except ParserError as exc: + msg = f"Could not load settings from {config_file_path}.\n{exc}" + raise DataSafeHavenConfigError(msg) from exc diff --git a/pyproject.toml b/pyproject.toml index 95afe80918..203b1ff2c5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,6 +44,7 @@ dependencies = [ "pytz~=2022.7.0", "PyYAML~=6.0", "rich~=13.4.2", + "schema~=0.7.0", "simple-acme-dns~=1.2.0", "typer~=0.9.0", "websocket-client~=1.5.0", From 5603417ef1297925a13178ca502cd3cecd3ddf45 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 27 Oct 2023 16:04:23 +0100 Subject: [PATCH 05/36] WIP: Correct constructor --- data_safe_haven/config/backend_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index 3e7257b051..65936007db 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -60,7 +60,7 @@ def __init__(self, settings_dict: dict[Any, Any]) -> None: }) try: - self._settings: schema.validate(settings_dict) + self._settings = schema.validate(settings_dict) except SchemaError as exc: msg = f"Invalid context configuration file.\n{exc}" raise DataSafeHavenParameterError(msg) From 608a3d3656e86b9f95c9fd4f7f16f593c54141fd Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 27 Oct 2023 16:05:06 +0100 Subject: [PATCH 06/36] WIP: Add tests --- pyproject.toml | 13 ++++ tests_/config/test_backend_settings.py | 93 ++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 tests_/config/test_backend_settings.py diff --git a/pyproject.toml b/pyproject.toml index 203b1ff2c5..7afa7beb7c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -86,6 +86,14 @@ all = [ "typing", ] +[tool.hatch.envs.test] +dependencies = [ + "pytest~=7.4.3" +] + +[tool.hatch.envs.test.scripts] +test = "pytest {args:-vvv tests_}" + [tool.black] target-version = ["py310", "py311"] @@ -167,3 +175,8 @@ module = [ "websocket.*", ] ignore_missing_imports = true + +[tool.pytest.ini_options] +addopts = [ + "--import-mode=importlib", +] diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_backend_settings.py new file mode 100644 index 0000000000..08226f37e0 --- /dev/null +++ b/tests_/config/test_backend_settings.py @@ -0,0 +1,93 @@ +from data_safe_haven.config.backend_settings import Context, ContextSettings +from data_safe_haven.exceptions import DataSafeHavenParameterError + +import pytest +import yaml + + +class TestContext: + def test_constructor(self): + context_dict = { + "admin_group_id": "d5c5c439-1115-4cb6-ab50-b8e547b6c8dd", + "location": "uksouth", + "name": "Acme Deployment", + "subscription_name": "Data Safe Haven (Acme)" + } + context = Context(**context_dict) + assert isinstance(context, Context) + assert all([ + getattr(context, item) == context_dict[item] for item in context_dict.keys() + ]) + + +class TestContextSettings: + context_settings = """\ + selected: acme_deployment + contexts: + acme_deployment: + name: Acme Deployment + admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd + location: uksouth + subscription_name: Data Safe Haven (Acme) + gems: + name: Gems + admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd + location: uksouth + subscription_name: Data Safe Haven (Gems)""" + + def test_constructor(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + assert isinstance(settings, ContextSettings) + + def test_invalid(self): + context_settings = "\n".join(self.context_settings.splitlines()[1:]) + + with pytest.raises(DataSafeHavenParameterError) as exc: + ContextSettings(yaml.safe_load(context_settings)) + assert "Missing Key: 'selected'" in exc + + def test_settings(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + assert isinstance(settings.settings, dict) + + def test_selected(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + assert settings.selected == "acme_deployment" + + def test_set_selected(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + assert settings.selected == "acme_deployment" + settings.selected = "gems" + assert settings.selected == "gems" + + def test_context(self): + yaml_settings = yaml.safe_load(self.context_settings) + settings = ContextSettings(yaml_settings) + assert isinstance(settings.context, Context) + assert all([ + getattr(settings.context, item) == yaml_settings["contexts"]["acme_deployment"][item] + for item in yaml_settings["contexts"]["acme_deployment"].keys() + ]) + + def test_set_context(self): + yaml_settings = yaml.safe_load(self.context_settings) + settings = ContextSettings(yaml_settings) + settings.selected = "gems" + assert isinstance(settings.context, Context) + assert all([ + getattr(settings.context, item) == yaml_settings["contexts"]["gems"][item] + for item in yaml_settings["contexts"]["gems"].keys() + ]) + + def test_update(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + assert settings.context.name == "Acme Deployment" + settings.update(name="replaced") + assert settings.context.name == "replaced" + + def test_set_update(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + settings.selected = "gems" + assert settings.context.name == "Gems" + settings.update(name="replaced") + assert settings.context.name == "replaced" From f3ba5a55a284ba2c6729ad2a4017249d5f7c2538 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 27 Oct 2023 16:05:21 +0100 Subject: [PATCH 07/36] Add tests workflow --- .github/workflows/test_code.yaml | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/.github/workflows/test_code.yaml b/.github/workflows/test_code.yaml index a1fa737f67..4ae7b72ec7 100644 --- a/.github/workflows/test_code.yaml +++ b/.github/workflows/test_code.yaml @@ -4,24 +4,24 @@ name: Test code # Run workflow on pushes to matching branches on: # yamllint disable-line rule:truthy push: - branches: [develop] + branches: [develop, python-migration] pull_request: - branches: [develop] + branches: [develop, python-migration] jobs: - test_powershell: + test_python: runs-on: ubuntu-latest steps: - name: Checkout code uses: actions/checkout@v3 - - name: Install requirements - shell: pwsh - run: | - Set-PSRepository PSGallery -InstallationPolicy Trusted - deployment/CheckRequirements.ps1 -InstallMissing -IncludeDev - - name: Test PowerShell - shell: pwsh - run: ./tests/Run_Pester_Tests.ps1 + - name: Set up Python + uses: actions/setup-python@v4 + with: + python-version: 3.11 + - name: Install hatch + run: pip install hatch + - name: Lint Python + run: hatch run test:test test_markdown_links: runs-on: ubuntu-latest From a1feb76d8680bf0a70570353de9bc7a9eb1db99f Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 27 Oct 2023 16:18:01 +0100 Subject: [PATCH 08/36] WIP: Add available method --- data_safe_haven/config/__init__.py | 2 -- data_safe_haven/config/backend_settings.py | 4 ++++ data_safe_haven/config/config.py | 2 -- tests_/config/test_backend_settings.py | 8 ++++++++ 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/data_safe_haven/config/__init__.py b/data_safe_haven/config/__init__.py index d7132460da..f9c53f80d1 100644 --- a/data_safe_haven/config/__init__.py +++ b/data_safe_haven/config/__init__.py @@ -1,7 +1,5 @@ -from .backend_settings import BackendSettings from .config import Config __all__ = [ - "BackendSettings", "Config", ] diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index 65936007db..92f5043258 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -94,6 +94,10 @@ def context(self) -> Context: msg = f"Context {self.selected} is not defined." raise DataSafeHavenParameterError(msg) + @property + def available(self) -> list[str]: + return list(self.settings.get("contexts").keys()) + def update( self, *, diff --git a/data_safe_haven/config/config.py b/data_safe_haven/config/config.py index 1acc30ec90..6fd9c9c99d 100644 --- a/data_safe_haven/config/config.py +++ b/data_safe_haven/config/config.py @@ -40,8 +40,6 @@ SoftwarePackageCategory, ) -from .backend_settings import BackendSettings - class Validator: validation_functions: ClassVar[dict[str, Callable[[Any], Any]]] = {} diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_backend_settings.py index 08226f37e0..9007ad5068 100644 --- a/tests_/config/test_backend_settings.py +++ b/tests_/config/test_backend_settings.py @@ -91,3 +91,11 @@ def test_set_update(self): assert settings.context.name == "Gems" settings.update(name="replaced") assert settings.context.name == "replaced" + + def test_available(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + available = settings.available + print(available) + assert isinstance(available, list) + assert all([isinstance(item, str) for item in available]) + assert available == ["acme_deployment", "gems"] From 11fdc19f5d4116aba3c6685a6f024a09585ca128 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Mon, 30 Oct 2023 10:52:55 +0000 Subject: [PATCH 09/36] WIP: Add tests for from_file and write methods --- data_safe_haven/config/backend_settings.py | 22 +++++++++++----------- tests_/config/test_backend_settings.py | 20 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index 92f5043258..998a95f980 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -125,17 +125,6 @@ def update( ) context_dict["subscription_name"] = subscription_name - def write(self, config_file_path: str = config_file_path) -> None: - """Write settings to YAML file""" - # Create the parent directory if it does not exist then write YAML - config_file_path.parent.mkdir(parents=True, exist_ok=True) - - with open(config_file_path, "w", encoding="utf-8") as f_yaml: - yaml.dump(self.settings, f_yaml, indent=2) - self.logger.info( - f"Saved context settings to '[green]{config_file_path}[/]'." - ) - @classmethod def from_file(cls, config_file_path: str = config_file_path) -> None: logger = LoggingSingleton() @@ -153,3 +142,14 @@ def from_file(cls, config_file_path: str = config_file_path) -> None: except ParserError as exc: msg = f"Could not load settings from {config_file_path}.\n{exc}" raise DataSafeHavenConfigError(msg) from exc + + def write(self, config_file_path: str = config_file_path) -> None: + """Write settings to YAML file""" + # Create the parent directory if it does not exist then write YAML + config_file_path.parent.mkdir(parents=True, exist_ok=True) + + with open(config_file_path, "w", encoding="utf-8") as f_yaml: + yaml.dump(self.settings, f_yaml, indent=2) + self.logger.info( + f"Saved context settings to '[green]{config_file_path}[/]'." + ) diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_backend_settings.py index 9007ad5068..2febf0a61d 100644 --- a/tests_/config/test_backend_settings.py +++ b/tests_/config/test_backend_settings.py @@ -99,3 +99,23 @@ def test_available(self): assert isinstance(available, list) assert all([isinstance(item, str) for item in available]) assert available == ["acme_deployment", "gems"] + + def test_from_file(self, tmp_path): + config_file_path = tmp_path / "config.yaml" + with open(config_file_path, "w") as f: + f.write(self.context_settings) + settings = ContextSettings.from_file(config_file_path=config_file_path) + assert settings.context.name == "Acme Deployment" + + def test_write(self, tmp_path): + config_file_path = tmp_path / "config.yaml" + with open(config_file_path, "w") as f: + f.write(self.context_settings) + settings = ContextSettings.from_file(config_file_path=config_file_path) + settings.selected = "gems" + settings.update(name="replaced") + settings.write(config_file_path) + with open(config_file_path, "r") as f: + context_dict = yaml.safe_load(f) + assert context_dict["selected"] == "gems" + assert context_dict["contexts"]["gems"]["name"] == "replaced" From 5a79eaf4814151dd069587956eb68a99c7b2d1d0 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Mon, 30 Oct 2023 11:10:38 +0000 Subject: [PATCH 10/36] Add add method --- data_safe_haven/config/backend_settings.py | 21 +++++++++++++++++ tests_/config/test_backend_settings.py | 26 +++++++++++++++++----- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index 998a95f980..a4f92105c9 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -125,6 +125,27 @@ def update( ) context_dict["subscription_name"] = subscription_name + def add( + self, + *, + key: str, + name: str, + admin_group_id: str, + location: str, + subscription_name: str + ) -> None: + # Ensure context is not already present + if key in self.settings["contexts"].keys(): + msg = f"A context with key '{key}' is already defined" + raise DataSafeHavenParameterError(msg) + + self.settings["contexts"][key] = { + "name": name, + "admin_group_id": admin_group_id, + "location": location, + "subscription_name": subscription_name, + } + @classmethod def from_file(cls, config_file_path: str = config_file_path) -> None: logger = LoggingSingleton() diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_backend_settings.py index 2febf0a61d..963c070e28 100644 --- a/tests_/config/test_backend_settings.py +++ b/tests_/config/test_backend_settings.py @@ -79,6 +79,14 @@ def test_set_context(self): for item in yaml_settings["contexts"]["gems"].keys() ]) + def test_available(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + available = settings.available + print(available) + assert isinstance(available, list) + assert all([isinstance(item, str) for item in available]) + assert available == ["acme_deployment", "gems"] + def test_update(self): settings = ContextSettings(yaml.safe_load(self.context_settings)) assert settings.context.name == "Acme Deployment" @@ -92,13 +100,19 @@ def test_set_update(self): settings.update(name="replaced") assert settings.context.name == "replaced" - def test_available(self): + def test_add(self): settings = ContextSettings(yaml.safe_load(self.context_settings)) - available = settings.available - print(available) - assert isinstance(available, list) - assert all([isinstance(item, str) for item in available]) - assert available == ["acme_deployment", "gems"] + settings.add( + key="example", + name="Example", + subscription_name="Data Safe Haven (Example)", + admin_group_id="d5c5c439-1115-4cb6-ab50-b8e547b6c8dd", + location="uksouth", + ) + settings.selected = "example" + assert settings.selected == "example" + assert settings.context.name == "Example" + assert settings.context.subscription_name == "Data Safe Haven (Example)" def test_from_file(self, tmp_path): config_file_path = tmp_path / "config.yaml" From f6071f5231392c38410a7619365ebdaee55c60be Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Mon, 30 Oct 2023 11:14:59 +0000 Subject: [PATCH 11/36] WIP: Add test for invalid context selection --- tests_/config/test_backend_settings.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_backend_settings.py index 963c070e28..e358c2922c 100644 --- a/tests_/config/test_backend_settings.py +++ b/tests_/config/test_backend_settings.py @@ -60,6 +60,12 @@ def test_set_selected(self): settings.selected = "gems" assert settings.selected == "gems" + def test_invalid_selected(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + with pytest.raises(DataSafeHavenParameterError) as exc: + settings.selected = "invalid" + assert "Context invalid is not defined." in exc + def test_context(self): yaml_settings = yaml.safe_load(self.context_settings) settings = ContextSettings(yaml_settings) From e12099033f765b45f51b432352ae9e2f49f830a7 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Mon, 30 Oct 2023 11:37:26 +0000 Subject: [PATCH 12/36] WIP: Add invalid add test --- data_safe_haven/config/backend_settings.py | 2 +- tests_/config/test_backend_settings.py | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index a4f92105c9..5488d3f8dc 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -136,7 +136,7 @@ def add( ) -> None: # Ensure context is not already present if key in self.settings["contexts"].keys(): - msg = f"A context with key '{key}' is already defined" + msg = f"A context with key '{key}' is already defined." raise DataSafeHavenParameterError(msg) self.settings["contexts"][key] = { diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_backend_settings.py index e358c2922c..6025a6aa6f 100644 --- a/tests_/config/test_backend_settings.py +++ b/tests_/config/test_backend_settings.py @@ -120,6 +120,18 @@ def test_add(self): assert settings.context.name == "Example" assert settings.context.subscription_name == "Data Safe Haven (Example)" + def test_invalid_add(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + with pytest.raises(DataSafeHavenParameterError) as exc: + settings.add( + key="acme_deployment", + name="Acme Deployment", + subscription_name="Data Safe Haven (Acme)", + admin_group_id="d5c5c439-1115-4cb6-ab50-b8e547b6c8dd", + location="uksouth", + ) + assert "A context with key 'acme' is already defined." in exc + def test_from_file(self, tmp_path): config_file_path = tmp_path / "config.yaml" with open(config_file_path, "w") as f: From 38fd39da7352bbea8658f01d95c9f9dc3d1915ae Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Mon, 30 Oct 2023 11:45:58 +0000 Subject: [PATCH 13/36] Add remove method --- data_safe_haven/config/backend_settings.py | 6 ++++++ tests_/config/test_backend_settings.py | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/backend_settings.py index 5488d3f8dc..a608c87e5a 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/backend_settings.py @@ -146,6 +146,12 @@ def add( "subscription_name": subscription_name, } + def remove(self, key: str) -> None: + if key not in self.settings["contexts"].keys(): + msg = f"No context with key '{key}'." + raise DataSafeHavenParameterError(msg) + del self.settings["contexts"][key] + @classmethod def from_file(cls, config_file_path: str = config_file_path) -> None: logger = LoggingSingleton() diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_backend_settings.py index 6025a6aa6f..3ece077dca 100644 --- a/tests_/config/test_backend_settings.py +++ b/tests_/config/test_backend_settings.py @@ -132,6 +132,17 @@ def test_invalid_add(self): ) assert "A context with key 'acme' is already defined." in exc + def test_remove(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + settings.remove("acme_deployment") + assert "acme_deployment" not in settings.available + + def test_invalid_remove(self): + settings = ContextSettings(yaml.safe_load(self.context_settings)) + with pytest.raises(DataSafeHavenParameterError) as exc: + settings.remove("invalid") + assert "No context with key 'invalid'." in exc + def test_from_file(self, tmp_path): config_file_path = tmp_path / "config.yaml" with open(config_file_path, "w") as f: From 03456b43e4b2871181bfe8230084e9909f8cf26b Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 31 Oct 2023 10:18:56 +0000 Subject: [PATCH 14/36] Add context show command --- data_safe_haven/cli.py | 12 ++-- data_safe_haven/commands/__init__.py | 4 +- data_safe_haven/commands/context.py | 36 +++++----- data_safe_haven/commands/init.py | 67 ------------------- data_safe_haven/config/__init__.py | 2 + ...ackend_settings.py => context_settings.py} | 24 +++++-- tests_/commands/test_context.py | 41 ++++++++++++ ...d_settings.py => test_context_settings.py} | 2 +- 8 files changed, 89 insertions(+), 99 deletions(-) delete mode 100644 data_safe_haven/commands/init.py rename data_safe_haven/config/{backend_settings.py => context_settings.py} (88%) create mode 100644 tests_/commands/test_context.py rename tests_/config/{test_backend_settings.py => test_context_settings.py} (99%) diff --git a/data_safe_haven/cli.py b/data_safe_haven/cli.py index ae2a5bdf20..2ab080e1ca 100644 --- a/data_safe_haven/cli.py +++ b/data_safe_haven/cli.py @@ -7,8 +7,8 @@ from data_safe_haven import __version__ from data_safe_haven.commands import ( admin_command_group, + context_command_group, deploy_command_group, - initialise_command, teardown_command_group, ) from data_safe_haven.exceptions import DataSafeHavenError @@ -69,6 +69,11 @@ def main() -> None: name="admin", help="Perform administrative tasks for a Data Safe Haven deployment.", ) + application.add_typer( + context_command_group, + name="context", + help="Manage Data Safe Haven contexts." + ) application.add_typer( deploy_command_group, name="deploy", @@ -80,11 +85,6 @@ def main() -> None: help="Tear down a Data Safe Haven component.", ) - # Register direct subcommands - application.command(name="init", help="Initialise a Data Safe Haven deployment.")( - initialise_command - ) - # Start the application try: application() diff --git a/data_safe_haven/commands/__init__.py b/data_safe_haven/commands/__init__.py index 5ba59d66de..299c982302 100644 --- a/data_safe_haven/commands/__init__.py +++ b/data_safe_haven/commands/__init__.py @@ -1,11 +1,11 @@ from .admin import admin_command_group +from .context import context_command_group from .deploy import deploy_command_group -from .init import initialise_command from .teardown import teardown_command_group __all__ = [ "admin_command_group", + "context_command_group", "deploy_command_group", - "initialise_command", "teardown_command_group", ] diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 55aa778435..2ef61e900e 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -2,9 +2,10 @@ from typing import Annotated, Optional import typer +from rich import print from data_safe_haven.backend import Backend -from data_safe_haven.config import BackendSettings +from data_safe_haven.config import ContextSettings from data_safe_haven.exceptions import ( DataSafeHavenError, DataSafeHavenInputError, @@ -14,6 +15,23 @@ context_command_group = typer.Typer() +@context_command_group.command() +def show() -> None: + settings = ContextSettings.from_file() + + current_context_key = settings.selected + current_context = settings.context + available = settings.available + + print(f"Current context: [green]{current_context_key}") + print(f"\tName: {current_context.name}") + print(f"\tAdmin Group ID: {current_context.admin_group_id}") + print(f"\tSubscription name: {current_context.subscription_name}") + print(f"\tLocation: {current_context.location}") + print("\nAvailable contexts:") + print("\n".join(available)) + + @context_command_group.command() def add( admin_group: Annotated[ @@ -50,7 +68,7 @@ def add( ), ] = None, ) -> None: - settings = BackendSettings() + settings = ContextSettings() settings.add( admin_group_id=admin_group, location=location, @@ -64,20 +82,6 @@ def remove() -> None: pass -@context_command_group.command() -def show() -> None: - settings = BackendSettings() - settings.summarise() - - -@context_command_group.command() -def switch( - name: Annotated[str, typer.Argument(help="Name of the context to switch to.")] -) -> None: - settings = BackendSettings() - settings.context = name - - @context_command_group.command() def create() -> None: backend = Backend() # How does this get the config!?! diff --git a/data_safe_haven/commands/init.py b/data_safe_haven/commands/init.py deleted file mode 100644 index 916687975f..0000000000 --- a/data_safe_haven/commands/init.py +++ /dev/null @@ -1,67 +0,0 @@ -"""Command-line application for initialising a Data Safe Haven deployment""" -from typing import Annotated, Optional - -import typer - -from data_safe_haven.backend import Backend -from data_safe_haven.config import BackendSettings -from data_safe_haven.exceptions import DataSafeHavenError -from data_safe_haven.functions import validate_aad_guid - - -def initialise_command( - admin_group: Annotated[ - Optional[str], # noqa: UP007 - typer.Option( - "--admin-group", - "-a", - help="The ID of an Azure group containing all administrators.", - callback=validate_aad_guid, - ), - ] = None, - location: Annotated[ - Optional[str], # noqa: UP007 - typer.Option( - "--location", - "-l", - help="The Azure location to deploy resources into.", - ), - ] = None, - name: Annotated[ - Optional[str], # noqa: UP007 - typer.Option( - "--name", - "-n", - help="The name to give this Data Safe Haven deployment.", - ), - ] = None, - subscription: Annotated[ - Optional[str], # noqa: UP007 - typer.Option( - "--subscription", - "-s", - help="The name of an Azure subscription to deploy resources into.", - ), - ] = None, -) -> None: - """Typer command line entrypoint""" - try: - # Load backend settings and update with command line arguments - settings = BackendSettings() - settings.update( - admin_group_id=admin_group, - location=location, - name=name, - subscription_name=subscription, - ) - - # Ensure that the Pulumi backend exists - backend = Backend() - backend.create() - - # Load the generated configuration file and upload it to blob storage - backend.config.upload() - - except DataSafeHavenError as exc: - msg = f"Could not initialise Data Safe Haven.\n{exc}" - raise DataSafeHavenError(msg) from exc diff --git a/data_safe_haven/config/__init__.py b/data_safe_haven/config/__init__.py index f9c53f80d1..4723c0bf08 100644 --- a/data_safe_haven/config/__init__.py +++ b/data_safe_haven/config/__init__.py @@ -1,5 +1,7 @@ from .config import Config +from .context_settings import ContextSettings __all__ = [ "Config", + "ContextSettings", ] diff --git a/data_safe_haven/config/backend_settings.py b/data_safe_haven/config/context_settings.py similarity index 88% rename from data_safe_haven/config/backend_settings.py rename to data_safe_haven/config/context_settings.py index a608c87e5a..05f87cdb02 100644 --- a/data_safe_haven/config/backend_settings.py +++ b/data_safe_haven/config/context_settings.py @@ -1,6 +1,7 @@ """Load global and local settings from dotfiles""" -import pathlib +from pathlib import Path from dataclasses import dataclass +from os import getenv from typing import Any import appdirs @@ -15,10 +16,15 @@ from data_safe_haven.utility import LoggingSingleton -config_directory = pathlib.Path( - appdirs.user_config_dir(appname="data_safe_haven") -).resolve() -config_file_path = config_directory / "config.yaml" +def default_config_file_path() -> Path: + if config_directory_env := getenv("DSH_CONFIG_DIRECTORY"): + config_directory = Path(config_directory_env).resolve() + else: + config_directory = Path( + appdirs.user_config_dir(appname="data_safe_haven") + ).resolve() + + return config_directory / "contexts.yaml" @dataclass @@ -153,7 +159,9 @@ def remove(self, key: str) -> None: del self.settings["contexts"][key] @classmethod - def from_file(cls, config_file_path: str = config_file_path) -> None: + def from_file(cls, config_file_path: str | None = None) -> None: + if config_file_path is None: + config_file_path = default_config_file_path() logger = LoggingSingleton() try: with open(config_file_path, encoding="utf-8") as f_yaml: @@ -170,8 +178,10 @@ def from_file(cls, config_file_path: str = config_file_path) -> None: msg = f"Could not load settings from {config_file_path}.\n{exc}" raise DataSafeHavenConfigError(msg) from exc - def write(self, config_file_path: str = config_file_path) -> None: + def write(self, config_file_path: str | None = None) -> None: """Write settings to YAML file""" + if config_file_path is None: + config_file_path = default_config_file_path() # Create the parent directory if it does not exist then write YAML config_file_path.parent.mkdir(parents=True, exist_ok=True) diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py new file mode 100644 index 0000000000..e6356c17c8 --- /dev/null +++ b/tests_/commands/test_context.py @@ -0,0 +1,41 @@ +from data_safe_haven.commands.context import context_command_group + +from pytest import fixture +from typer.testing import CliRunner + +context_settings = """\ + selected: acme_deployment + contexts: + acme_deployment: + name: Acme Deployment + admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd + location: uksouth + subscription_name: Data Safe Haven (Acme) + gems: + name: Gems + admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd + location: uksouth + subscription_name: Data Safe Haven (Gems)""" + + +@fixture +def tmp_contexts(tmp_path): + config_file_path = tmp_path / "contexts.yaml" + with open(config_file_path, "w") as f: + f.write(context_settings) + return tmp_path + + +@fixture +def runner(tmp_contexts): + runner = CliRunner(env={ + "DSH_CONFIG_DIRECTORY": str(tmp_contexts) + }) + return runner + + +class TestShow: + def test_show(self, runner): + result = runner.invoke(context_command_group, ["show"]) + assert result.exit_code == 0 + assert "Current context: acme_deployment" in result.stdout diff --git a/tests_/config/test_backend_settings.py b/tests_/config/test_context_settings.py similarity index 99% rename from tests_/config/test_backend_settings.py rename to tests_/config/test_context_settings.py index 3ece077dca..09a1dfb338 100644 --- a/tests_/config/test_backend_settings.py +++ b/tests_/config/test_context_settings.py @@ -1,4 +1,4 @@ -from data_safe_haven.config.backend_settings import Context, ContextSettings +from data_safe_haven.config.context_settings import Context, ContextSettings from data_safe_haven.exceptions import DataSafeHavenParameterError import pytest From 3981a83557d9f8da9cd064d84e83554c1b756a7c Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 31 Oct 2023 12:10:42 +0000 Subject: [PATCH 15/36] Add context switch command --- data_safe_haven/commands/context.py | 9 ++++++++ data_safe_haven/config/context_settings.py | 4 ++-- tests_/commands/test_context.py | 26 +++++++++++++++++++--- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 2ef61e900e..1353d32331 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -32,6 +32,15 @@ def show() -> None: print("\n".join(available)) +@context_command_group.command() +def switch( + name: Annotated[str, typer.Argument(help="Name of the context to switch to.")] +) -> None: + settings = ContextSettings.from_file() + settings.selected = name + settings.write() + + @context_command_group.command() def add( admin_group: Annotated[ diff --git a/data_safe_haven/config/context_settings.py b/data_safe_haven/config/context_settings.py index 05f87cdb02..c88836dde3 100644 --- a/data_safe_haven/config/context_settings.py +++ b/data_safe_haven/config/context_settings.py @@ -87,9 +87,9 @@ def selected(self) -> str: def selected(self, context_name: str) -> None: if context_name in self.settings["contexts"].keys(): self.settings["selected"] = context_name - self.logger.info(f"Switched context to {context_name}.") + self.logger.info(f"Switched context to '{context_name}'.") else: - msg = f"Context {context_name} is not defined." + msg = f"Context '{context_name}' is not defined." raise DataSafeHavenParameterError(msg) @property diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index e6356c17c8..9745850deb 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -28,9 +28,13 @@ def tmp_contexts(tmp_path): @fixture def runner(tmp_contexts): - runner = CliRunner(env={ - "DSH_CONFIG_DIRECTORY": str(tmp_contexts) - }) + runner = CliRunner( + env={ + "DSH_CONFIG_DIRECTORY": str(tmp_contexts), + "COLUMNS": "500" # Set large number of columns to avoid rich wrapping text + }, + mix_stderr=False, + ) return runner @@ -39,3 +43,19 @@ def test_show(self, runner): result = runner.invoke(context_command_group, ["show"]) assert result.exit_code == 0 assert "Current context: acme_deployment" in result.stdout + + +class TestSwitch: + def test_switch(self, runner): + result = runner.invoke(context_command_group, ["switch", "gems"]) + assert result.exit_code == 0 + assert "Switched context to 'gems'." in result.stdout + result = runner.invoke(context_command_group, ["show"]) + assert result.exit_code == 0 + assert "Current context: gems" in result.stdout + + def test_invalid_switch(self, runner): + result = runner.invoke(context_command_group, ["switch", "invalid"]) + assert result.exit_code == 1 + # Unable to check error as this is written outside of any Typer + # assert "Context 'invalid' is not defined " in result.stdout From 5f62656b0b8cadedcfccf9c18942cae36d369121 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 31 Oct 2023 14:26:45 +0000 Subject: [PATCH 16/36] Add context add command --- data_safe_haven/commands/context.py | 36 +++++++------- tests_/commands/test_context.py | 75 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 19 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 1353d32331..a68803ac49 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -1,5 +1,5 @@ """Command group and entrypoints for managing a DSH context""" -from typing import Annotated, Optional +from typing import Annotated import typer from rich import print @@ -43,47 +43,45 @@ def switch( @context_command_group.command() def add( + key: Annotated[ + str, + typer.Argument(help="Name of the context to add.") + ], admin_group: Annotated[ - Optional[str], # noqa: UP007 + str, typer.Option( - "--admin-group", - "-a", help="The ID of an Azure group containing all administrators.", callback=validate_aad_guid, ), - ] = None, + ], location: Annotated[ - Optional[str], # noqa: UP007 + str, typer.Option( - "--location", - "-l", help="The Azure location to deploy resources into.", ), - ] = None, + ], name: Annotated[ - Optional[str], # noqa: UP007 + str, typer.Option( - "--name", - "-n", - help="The name to give this Data Safe Haven deployment.", + help="The human friendly name to give this Data Safe Haven deployment.", ), - ] = None, + ], subscription: Annotated[ - Optional[str], # noqa: UP007 + str, typer.Option( - "--subscription", - "-s", help="The name of an Azure subscription to deploy resources into.", ), - ] = None, + ], ) -> None: - settings = ContextSettings() + settings = ContextSettings.from_file() settings.add( + key=key, admin_group_id=admin_group, location=location, name=name, subscription_name=subscription, ) + settings.write() @context_command_group.command() diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index 9745850deb..b6d0b980f8 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -59,3 +59,78 @@ def test_invalid_switch(self, runner): assert result.exit_code == 1 # Unable to check error as this is written outside of any Typer # assert "Context 'invalid' is not defined " in result.stdout + + +class TestAdd: + def test_add(self, runner): + result = runner.invoke( + context_command_group, + [ + "add", + "example", + "--name", + "Example", + "--admin-group", + "d5c5c439-1115-4cb6-ab50-b8e547b6c8dd", + "--location", + "uksouth", + "--subscription", + "Data Safe Haven (Example)", + ] + ) + assert result.exit_code == 0 + result = runner.invoke(context_command_group, ["switch", "example"]) + assert result.exit_code == 0 + + def test_add_duplicate(self, runner): + result = runner.invoke( + context_command_group, + [ + "add", + "acme_deployment", + "--name", + "Acme Deployment", + "--admin-group", + "d5c5c439-1115-4cb6-ab50-b8e547b6c8dd", + "--location", + "uksouth", + "--subscription", + "Data Safe Haven (Acme)", + ] + ) + assert result.exit_code == 1 + # Unable to check error as this is written outside of any Typer + # assert "A context with key 'acme_deployment' is already defined." in result.stdout + + def test_add_invalid_uuid(self, runner): + result = runner.invoke( + context_command_group, + [ + "add", + "example", + "--name", + "Example", + "--admin-group", + "not a uuid", + "--location", + "uksouth", + "--subscription", + "Data Safe Haven (Example)", + ] + ) + assert result.exit_code == 2 + # This works because the context_command_group Typer writes this error + assert "Invalid value for '--admin-group': Expected GUID" in result.stderr + + def test_add_missing_ags(self, runner): + result = runner.invoke( + context_command_group, + [ + "add", + "example", + "--name", + "Example", + ] + ) + assert result.exit_code == 2 + assert "Missing option" in result.stderr From a64a1e241c2f3ceb1f0f97b0395b35f78f33b621 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 31 Oct 2023 14:33:34 +0000 Subject: [PATCH 17/36] Add remove command --- data_safe_haven/commands/context.py | 11 +++++++++-- tests_/commands/test_context.py | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index a68803ac49..47f5c5432a 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -85,8 +85,15 @@ def add( @context_command_group.command() -def remove() -> None: - pass +def remove( + key: Annotated[ + str, + typer.Argument(help="Name of the context to add.") + ], +) -> None: + settings = ContextSettings.from_file() + settings.remove(key) + settings.write() @context_command_group.command() diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index b6d0b980f8..e7a1834a65 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -134,3 +134,18 @@ def test_add_missing_ags(self, runner): ) assert result.exit_code == 2 assert "Missing option" in result.stderr + + +class TestRemove: + def test_remove(self, runner): + result = runner.invoke(context_command_group, ["remove", "gems"]) + assert result.exit_code == 0 + result = runner.invoke(context_command_group, ["show"]) + assert result.exit_code == 0 + assert "gems" not in result.stdout + + def test_remove_invalid(self, runner): + result = runner.invoke(context_command_group, ["remove", "invalid"]) + assert result.exit_code == 1 + # Unable to check error as this is written outside of any Typer + # assert "No context with key 'invalid'." in result.stdout From 8da10c5d8e8dd209bdd64e3f2db13a619e1287b5 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 31 Oct 2023 14:42:37 +0000 Subject: [PATCH 18/36] Add context available command --- data_safe_haven/commands/context.py | 14 ++++++++++++-- tests_/commands/test_context.py | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 47f5c5432a..69c572fa1d 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -21,14 +21,24 @@ def show() -> None: current_context_key = settings.selected current_context = settings.context - available = settings.available print(f"Current context: [green]{current_context_key}") print(f"\tName: {current_context.name}") print(f"\tAdmin Group ID: {current_context.admin_group_id}") print(f"\tSubscription name: {current_context.subscription_name}") print(f"\tLocation: {current_context.location}") - print("\nAvailable contexts:") + + +@context_command_group.command() +def available() -> None: + settings = ContextSettings.from_file() + + current_context_key = settings.selected + available = settings.available + + available.remove(current_context_key) + available = [f"[green]{current_context_key}*[/]"]+available + print("\n".join(available)) diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index e7a1834a65..93533a3a24 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -43,6 +43,15 @@ def test_show(self, runner): result = runner.invoke(context_command_group, ["show"]) assert result.exit_code == 0 assert "Current context: acme_deployment" in result.stdout + assert "Name: Acme Deployment" in result.stdout + + +class TestAvailable: + def test_available(self, runner): + result = runner.invoke(context_command_group, ["available"]) + assert result.exit_code == 0 + assert "acme_deployment*" in result.stdout + assert "gems" in result.stdout class TestSwitch: @@ -50,9 +59,9 @@ def test_switch(self, runner): result = runner.invoke(context_command_group, ["switch", "gems"]) assert result.exit_code == 0 assert "Switched context to 'gems'." in result.stdout - result = runner.invoke(context_command_group, ["show"]) + result = runner.invoke(context_command_group, ["available"]) assert result.exit_code == 0 - assert "Current context: gems" in result.stdout + assert "gems*" in result.stdout def test_invalid_switch(self, runner): result = runner.invoke(context_command_group, ["switch", "invalid"]) @@ -140,7 +149,7 @@ class TestRemove: def test_remove(self, runner): result = runner.invoke(context_command_group, ["remove", "gems"]) assert result.exit_code == 0 - result = runner.invoke(context_command_group, ["show"]) + result = runner.invoke(context_command_group, ["available"]) assert result.exit_code == 0 assert "gems" not in result.stdout From e6d60178878d06251ad0e64249325a5e90240660 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 31 Oct 2023 14:54:08 +0000 Subject: [PATCH 19/36] Add context update command --- data_safe_haven/commands/context.py | 39 +++++++++++++++++++++++++++++ tests_/commands/test_context.py | 9 +++++++ 2 files changed, 48 insertions(+) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 69c572fa1d..d58ada1f55 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -94,6 +94,45 @@ def add( settings.write() +@context_command_group.command() +def update( + admin_group: Annotated[ + str, + typer.Option( + help="The ID of an Azure group containing all administrators.", + callback=validate_aad_guid, + ), + ] = None, + location: Annotated[ + str, + typer.Option( + help="The Azure location to deploy resources into.", + ), + ] = None, + name: Annotated[ + str, + typer.Option( + help="The human friendly name to give this Data Safe Haven deployment.", + ), + ] = None, + subscription: Annotated[ + str, + typer.Option( + help="The name of an Azure subscription to deploy resources into.", + ), + ] = None, +) -> None: + """Update the selected context.""" + settings = ContextSettings.from_file() + settings.update( + admin_group_id=admin_group, + location=location, + name=name, + subscription_name=subscription, + ) + settings.write() + + @context_command_group.command() def remove( key: Annotated[ diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index 93533a3a24..621a4885c6 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -145,6 +145,15 @@ def test_add_missing_ags(self, runner): assert "Missing option" in result.stderr +class TestUpdate: + def test_update(self, runner): + result = runner.invoke(context_command_group, ["update", "--name", "New Name"]) + assert result.exit_code == 0 + result = runner.invoke(context_command_group, ["show"]) + assert result.exit_code == 0 + assert "Name: New Name" in result.stdout + + class TestRemove: def test_remove(self, runner): result = runner.invoke(context_command_group, ["remove", "gems"]) From afd200128e900f22596730fb940a76177e0d1ace Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 31 Oct 2023 15:29:07 +0000 Subject: [PATCH 20/36] Run lint:fmt --- data_safe_haven/cli.py | 4 +-- data_safe_haven/commands/context.py | 22 +++++------- data_safe_haven/config/config.py | 4 ++- data_safe_haven/config/context_settings.py | 40 ++++++++++++---------- pyproject.toml | 1 + 5 files changed, 35 insertions(+), 36 deletions(-) diff --git a/data_safe_haven/cli.py b/data_safe_haven/cli.py index 2ab080e1ca..a48a3e38af 100644 --- a/data_safe_haven/cli.py +++ b/data_safe_haven/cli.py @@ -70,9 +70,7 @@ def main() -> None: help="Perform administrative tasks for a Data Safe Haven deployment.", ) application.add_typer( - context_command_group, - name="context", - help="Manage Data Safe Haven contexts." + context_command_group, name="context", help="Manage Data Safe Haven contexts." ) application.add_typer( deploy_command_group, diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index d58ada1f55..06c4c2e6b8 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -1,5 +1,5 @@ """Command group and entrypoints for managing a DSH context""" -from typing import Annotated +from typing import Annotated, Optional import typer from rich import print @@ -37,7 +37,7 @@ def available() -> None: available = settings.available available.remove(current_context_key) - available = [f"[green]{current_context_key}*[/]"]+available + available = [f"[green]{current_context_key}*[/]", *available] print("\n".join(available)) @@ -53,10 +53,7 @@ def switch( @context_command_group.command() def add( - key: Annotated[ - str, - typer.Argument(help="Name of the context to add.") - ], + key: Annotated[str, typer.Argument(help="Name of the context to add.")], admin_group: Annotated[ str, typer.Option( @@ -97,26 +94,26 @@ def add( @context_command_group.command() def update( admin_group: Annotated[ - str, + Optional[str], # noqa: UP007 typer.Option( help="The ID of an Azure group containing all administrators.", callback=validate_aad_guid, ), ] = None, location: Annotated[ - str, + Optional[str], # noqa: UP007 typer.Option( help="The Azure location to deploy resources into.", ), ] = None, name: Annotated[ - str, + Optional[str], # noqa: UP007 typer.Option( help="The human friendly name to give this Data Safe Haven deployment.", ), ] = None, subscription: Annotated[ - str, + Optional[str], # noqa: UP007 typer.Option( help="The name of an Azure subscription to deploy resources into.", ), @@ -135,10 +132,7 @@ def update( @context_command_group.command() def remove( - key: Annotated[ - str, - typer.Argument(help="Name of the context to add.") - ], + key: Annotated[str, typer.Argument(help="Name of the context to add.")], ) -> None: settings = ContextSettings.from_file() settings.remove(key) diff --git a/data_safe_haven/config/config.py b/data_safe_haven/config/config.py index 6fd9c9c99d..7cd9beb9b0 100644 --- a/data_safe_haven/config/config.py +++ b/data_safe_haven/config/config.py @@ -40,6 +40,8 @@ SoftwarePackageCategory, ) +from .context_settings import ContextSettings + class Validator: validation_functions: ClassVar[dict[str, Callable[[Any], Any]]] = {} @@ -353,7 +355,7 @@ def __init__(self) -> None: self.tags_: ConfigSectionTags | None = None self.sres: dict[str, ConfigSectionSRE] = defaultdict(ConfigSectionSRE) # Read backend settings - settings = BackendSettings() + settings = ContextSettings.from_file() # Check if backend exists and was loaded try: self.name = settings.name diff --git a/data_safe_haven/config/context_settings.py b/data_safe_haven/config/context_settings.py index c88836dde3..c4bdff7983 100644 --- a/data_safe_haven/config/context_settings.py +++ b/data_safe_haven/config/context_settings.py @@ -1,7 +1,7 @@ """Load global and local settings from dotfiles""" -from pathlib import Path from dataclasses import dataclass from os import getenv +from pathlib import Path from typing import Any import appdirs @@ -51,25 +51,31 @@ class ContextSettings: def __init__(self, settings_dict: dict[Any, Any]) -> None: self.logger = LoggingSingleton() - context_schema = Schema({ - "name": str, - "admin_group_id": str, - "location": str, - "subscription_name": str, - }) + context_schema = Schema( + { + "name": str, + "admin_group_id": str, + "location": str, + "subscription_name": str, + } + ) - schema = Schema({ - "selected": str, - "contexts": Schema({ - str: context_schema, - }), - }) + schema = Schema( + { + "selected": str, + "contexts": Schema( + { + str: context_schema, + } + ), + } + ) try: self._settings = schema.validate(settings_dict) except SchemaError as exc: msg = f"Invalid context configuration file.\n{exc}" - raise DataSafeHavenParameterError(msg) + raise DataSafeHavenParameterError(msg) from exc @property def settings(self) -> dict[Any, Any]: @@ -138,7 +144,7 @@ def add( name: str, admin_group_id: str, location: str, - subscription_name: str + subscription_name: str, ) -> None: # Ensure context is not already present if key in self.settings["contexts"].keys(): @@ -187,6 +193,4 @@ def write(self, config_file_path: str | None = None) -> None: with open(config_file_path, "w", encoding="utf-8") as f_yaml: yaml.dump(self.settings, f_yaml, indent=2) - self.logger.info( - f"Saved context settings to '[green]{config_file_path}[/]'." - ) + self.logger.info(f"Saved context settings to '[green]{config_file_path}[/]'.") diff --git a/pyproject.toml b/pyproject.toml index 7afa7beb7c..7bd45ee03e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -179,4 +179,5 @@ ignore_missing_imports = true [tool.pytest.ini_options] addopts = [ "--import-mode=importlib", + "--disable-warnings", ] From eedd004bac85ceca3062aea766b1d92d240303ed Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Wed, 1 Nov 2023 11:28:18 +0000 Subject: [PATCH 21/36] Correct typing --- data_safe_haven/config/config.py | 12 +++--- data_safe_haven/config/context_settings.py | 43 ++++++++----------- .../infrastructure/stack_manager.py | 4 +- data_safe_haven/utility/__init__.py | 2 + data_safe_haven/utility/directories.py | 15 +++++++ typings/schema/__init__.pyi | 10 +++++ 6 files changed, 54 insertions(+), 32 deletions(-) create mode 100644 data_safe_haven/utility/directories.py create mode 100644 typings/schema/__init__.pyi diff --git a/data_safe_haven/config/config.py b/data_safe_haven/config/config.py index 7cd9beb9b0..f7d920a910 100644 --- a/data_safe_haven/config/config.py +++ b/data_safe_haven/config/config.py @@ -38,6 +38,7 @@ DatabaseSystem, LoggingSingleton, SoftwarePackageCategory, + config_dir, ) from .context_settings import ContextSettings @@ -356,15 +357,16 @@ def __init__(self) -> None: self.sres: dict[str, ConfigSectionSRE] = defaultdict(ConfigSectionSRE) # Read backend settings settings = ContextSettings.from_file() + context = settings.context # Check if backend exists and was loaded try: - self.name = settings.name + self.name = context.name except DataSafeHavenParameterError as exc: msg = "Data Safe Haven has not been initialised: run '[bright_cyan]dsh init[/]' before continuing." raise DataSafeHavenConfigError(msg) from exc - self.subscription_name = settings.subscription_name - self.azure.location = settings.location - self.azure.admin_group_id = settings.admin_group_id + self.subscription_name = context.subscription_name + self.azure.location = context.location + self.azure.admin_group_id = context.admin_group_id self.backend_storage_container_name = "config" # Set derived names self.shm_name_ = alphanumeric(self.name).lower() @@ -373,7 +375,7 @@ def __init__(self) -> None: self.backend_storage_account_name = ( f"shm{self.shm_name_[:14]}backend" # maximum of 24 characters allowed ) - self.work_directory = settings.config_directory / self.shm_name_ + self.work_directory = config_dir() / self.shm_name_ self.azure_api = AzureApi(subscription_name=self.subscription_name) # Attempt to load YAML dictionary from blob storage yaml_input = {} diff --git a/data_safe_haven/config/context_settings.py b/data_safe_haven/config/context_settings.py index c4bdff7983..bf0943ab9e 100644 --- a/data_safe_haven/config/context_settings.py +++ b/data_safe_haven/config/context_settings.py @@ -1,10 +1,13 @@ """Load global and local settings from dotfiles""" +# For postponed evaluation of annotations https://peps.python.org/pep-0563 +from __future__ import ( + annotations, +) + from dataclasses import dataclass -from os import getenv from pathlib import Path from typing import Any -import appdirs import yaml from schema import Schema, SchemaError from yaml.parser import ParserError @@ -13,18 +16,11 @@ DataSafeHavenConfigError, DataSafeHavenParameterError, ) -from data_safe_haven.utility import LoggingSingleton +from data_safe_haven.utility import LoggingSingleton, config_dir def default_config_file_path() -> Path: - if config_directory_env := getenv("DSH_CONFIG_DIRECTORY"): - config_directory = Path(config_directory_env).resolve() - else: - config_directory = Path( - appdirs.user_config_dir(appname="data_safe_haven") - ).resolve() - - return config_directory / "contexts.yaml" + return config_dir() / "contexts.yaml" @dataclass @@ -72,7 +68,7 @@ def __init__(self, settings_dict: dict[Any, Any]) -> None: ) try: - self._settings = schema.validate(settings_dict) + self._settings: dict[Any, Any] = schema.validate(settings_dict) except SchemaError as exc: msg = f"Invalid context configuration file.\n{exc}" raise DataSafeHavenParameterError(msg) from exc @@ -83,11 +79,7 @@ def settings(self) -> dict[Any, Any]: @property def selected(self) -> str: - if selected := self.settings.get("selected"): - return selected - else: - msg = "Selected context is not defined." - raise DataSafeHavenParameterError(msg) + return str(self.settings["selected"]) @selected.setter def selected(self, context_name: str) -> None: @@ -100,15 +92,11 @@ def selected(self, context_name: str) -> None: @property def context(self) -> Context: - if context_dict := self.settings.get("contexts").get(self.selected): - return Context(**context_dict) - else: - msg = f"Context {self.selected} is not defined." - raise DataSafeHavenParameterError(msg) + return Context(**self.settings["contexts"][self.selected]) @property def available(self) -> list[str]: - return list(self.settings.get("contexts").keys()) + return list(self.settings["contexts"].keys()) def update( self, @@ -118,7 +106,7 @@ def update( name: str | None = None, subscription_name: str | None = None, ) -> None: - context_dict = self.settings.get("contexts").get(self.selected) + context_dict = self.settings["contexts"][self.selected] if admin_group_id: self.logger.debug( @@ -165,7 +153,7 @@ def remove(self, key: str) -> None: del self.settings["contexts"][key] @classmethod - def from_file(cls, config_file_path: str | None = None) -> None: + def from_file(cls, config_file_path: Path | None = None) -> ContextSettings: if config_file_path is None: config_file_path = default_config_file_path() logger = LoggingSingleton() @@ -177,6 +165,9 @@ def from_file(cls, config_file_path: str | None = None) -> None: f"Reading project settings from '[green]{config_file_path}[/]'." ) return cls(settings) + else: + msg = f"Unable to parse {config_file_path} as a dict." + raise DataSafeHavenConfigError(msg) except FileNotFoundError as exc: msg = f"Could not find file {config_file_path}.\n{exc}" raise DataSafeHavenConfigError(msg) from exc @@ -184,7 +175,7 @@ def from_file(cls, config_file_path: str | None = None) -> None: msg = f"Could not load settings from {config_file_path}.\n{exc}" raise DataSafeHavenConfigError(msg) from exc - def write(self, config_file_path: str | None = None) -> None: + def write(self, config_file_path: Path | None = None) -> None: """Write settings to YAML file""" if config_file_path is None: config_file_path = default_config_file_path() diff --git a/data_safe_haven/infrastructure/stack_manager.py b/data_safe_haven/infrastructure/stack_manager.py index 48d6c02d1b..398a1f82ec 100644 --- a/data_safe_haven/infrastructure/stack_manager.py +++ b/data_safe_haven/infrastructure/stack_manager.py @@ -68,7 +68,9 @@ def __init__( self.program = program self.project_name = replace_separators(self.cfg.tags.project.lower(), "-") self.stack_name = self.program.stack_name - self.work_dir = config.work_directory / "pulumi" / self.program.short_name + self.work_dir: pathlib.Path = ( + config.work_directory / "pulumi" / self.program.short_name + ) self.work_dir.mkdir(parents=True, exist_ok=True) self.initialise_workdir() self.install_plugins() diff --git a/data_safe_haven/utility/__init__.py b/data_safe_haven/utility/__init__.py index 73ab4620ec..923e6f31f5 100644 --- a/data_safe_haven/utility/__init__.py +++ b/data_safe_haven/utility/__init__.py @@ -1,3 +1,4 @@ +from .directories import config_dir from .enums import DatabaseSystem, SoftwarePackageCategory from .file_reader import FileReader from .logger import LoggingSingleton, NonLoggingSingleton @@ -5,6 +6,7 @@ from .types import PathType __all__ = [ + "config_dir", "DatabaseSystem", "FileReader", "LoggingSingleton", diff --git a/data_safe_haven/utility/directories.py b/data_safe_haven/utility/directories.py new file mode 100644 index 0000000000..593f64bb50 --- /dev/null +++ b/data_safe_haven/utility/directories.py @@ -0,0 +1,15 @@ +from os import getenv +from pathlib import Path + +import appdirs + + +def config_dir() -> Path: + if config_directory_env := getenv("DSH_CONFIG_DIRECTORY"): + config_directory = Path(config_directory_env).resolve() + else: + config_directory = Path( + appdirs.user_config_dir(appname="data_safe_haven") + ).resolve() + + return config_directory diff --git a/typings/schema/__init__.pyi b/typings/schema/__init__.pyi new file mode 100644 index 0000000000..2bd78ba644 --- /dev/null +++ b/typings/schema/__init__.pyi @@ -0,0 +1,10 @@ +from typing import Any + + +class SchemaError(Exception): + ... + + +class Schema: + def __init__(self, schema: dict[Any, Any]) -> None: ... + def validate(self, data: Any) -> Any: ... From 35dc59f399d98e9b4ddf67c2c6fe066b06e43bae Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 13:13:54 +0000 Subject: [PATCH 22/36] WIP: use available method --- data_safe_haven/config/context_settings.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/data_safe_haven/config/context_settings.py b/data_safe_haven/config/context_settings.py index bf0943ab9e..32c0cb0b75 100644 --- a/data_safe_haven/config/context_settings.py +++ b/data_safe_haven/config/context_settings.py @@ -83,7 +83,7 @@ def selected(self) -> str: @selected.setter def selected(self, context_name: str) -> None: - if context_name in self.settings["contexts"].keys(): + if context_name in self.available: self.settings["selected"] = context_name self.logger.info(f"Switched context to '{context_name}'.") else: @@ -135,7 +135,7 @@ def add( subscription_name: str, ) -> None: # Ensure context is not already present - if key in self.settings["contexts"].keys(): + if key in self.available: msg = f"A context with key '{key}' is already defined." raise DataSafeHavenParameterError(msg) @@ -147,7 +147,7 @@ def add( } def remove(self, key: str) -> None: - if key not in self.settings["contexts"].keys(): + if key not in self.available: msg = f"No context with key '{key}'." raise DataSafeHavenParameterError(msg) del self.settings["contexts"][key] From bb144b0fcf03bb65f2de10d333a54591fdacd3c3 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 13:18:24 +0000 Subject: [PATCH 23/36] WIP: Correct workflow step name --- .github/workflows/test_code.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test_code.yaml b/.github/workflows/test_code.yaml index 4ae7b72ec7..f25d44b1f4 100644 --- a/.github/workflows/test_code.yaml +++ b/.github/workflows/test_code.yaml @@ -20,7 +20,7 @@ jobs: python-version: 3.11 - name: Install hatch run: pip install hatch - - name: Lint Python + - name: Test Python run: hatch run test:test test_markdown_links: From e6c849902d9b35e0ed6fad19aa894a212a062717 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 13:35:01 +0000 Subject: [PATCH 24/36] WIP: Tidy entrypoints --- data_safe_haven/commands/context.py | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 06c4c2e6b8..9055728a9d 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -6,10 +6,6 @@ from data_safe_haven.backend import Backend from data_safe_haven.config import ContextSettings -from data_safe_haven.exceptions import ( - DataSafeHavenError, - DataSafeHavenInputError, -) from data_safe_haven.functions import validate_aad_guid context_command_group = typer.Typer() @@ -141,22 +137,14 @@ def remove( @context_command_group.command() def create() -> None: - backend = Backend() # How does this get the config!?! + """Create Data Safe Haven context infrastructure""" + backend = Backend() backend.create() - - backend.config.upload() # What does this do? + backend.config.upload() @context_command_group.command() def teardown() -> None: - """Tear down a Data Safe Haven context""" - try: - try: - backend = Backend() - backend.teardown() - except Exception as exc: - msg = f"Unable to teardown Pulumi backend.\n{exc}" - raise DataSafeHavenInputError(msg) from exc # Input error? No input. - except DataSafeHavenError as exc: - msg = f"Could not teardown Data Safe Haven backend.\n{exc}" - raise DataSafeHavenError(msg) from exc + """Tear down Data Safe Haven context infrastructure""" + backend = Backend() + backend.teardown() From e331299b3a6db341f5941058fad1eeab541e7bec Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 13:59:18 +0000 Subject: [PATCH 25/36] WIP: Rename 'backend' to context --- data_safe_haven/backend/__init__.py | 5 -- data_safe_haven/commands/context.py | 12 ++--- data_safe_haven/commands/teardown.py | 6 --- data_safe_haven/commands/teardown_backend.py | 21 -------- data_safe_haven/config/config.py | 54 +++++++++---------- data_safe_haven/context/__init__.py | 5 ++ .../backend.py => context/context.py} | 30 +++++------ .../interface/azure_container_instance.py | 1 - .../interface/azure_postgresql_database.py | 1 - 9 files changed, 52 insertions(+), 83 deletions(-) delete mode 100644 data_safe_haven/backend/__init__.py delete mode 100644 data_safe_haven/commands/teardown_backend.py create mode 100644 data_safe_haven/context/__init__.py rename data_safe_haven/{backend/backend.py => context/context.py} (81%) diff --git a/data_safe_haven/backend/__init__.py b/data_safe_haven/backend/__init__.py deleted file mode 100644 index 7401760657..0000000000 --- a/data_safe_haven/backend/__init__.py +++ /dev/null @@ -1,5 +0,0 @@ -from .backend import Backend - -__all__ = [ - "Backend", -] diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 9055728a9d..da70dbfd87 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -4,7 +4,7 @@ import typer from rich import print -from data_safe_haven.backend import Backend +from data_safe_haven.context import Context from data_safe_haven.config import ContextSettings from data_safe_haven.functions import validate_aad_guid @@ -138,13 +138,13 @@ def remove( @context_command_group.command() def create() -> None: """Create Data Safe Haven context infrastructure""" - backend = Backend() - backend.create() - backend.config.upload() + context = Context() + context.create() + context.config.upload() @context_command_group.command() def teardown() -> None: """Tear down Data Safe Haven context infrastructure""" - backend = Backend() - backend.teardown() + context = Context() + context.teardown() diff --git a/data_safe_haven/commands/teardown.py b/data_safe_haven/commands/teardown.py index 2895d20ee7..5e6e7d08ec 100644 --- a/data_safe_haven/commands/teardown.py +++ b/data_safe_haven/commands/teardown.py @@ -3,18 +3,12 @@ import typer -from .teardown_backend import teardown_backend from .teardown_shm import teardown_shm from .teardown_sre import teardown_sre teardown_command_group = typer.Typer() -@teardown_command_group.command(help="Tear down a deployed Data Safe Haven backend.") -def backend() -> None: - teardown_backend() - - @teardown_command_group.command( help="Tear down a deployed a Safe Haven Management component." ) diff --git a/data_safe_haven/commands/teardown_backend.py b/data_safe_haven/commands/teardown_backend.py deleted file mode 100644 index f90e4a6c16..0000000000 --- a/data_safe_haven/commands/teardown_backend.py +++ /dev/null @@ -1,21 +0,0 @@ -"""Tear down a deployed Data Safe Haven backend""" -from data_safe_haven.backend import Backend -from data_safe_haven.exceptions import ( - DataSafeHavenError, - DataSafeHavenInputError, -) - - -def teardown_backend() -> None: - """Tear down a deployed Data Safe Haven backend""" - try: - # Remove the Pulumi backend - try: - backend = Backend() - backend.teardown() - except Exception as exc: - msg = f"Unable to teardown Pulumi backend.\n{exc}" - raise DataSafeHavenInputError(msg) from exc - except DataSafeHavenError as exc: - msg = f"Could not teardown Data Safe Haven backend.\n{exc}" - raise DataSafeHavenError(msg) from exc diff --git a/data_safe_haven/config/config.py b/data_safe_haven/config/config.py index f7d920a910..c0f32247df 100644 --- a/data_safe_haven/config/config.py +++ b/data_safe_haven/config/config.py @@ -95,7 +95,7 @@ class ConfigSectionAzure(ConfigSection): @dataclass -class ConfigSectionBackend(ConfigSection): +class ConfigSectionContext(ConfigSection): key_vault_name: str = "" managed_identity_name: str = "" resource_group_name: str = "" @@ -350,12 +350,12 @@ class Config: def __init__(self) -> None: # Initialise config sections self.azure_: ConfigSectionAzure | None = None - self.backend_: ConfigSectionBackend | None = None + self.context_: ConfigSectionContext | None = None self.pulumi_: ConfigSectionPulumi | None = None self.shm_: ConfigSectionSHM | None = None self.tags_: ConfigSectionTags | None = None self.sres: dict[str, ConfigSectionSRE] = defaultdict(ConfigSectionSRE) - # Read backend settings + # Read context settings settings = ContextSettings.from_file() context = settings.context # Check if backend exists and was loaded @@ -367,13 +367,13 @@ def __init__(self) -> None: self.subscription_name = context.subscription_name self.azure.location = context.location self.azure.admin_group_id = context.admin_group_id - self.backend_storage_container_name = "config" + self.context_storage_container_name = "config" # Set derived names self.shm_name_ = alphanumeric(self.name).lower() self.filename = f"config-{self.shm_name_}.yaml" - self.backend_resource_group_name = f"shm-{self.shm_name_}-rg-backend" - self.backend_storage_account_name = ( - f"shm{self.shm_name_[:14]}backend" # maximum of 24 characters allowed + self.context_resource_group_name = f"shm-{self.shm_name_}-rg-context" + self.context_storage_account_name = ( + f"shm{self.shm_name_[:14]}context" # maximum of 24 characters allowed ) self.work_directory = config_dir() / self.shm_name_ self.azure_api = AzureApi(subscription_name=self.subscription_name) @@ -383,18 +383,18 @@ def __init__(self) -> None: yaml_input = yaml.safe_load( self.azure_api.download_blob( self.filename, - self.backend_resource_group_name, - self.backend_storage_account_name, - self.backend_storage_container_name, + self.context_resource_group_name, + self.context_storage_account_name, + self.context_storage_container_name, ) ) # Attempt to decode each config section if yaml_input: if "azure" in yaml_input: self.azure_ = chili.decode(yaml_input["azure"], ConfigSectionAzure) - if "backend" in yaml_input: - self.backend_ = chili.decode( - yaml_input["backend"], ConfigSectionBackend + if "context" in yaml_input: + self.context_ = chili.decode( + yaml_input["context"], ConfigSectionContext ) if "pulumi" in yaml_input: self.pulumi_ = chili.decode(yaml_input["pulumi"], ConfigSectionPulumi) @@ -411,16 +411,16 @@ def azure(self) -> ConfigSectionAzure: return self.azure_ @property - def backend(self) -> ConfigSectionBackend: - if not self.backend_: - self.backend_ = ConfigSectionBackend( - key_vault_name=f"shm-{self.shm_name_[:9]}-kv-backend", - managed_identity_name=f"shm-{self.shm_name_}-identity-reader-backend", - resource_group_name=self.backend_resource_group_name, - storage_account_name=self.backend_storage_account_name, - storage_container_name=self.backend_storage_container_name, + def context(self) -> ConfigSectionContext: + if not self.context_: + self.context_ = ConfigSectionContext( + key_vault_name=f"shm-{self.shm_name_[:9]}-kv-context", + managed_identity_name=f"shm-{self.shm_name_}-identity-reader-context", + resource_group_name=self.context_resource_group_name, + storage_account_name=self.context_storage_account_name, + storage_container_name=self.context_storage_container_name, ) - return self.backend_ + return self.context_ @property def pulumi(self) -> ConfigSectionPulumi: @@ -445,8 +445,8 @@ def __str__(self) -> str: contents: dict[str, Any] = {} if self.azure_: contents["azure"] = self.azure.to_dict() - if self.backend_: - contents["backend"] = self.backend.to_dict() + if self.context_: + contents["context"] = self.context.to_dict() if self.pulumi_: contents["pulumi"] = self.pulumi.to_dict() if self.shm_: @@ -485,9 +485,9 @@ def upload(self) -> None: self.azure_api.upload_blob( str(self), self.filename, - self.backend_resource_group_name, - self.backend_storage_account_name, - self.backend_storage_container_name, + self.context_resource_group_name, + self.context_storage_account_name, + self.context_storage_container_name, ) def write_stack(self, name: str, path: pathlib.Path) -> None: diff --git a/data_safe_haven/context/__init__.py b/data_safe_haven/context/__init__.py new file mode 100644 index 0000000000..94370d9ba1 --- /dev/null +++ b/data_safe_haven/context/__init__.py @@ -0,0 +1,5 @@ +from .context import Context + +__all__ = [ + "Context", +] diff --git a/data_safe_haven/backend/backend.py b/data_safe_haven/context/context.py similarity index 81% rename from data_safe_haven/backend/backend.py rename to data_safe_haven/context/context.py index a303955af1..bbb037c2ca 100644 --- a/data_safe_haven/backend/backend.py +++ b/data_safe_haven/context/context.py @@ -1,17 +1,15 @@ -"""Azure backend for a Data Safe Haven deployment""" - from data_safe_haven.config import Config from data_safe_haven.exceptions import DataSafeHavenAzureError from data_safe_haven.external import AzureApi -class Backend: - """Azure backend for a Data Safe Haven deployment""" +class Context: + """Azure resources to support Data Safe Haven context""" def __init__(self) -> None: self.azure_api_: AzureApi | None = None self.config = Config() - self.tags = {"component": "backend"} | self.config.tags.to_dict() + self.tags = {"component": "context"} | self.config.tags.to_dict() @property def azure_api(self) -> AzureApi: @@ -37,28 +35,28 @@ def create(self) -> None: self.config.azure.tenant_id = self.azure_api.tenant_id resource_group = self.azure_api.ensure_resource_group( location=self.config.azure.location, - resource_group_name=self.config.backend.resource_group_name, + resource_group_name=self.config.context.resource_group_name, tags=self.tags, ) if not resource_group.name: - msg = f"Resource group '{self.config.backend.resource_group_name}' was not created." + msg = f"Resource group '{self.config.context.resource_group_name}' was not created." raise DataSafeHavenAzureError(msg) identity = self.azure_api.ensure_managed_identity( - identity_name=self.config.backend.managed_identity_name, + identity_name=self.config.context.managed_identity_name, location=resource_group.location, resource_group_name=resource_group.name, ) storage_account = self.azure_api.ensure_storage_account( location=resource_group.location, resource_group_name=resource_group.name, - storage_account_name=self.config.backend.storage_account_name, + storage_account_name=self.config.context.storage_account_name, tags=self.tags, ) if not storage_account.name: - msg = f"Storage account '{self.config.backend.storage_account_name}' was not created." + msg = f"Storage account '{self.config.context.storage_account_name}' was not created." raise DataSafeHavenAzureError(msg) _ = self.azure_api.ensure_storage_blob_container( - container_name=self.config.backend.storage_container_name, + container_name=self.config.context.storage_container_name, resource_group_name=resource_group.name, storage_account_name=storage_account.name, ) @@ -69,7 +67,7 @@ def create(self) -> None: ) keyvault = self.azure_api.ensure_keyvault( admin_group_id=self.config.azure.admin_group_id, - key_vault_name=self.config.backend.key_vault_name, + key_vault_name=self.config.context.key_vault_name, location=resource_group.location, managed_identity=identity, resource_group_name=resource_group.name, @@ -77,7 +75,7 @@ def create(self) -> None: ) if not keyvault.name: msg = ( - f"Keyvault '{self.config.backend.key_vault_name}' was not created." + f"Keyvault '{self.config.context.key_vault_name}' was not created." ) raise DataSafeHavenAzureError(msg) pulumi_encryption_key = self.azure_api.ensure_keyvault_key( @@ -87,7 +85,7 @@ def create(self) -> None: key_version = pulumi_encryption_key.id.split("/")[-1] self.config.pulumi.encryption_key_version = key_version except Exception as exc: - msg = f"Failed to create backend resources.\n{exc}" + msg = f"Failed to create context resources.\n{exc}" raise DataSafeHavenAzureError(msg) from exc def teardown(self) -> None: @@ -98,8 +96,8 @@ def teardown(self) -> None: """ try: self.azure_api.remove_resource_group( - self.config.backend.resource_group_name + self.config.context.resource_group_name ) except Exception as exc: - msg = f"Failed to destroy backend resources.\n{exc}" + msg = f"Failed to destroy context resources.\n{exc}" raise DataSafeHavenAzureError(msg) from exc diff --git a/data_safe_haven/external/interface/azure_container_instance.py b/data_safe_haven/external/interface/azure_container_instance.py index 623c837555..1f696f4b04 100644 --- a/data_safe_haven/external/interface/azure_container_instance.py +++ b/data_safe_haven/external/interface/azure_container_instance.py @@ -1,4 +1,3 @@ -"""Backend for a Data Safe Haven environment""" import contextlib import time diff --git a/data_safe_haven/external/interface/azure_postgresql_database.py b/data_safe_haven/external/interface/azure_postgresql_database.py index 395791105a..ad0edc30da 100644 --- a/data_safe_haven/external/interface/azure_postgresql_database.py +++ b/data_safe_haven/external/interface/azure_postgresql_database.py @@ -1,4 +1,3 @@ -"""Backend for a Data Safe Haven environment""" import datetime import pathlib import time From d12c403bb2007323cc3781b5171081fd3ae6d509 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 15:17:16 +0000 Subject: [PATCH 26/36] Add tests for create and teardown commands --- data_safe_haven/commands/context.py | 8 +++++--- data_safe_haven/context/context.py | 4 ++-- tests_/commands/test_context.py | 31 +++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index da70dbfd87..c61e4f65d2 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -5,7 +5,7 @@ from rich import print from data_safe_haven.context import Context -from data_safe_haven.config import ContextSettings +from data_safe_haven.config import Config, ContextSettings from data_safe_haven.functions import validate_aad_guid context_command_group = typer.Typer() @@ -138,7 +138,8 @@ def remove( @context_command_group.command() def create() -> None: """Create Data Safe Haven context infrastructure""" - context = Context() + config = Config() + context = Context(config) context.create() context.config.upload() @@ -146,5 +147,6 @@ def create() -> None: @context_command_group.command() def teardown() -> None: """Tear down Data Safe Haven context infrastructure""" - context = Context() + config = Config() + context = Context(config) context.teardown() diff --git a/data_safe_haven/context/context.py b/data_safe_haven/context/context.py index bbb037c2ca..234dc6c4c0 100644 --- a/data_safe_haven/context/context.py +++ b/data_safe_haven/context/context.py @@ -6,9 +6,9 @@ class Context: """Azure resources to support Data Safe Haven context""" - def __init__(self) -> None: + def __init__(self, config: Config) -> None: self.azure_api_: AzureApi | None = None - self.config = Config() + self.config = config self.tags = {"component": "context"} | self.config.tags.to_dict() @property diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index 621a4885c6..047cdd5c48 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -1,4 +1,6 @@ from data_safe_haven.commands.context import context_command_group +from data_safe_haven.config import Config +from data_safe_haven.context import Context from pytest import fixture from typer.testing import CliRunner @@ -167,3 +169,32 @@ def test_remove_invalid(self, runner): assert result.exit_code == 1 # Unable to check error as this is written outside of any Typer # assert "No context with key 'invalid'." in result.stdout + + +class TestCreate: + def test_create(self, runner, monkeypatch): + def mock_create(self): + print("mock create") + + def mock_upload(self): + print("mock upload") + + monkeypatch.setattr(Context, "create", mock_create) + monkeypatch.setattr(Config, "upload", mock_upload) + + result = runner.invoke(context_command_group, ["create"]) + assert "mock create" in result.stdout + assert "mock upload" in result.stdout + assert result.exit_code == 0 + + +class TestTeardown: + def test_teardown(self, runner, monkeypatch): + def mock_teardown(self): + print("mock teardown") + + monkeypatch.setattr(Context, "teardown", mock_teardown) + + result = runner.invoke(context_command_group, ["teardown"]) + assert "mock teardown" in result.stdout + assert result.exit_code == 0 From 087ec8d6916e4ce065e745ab45a6512eda9a502d Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 15:21:41 +0000 Subject: [PATCH 27/36] Sort import block --- data_safe_haven/commands/context.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index c61e4f65d2..8bc1de6706 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -4,8 +4,8 @@ import typer from rich import print -from data_safe_haven.context import Context from data_safe_haven.config import Config, ContextSettings +from data_safe_haven.context import Context from data_safe_haven.functions import validate_aad_guid context_command_group = typer.Typer() From 67b1ff46941639b787b58dbc73dfcdbae0413df6 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 15:36:57 +0000 Subject: [PATCH 28/36] Update renamed configuration section references --- data_safe_haven/infrastructure/stack_manager.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/data_safe_haven/infrastructure/stack_manager.py b/data_safe_haven/infrastructure/stack_manager.py index 398a1f82ec..f63026ff1b 100644 --- a/data_safe_haven/infrastructure/stack_manager.py +++ b/data_safe_haven/infrastructure/stack_manager.py @@ -39,11 +39,11 @@ def env(self) -> dict[str, Any]: if not self.env_: azure_api = AzureApi(self.cfg.subscription_name) backend_storage_account_keys = azure_api.get_storage_account_keys( - self.cfg.backend.resource_group_name, - self.cfg.backend.storage_account_name, + self.cfg.context.resource_group_name, + self.cfg.context.storage_account_name, ) self.env_ = { - "AZURE_STORAGE_ACCOUNT": self.cfg.backend.storage_account_name, + "AZURE_STORAGE_ACCOUNT": self.cfg.context.storage_account_name, "AZURE_STORAGE_KEY": str(backend_storage_account_keys[0].value), "AZURE_KEYVAULT_AUTH_VIA_CLI": "true", "PULUMI_BACKEND_URL": f"azblob://{self.cfg.pulumi.storage_container_name}", @@ -100,7 +100,7 @@ def stack(self) -> automation.Stack: stack_name=self.stack_name, program=self.program.run, opts=automation.LocalWorkspaceOptions( - secrets_provider=f"azurekeyvault://{self.cfg.backend.key_vault_name}.vault.azure.net/keys/{self.cfg.pulumi.encryption_key_name}/{self.cfg.pulumi.encryption_key_version}", + secrets_provider=f"azurekeyvault://{self.cfg.context.key_vault_name}.vault.azure.net/keys/{self.cfg.pulumi.encryption_key_name}/{self.cfg.pulumi.encryption_key_version}", work_dir=str(self.work_dir), env_vars=self.account.env, ), @@ -213,8 +213,8 @@ def destroy(self) -> None: azure_api = AzureApi(self.cfg.subscription_name) azure_api.remove_blob( blob_name=f".pulumi/stacks/{self.project_name}/{stack_backup_name}", - resource_group_name=self.cfg.backend.resource_group_name, - storage_account_name=self.cfg.backend.storage_account_name, + resource_group_name=self.cfg.context.resource_group_name, + storage_account_name=self.cfg.context.storage_account_name, storage_container_name=self.cfg.pulumi.storage_container_name, ) except DataSafeHavenAzureError as exc: From 1d85e2568d016bfe708b51da5d238c3a1c90bd13 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Thu, 2 Nov 2023 15:43:37 +0000 Subject: [PATCH 29/36] Tidy ContextSettings tests with fixtures --- tests_/config/test_context_settings.py | 136 ++++++++++++------------- 1 file changed, 66 insertions(+), 70 deletions(-) diff --git a/tests_/config/test_context_settings.py b/tests_/config/test_context_settings.py index 09a1dfb338..66783d788e 100644 --- a/tests_/config/test_context_settings.py +++ b/tests_/config/test_context_settings.py @@ -3,6 +3,7 @@ import pytest import yaml +from pytest import fixture class TestContext: @@ -20,8 +21,9 @@ def test_constructor(self): ]) -class TestContextSettings: - context_settings = """\ +@fixture +def context_yaml(): + context_yaml = """\ selected: acme_deployment contexts: acme_deployment: @@ -34,96 +36,92 @@ class TestContextSettings: admin_group_id: d5c5c439-1115-4cb6-ab50-b8e547b6c8dd location: uksouth subscription_name: Data Safe Haven (Gems)""" + return context_yaml - def test_constructor(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) + +@fixture +def context_settings(context_yaml): + return ContextSettings(yaml.safe_load(context_yaml)) + + +class TestContextSettings: + def test_constructor(self, context_yaml): + settings = ContextSettings(yaml.safe_load(context_yaml)) assert isinstance(settings, ContextSettings) - def test_invalid(self): - context_settings = "\n".join(self.context_settings.splitlines()[1:]) + def test_missing_selected(self, context_yaml): + context_yaml = "\n".join(context_yaml.splitlines()[1:]) with pytest.raises(DataSafeHavenParameterError) as exc: - ContextSettings(yaml.safe_load(context_settings)) + ContextSettings(yaml.safe_load(context_yaml)) assert "Missing Key: 'selected'" in exc - def test_settings(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - assert isinstance(settings.settings, dict) + def test_settings(self, context_settings): + assert isinstance(context_settings.settings, dict) - def test_selected(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - assert settings.selected == "acme_deployment" + def test_selected(self, context_settings): + assert context_settings.selected == "acme_deployment" - def test_set_selected(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - assert settings.selected == "acme_deployment" - settings.selected = "gems" - assert settings.selected == "gems" + def test_set_selected(self, context_settings): + assert context_settings.selected == "acme_deployment" + context_settings.selected = "gems" + assert context_settings.selected == "gems" - def test_invalid_selected(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) + def test_invalid_selected(self, context_settings): with pytest.raises(DataSafeHavenParameterError) as exc: - settings.selected = "invalid" + context_settings.selected = "invalid" assert "Context invalid is not defined." in exc - def test_context(self): - yaml_settings = yaml.safe_load(self.context_settings) - settings = ContextSettings(yaml_settings) - assert isinstance(settings.context, Context) + def test_context(self, context_yaml, context_settings): + yaml_dict = yaml.safe_load(context_yaml) + assert isinstance(context_settings.context, Context) assert all([ - getattr(settings.context, item) == yaml_settings["contexts"]["acme_deployment"][item] - for item in yaml_settings["contexts"]["acme_deployment"].keys() + getattr(context_settings.context, item) == yaml_dict["contexts"]["acme_deployment"][item] + for item in yaml_dict["contexts"]["acme_deployment"].keys() ]) - def test_set_context(self): - yaml_settings = yaml.safe_load(self.context_settings) - settings = ContextSettings(yaml_settings) - settings.selected = "gems" - assert isinstance(settings.context, Context) + def test_set_context(self, context_yaml, context_settings): + yaml_dict = yaml.safe_load(context_yaml) + context_settings.selected = "gems" + assert isinstance(context_settings.context, Context) assert all([ - getattr(settings.context, item) == yaml_settings["contexts"]["gems"][item] - for item in yaml_settings["contexts"]["gems"].keys() + getattr(context_settings.context, item) == yaml_dict["contexts"]["gems"][item] + for item in yaml_dict["contexts"]["gems"].keys() ]) - def test_available(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - available = settings.available - print(available) + def test_available(self, context_settings): + available = context_settings.available assert isinstance(available, list) assert all([isinstance(item, str) for item in available]) assert available == ["acme_deployment", "gems"] - def test_update(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - assert settings.context.name == "Acme Deployment" - settings.update(name="replaced") - assert settings.context.name == "replaced" + def test_update(self, context_settings): + assert context_settings.context.name == "Acme Deployment" + context_settings.update(name="replaced") + assert context_settings.context.name == "replaced" - def test_set_update(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - settings.selected = "gems" - assert settings.context.name == "Gems" - settings.update(name="replaced") - assert settings.context.name == "replaced" + def test_set_update(self, context_settings): + context_settings.selected = "gems" + assert context_settings.context.name == "Gems" + context_settings.update(name="replaced") + assert context_settings.context.name == "replaced" - def test_add(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - settings.add( + def test_add(self, context_settings): + context_settings.add( key="example", name="Example", subscription_name="Data Safe Haven (Example)", admin_group_id="d5c5c439-1115-4cb6-ab50-b8e547b6c8dd", location="uksouth", ) - settings.selected = "example" - assert settings.selected == "example" - assert settings.context.name == "Example" - assert settings.context.subscription_name == "Data Safe Haven (Example)" + context_settings.selected = "example" + assert context_settings.selected == "example" + assert context_settings.context.name == "Example" + assert context_settings.context.subscription_name == "Data Safe Haven (Example)" - def test_invalid_add(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) + def test_invalid_add(self, context_settings): with pytest.raises(DataSafeHavenParameterError) as exc: - settings.add( + context_settings.add( key="acme_deployment", name="Acme Deployment", subscription_name="Data Safe Haven (Acme)", @@ -132,28 +130,26 @@ def test_invalid_add(self): ) assert "A context with key 'acme' is already defined." in exc - def test_remove(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) - settings.remove("acme_deployment") - assert "acme_deployment" not in settings.available + def test_remove(self, context_settings): + context_settings.remove("acme_deployment") + assert "acme_deployment" not in context_settings.available - def test_invalid_remove(self): - settings = ContextSettings(yaml.safe_load(self.context_settings)) + def test_invalid_remove(self, context_settings): with pytest.raises(DataSafeHavenParameterError) as exc: - settings.remove("invalid") + context_settings.remove("invalid") assert "No context with key 'invalid'." in exc - def test_from_file(self, tmp_path): + def test_from_file(self, tmp_path, context_yaml): config_file_path = tmp_path / "config.yaml" with open(config_file_path, "w") as f: - f.write(self.context_settings) + f.write(context_yaml) settings = ContextSettings.from_file(config_file_path=config_file_path) assert settings.context.name == "Acme Deployment" - def test_write(self, tmp_path): + def test_write(self, tmp_path, context_yaml): config_file_path = tmp_path / "config.yaml" with open(config_file_path, "w") as f: - f.write(self.context_settings) + f.write(context_yaml) settings = ContextSettings.from_file(config_file_path=config_file_path) settings.selected = "gems" settings.update(name="replaced") From 7805f45480a6f057b8a0f2d0899885ccf87f0844 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 3 Nov 2023 10:58:56 +0000 Subject: [PATCH 30/36] Disable some rich features during tests This should help testing be more consistent across machines/CI. --- tests_/commands/test_context.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index 047cdd5c48..51e902f445 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -33,7 +33,8 @@ def runner(tmp_contexts): runner = CliRunner( env={ "DSH_CONFIG_DIRECTORY": str(tmp_contexts), - "COLUMNS": "500" # Set large number of columns to avoid rich wrapping text + "COLUMNS": "500", # Set large number of columns to avoid rich wrapping text + "TERM": "dumb", # Disable colours, style and interactive rich features }, mix_stderr=False, ) From ac99832cfe304dd7fd80e1fa0f7f9dc6458555c3 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 3 Nov 2023 11:25:34 +0000 Subject: [PATCH 31/36] Allow bootstrapping context settings file --- data_safe_haven/commands/context.py | 33 ++++++++++++++++++++++------- tests_/commands/test_context.py | 27 +++++++++++++++++++++++ 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 8bc1de6706..89eac06dee 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -5,6 +5,7 @@ from rich import print from data_safe_haven.config import Config, ContextSettings +from data_safe_haven.config.context_settings import default_config_file_path from data_safe_haven.context import Context from data_safe_haven.functions import validate_aad_guid @@ -76,14 +77,30 @@ def add( ), ], ) -> None: - settings = ContextSettings.from_file() - settings.add( - key=key, - admin_group_id=admin_group, - location=location, - name=name, - subscription_name=subscription, - ) + if default_config_file_path().exists(): + settings = ContextSettings.from_file() + settings.add( + key=key, + admin_group_id=admin_group, + location=location, + name=name, + subscription_name=subscription, + ) + else: + # Bootstrap context settings file + settings = ContextSettings( + { + "selected": key, + "contexts": { + key: { + "admin_group_id": admin_group, + "location": location, + "name": name, + "subscription_name": subscription, + } + }, + } + ) settings.write() diff --git a/tests_/commands/test_context.py b/tests_/commands/test_context.py index 51e902f445..0cdf878c44 100644 --- a/tests_/commands/test_context.py +++ b/tests_/commands/test_context.py @@ -147,6 +147,33 @@ def test_add_missing_ags(self, runner): assert result.exit_code == 2 assert "Missing option" in result.stderr + def test_add_bootstrap(self, tmp_contexts, runner): + (tmp_contexts / "contexts.yaml").unlink() + result = runner.invoke( + context_command_group, + [ + "add", + "acme_deployment", + "--name", + "Acme Deployment", + "--admin-group", + "d5c5c439-1115-4cb6-ab50-b8e547b6c8dd", + "--location", + "uksouth", + "--subscription", + "Data Safe Haven (Acme)", + ] + ) + assert result.exit_code == 0 + assert (tmp_contexts / "contexts.yaml").exists() + result = runner.invoke(context_command_group, ["show"]) + assert result.exit_code == 0 + assert "Name: Acme Deployment" in result.stdout + result = runner.invoke(context_command_group, ["available"]) + assert result.exit_code == 0 + assert "acme_deployment*" in result.stdout + assert "gems" not in result.stdout + class TestUpdate: def test_update(self, runner): From 878573c522ce79169da9f9b61ecd0fbf686dabd6 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 3 Nov 2023 11:29:10 +0000 Subject: [PATCH 32/36] Add missing help messages --- data_safe_haven/commands/context.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 89eac06dee..01cd5b4611 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -14,6 +14,7 @@ @context_command_group.command() def show() -> None: + """Show information about the selected context.""" settings = ContextSettings.from_file() current_context_key = settings.selected @@ -28,6 +29,7 @@ def show() -> None: @context_command_group.command() def available() -> None: + """Show the available contexts.""" settings = ContextSettings.from_file() current_context_key = settings.selected @@ -43,6 +45,7 @@ def available() -> None: def switch( name: Annotated[str, typer.Argument(help="Name of the context to switch to.")] ) -> None: + """Switch the context.""" settings = ContextSettings.from_file() settings.selected = name settings.write() @@ -77,6 +80,7 @@ def add( ), ], ) -> None: + """Add a new context.""" if default_config_file_path().exists(): settings = ContextSettings.from_file() settings.add( @@ -147,6 +151,7 @@ def update( def remove( key: Annotated[str, typer.Argument(help="Name of the context to add.")], ) -> None: + """Remove the selected context.""" settings = ContextSettings.from_file() settings.remove(key) settings.write() @@ -154,7 +159,7 @@ def remove( @context_command_group.command() def create() -> None: - """Create Data Safe Haven context infrastructure""" + """Create Data Safe Haven context infrastructure.""" config = Config() context = Context(config) context.create() @@ -163,7 +168,7 @@ def create() -> None: @context_command_group.command() def teardown() -> None: - """Tear down Data Safe Haven context infrastructure""" + """Tear down Data Safe Haven context infrastructure.""" config = Config() context = Context(config) context.teardown() From 4bd491456aaccb806cd8f12b11a008fafe95f713 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Fri, 3 Nov 2023 11:36:14 +0000 Subject: [PATCH 33/36] Update README --- data_safe_haven/README.md | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/data_safe_haven/README.md b/data_safe_haven/README.md index 83feed0443..9ed9f0e9ea 100644 --- a/data_safe_haven/README.md +++ b/data_safe_haven/README.md @@ -9,8 +9,9 @@ Install the following requirements before starting - Run the following to initialise the deployment [approx 5 minutes]: -```bash -> dsh init +```console +> dsh context add ... +> dsh context create ``` You will be prompted for various project settings. @@ -18,7 +19,7 @@ If you prefer to enter these at the command line, run `dsh init -h` to see the n - Next deploy the Safe Haven Management (SHM) infrastructure [approx 30 minutes]: -```bash +```console > dsh deploy shm ``` @@ -29,13 +30,13 @@ Run `dsh deploy shm -h` to see the necessary command line flags and provide them Note that the phone number must be in full international format. Note that the country code is the two letter `ISO 3166-1 Alpha-2` code. -```bash +```console > dsh admin add-users ``` - Next deploy the infrastructure for one or more Secure Research Environments (SREs) [approx 30 minutes]: -```bash +```console > dsh deploy sre ``` @@ -44,7 +45,7 @@ Run `dsh deploy sre -h` to see the necessary command line flags and provide them - Next add one or more existing users to your SRE -```bash +```console > dsh admin register-users -s ``` @@ -54,7 +55,7 @@ where you must specify the usernames for each user you want to add to this SRE - Run the following to list the currently available users -```bash +```console > dsh admin list-users ``` @@ -62,20 +63,20 @@ where you must specify the usernames for each user you want to add to this SRE - Run the following if you want to teardown a deployed SRE: -```bash +```console > dsh teardown sre ``` - Run the following if you want to teardown the deployed SHM: -```bash +```console > dsh teardown shm ``` -- Run the following if you want to teardown the deployed Data Safe Haven backend: +- Run the following if you want to teardown the deployed Data Safe Haven context: -```bash -> dsh teardown backend +```console +> dsh context teardown ``` ## Code structure @@ -83,15 +84,16 @@ where you must specify the usernames for each user you want to add to this SRE - administration - this is where we keep utility commands for adminstrators of a deployed DSH - eg. "add a user"; "remove a user from an SRE" -- backend +- context - in order to use the Pulumi Azure backend we need a KeyVault, Identity and Storage Account - this code deploys those resources to bootstrap the rest of the Pulumi-based code + - the storage account is also used to store configuration, so that it can be shared by admins - commands - the main `dsh` command line entrypoint lives in `cli.py` - the subsidiary `typer` command line entrypoints (eg. `dsh deploy shm`) live here - config - serialises and deserialises a config file from Azure - - `backend_settings` manages basic settings related to the Azure backend: arguably this could/should live in `backend` + - `context_settings` manages basic settings related to the context: arguably this could/should live in `context` - exceptions - definitions of a Python exception hierarchy - external From b72874e7251270ad5417fedfa78f8ccecdc0f974 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 7 Nov 2023 10:08:21 +0000 Subject: [PATCH 34/36] Remove old instructions --- data_safe_haven/README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/data_safe_haven/README.md b/data_safe_haven/README.md index 9ed9f0e9ea..03d55a48a8 100644 --- a/data_safe_haven/README.md +++ b/data_safe_haven/README.md @@ -14,9 +14,6 @@ Install the following requirements before starting > dsh context create ``` -You will be prompted for various project settings. -If you prefer to enter these at the command line, run `dsh init -h` to see the necessary command line flags. - - Next deploy the Safe Haven Management (SHM) infrastructure [approx 30 minutes]: ```console From 6b31ea41deff2a77333d1d08d548ee73b22c970d Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 7 Nov 2023 10:09:21 +0000 Subject: [PATCH 35/36] Update data_safe_haven/commands/context.py Co-authored-by: Matt Craddock --- data_safe_haven/commands/context.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 01cd5b4611..5a9ee7e779 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -149,7 +149,7 @@ def update( @context_command_group.command() def remove( - key: Annotated[str, typer.Argument(help="Name of the context to add.")], + key: Annotated[str, typer.Argument(help="Name of the context to remove.")], ) -> None: """Remove the selected context.""" settings = ContextSettings.from_file() From 84dd4e2bbe0f699135aadeafdf03ed0eb9338279 Mon Sep 17 00:00:00 2001 From: Jim Madge Date: Tue, 7 Nov 2023 10:36:47 +0000 Subject: [PATCH 36/36] Update argument names and command descriptions --- data_safe_haven/commands/context.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/data_safe_haven/commands/context.py b/data_safe_haven/commands/context.py index 5a9ee7e779..6a34a74562 100644 --- a/data_safe_haven/commands/context.py +++ b/data_safe_haven/commands/context.py @@ -43,17 +43,17 @@ def available() -> None: @context_command_group.command() def switch( - name: Annotated[str, typer.Argument(help="Name of the context to switch to.")] + key: Annotated[str, typer.Argument(help="Key of the context to switch to.")] ) -> None: - """Switch the context.""" + """Switch the selected context.""" settings = ContextSettings.from_file() - settings.selected = name + settings.selected = key settings.write() @context_command_group.command() def add( - key: Annotated[str, typer.Argument(help="Name of the context to add.")], + key: Annotated[str, typer.Argument(help="Key of the context to add.")], admin_group: Annotated[ str, typer.Option( @@ -80,7 +80,7 @@ def add( ), ], ) -> None: - """Add a new context.""" + """Add a new context to the context list.""" if default_config_file_path().exists(): settings = ContextSettings.from_file() settings.add( @@ -136,7 +136,7 @@ def update( ), ] = None, ) -> None: - """Update the selected context.""" + """Update the selected context settings.""" settings = ContextSettings.from_file() settings.update( admin_group_id=admin_group,