From 423ffc5e081eaee7dcb179d1708050f3840a844d Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 23 Oct 2024 15:19:57 +0000 Subject: [PATCH 01/28] Prevent SHM teardown if SRE is deployed --- data_safe_haven/infrastructure/programs/imperative_shm.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/data_safe_haven/infrastructure/programs/imperative_shm.py b/data_safe_haven/infrastructure/programs/imperative_shm.py index 9b748bbdd1..0af2e6f542 100644 --- a/data_safe_haven/infrastructure/programs/imperative_shm.py +++ b/data_safe_haven/infrastructure/programs/imperative_shm.py @@ -1,4 +1,4 @@ -from data_safe_haven.config import Context, SHMConfig +from data_safe_haven.config import Context, SHMConfig, DSHPulumiConfig from data_safe_haven.exceptions import ( DataSafeHavenAzureError, DataSafeHavenMicrosoftGraphError, @@ -172,6 +172,12 @@ def teardown(self) -> None: DataSafeHavenAzureError if any resources cannot be destroyed """ logger = get_logger() + pulumi_config = DSHPulumiConfig.from_remote(self.context) + deployed = pulumi_config.project_names + if deployed: + logger.info(f"Found deployed Pulumi SREs: {deployed}.") + msg = f"Deployed SREs must be torn down before the SHM can be torn down." + raise DataSafeHavenAzureError(msg) try: logger.info( f"Removing [green]{self.context.description}[/] resource group {self.context.resource_group_name}." From 600f931de2587175c02e250c388c78d69d6ea34d Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 23 Oct 2024 15:21:21 +0000 Subject: [PATCH 02/28] Fix linting --- data_safe_haven/infrastructure/programs/imperative_shm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/infrastructure/programs/imperative_shm.py b/data_safe_haven/infrastructure/programs/imperative_shm.py index 0af2e6f542..7c8eab3f98 100644 --- a/data_safe_haven/infrastructure/programs/imperative_shm.py +++ b/data_safe_haven/infrastructure/programs/imperative_shm.py @@ -1,4 +1,4 @@ -from data_safe_haven.config import Context, SHMConfig, DSHPulumiConfig +from data_safe_haven.config import Context, DSHPulumiConfig, SHMConfig from data_safe_haven.exceptions import ( DataSafeHavenAzureError, DataSafeHavenMicrosoftGraphError, @@ -176,7 +176,7 @@ def teardown(self) -> None: deployed = pulumi_config.project_names if deployed: logger.info(f"Found deployed Pulumi SREs: {deployed}.") - msg = f"Deployed SREs must be torn down before the SHM can be torn down." + msg = "Deployed SREs must be torn down before the SHM can be torn down." raise DataSafeHavenAzureError(msg) try: logger.info( From 754faf28690906726b1879fb0a7a7af782297be9 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 28 Oct 2024 15:34:51 +0000 Subject: [PATCH 03/28] Add confirmation step to SHM teardown --- data_safe_haven/commands/shm.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index 522609b8ff..9fe35692f1 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -142,6 +142,12 @@ def teardown() -> None: try: config = SHMConfig.from_remote(context) shm_infra = ImperativeSHM(context, config) + console.print( + "Tearing down the Safe Haven Management environment will permanently delete all associated resources, including remotely stored configurations." + ) + if not console.confirm("Do you wish to continue?", default_to_yes=True): + logger.info("SHM teardown cancelled by user.") + raise typer.Exit(0) shm_infra.teardown() except DataSafeHavenError as exc: logger.critical("Could not teardown Safe Haven Management environment.") From f2c1fa160b86e9fae20582275cc373f4f70895fb Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 28 Oct 2024 15:35:40 +0000 Subject: [PATCH 04/28] Ask for confirmation before doing anything --- data_safe_haven/commands/shm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index 9fe35692f1..7a56c5c8a5 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -140,14 +140,14 @@ def teardown() -> None: # Teardown Data Safe Haven SHM infrastructure. try: - config = SHMConfig.from_remote(context) - shm_infra = ImperativeSHM(context, config) console.print( "Tearing down the Safe Haven Management environment will permanently delete all associated resources, including remotely stored configurations." ) if not console.confirm("Do you wish to continue?", default_to_yes=True): logger.info("SHM teardown cancelled by user.") raise typer.Exit(0) + config = SHMConfig.from_remote(context) + shm_infra = ImperativeSHM(context, config) shm_infra.teardown() except DataSafeHavenError as exc: logger.critical("Could not teardown Safe Haven Management environment.") From c1e488f589498be06db3a8c7495b2c679eacd725 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 28 Oct 2024 16:10:40 +0000 Subject: [PATCH 05/28] Add confirmation step to SRE teardown --- data_safe_haven/commands/sre.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/data_safe_haven/commands/sre.py b/data_safe_haven/commands/sre.py index f03f0cc53e..c4519d573e 100644 --- a/data_safe_haven/commands/sre.py +++ b/data_safe_haven/commands/sre.py @@ -4,6 +4,7 @@ import typer +from data_safe_haven import console from data_safe_haven.config import ContextManager, DSHPulumiConfig, SHMConfig, SREConfig from data_safe_haven.exceptions import DataSafeHavenConfigError, DataSafeHavenError from data_safe_haven.external import AzureSdk, GraphApi @@ -183,6 +184,7 @@ def teardown( """Tear down a deployed a Secure Research Environment.""" logger = get_logger() try: + # Load context and SHM config context = ContextManager.from_file().assert_context() shm_config = SHMConfig.from_remote(context) @@ -197,6 +199,15 @@ def teardown( pulumi_config = DSHPulumiConfig.from_remote(context) sre_config = SREConfig.from_remote_by_name(context, name) + console.print( + "Tearing down the Secure Research Environment will permanently delete all associated resources, " + "including all data stored in the environment.\n" + "Ensure that any desired outputs have been extracted before continuing." + ) + if not console.confirm("Do you wish to continue tearing down the SRE?", default_to_yes=True): + logger.info("SRE teardown cancelled by user.") + raise typer.Exit(0) + # Check whether current IP address is authorised to take administrator actions if not ip_address_in_list(sre_config.sre.admin_ip_addresses): logger.warning( From 567273687657668eb9fd315d561b87344c9fd421 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 28 Oct 2024 16:11:09 +0000 Subject: [PATCH 06/28] Move SHM teardown confirmation after config retrieval --- data_safe_haven/commands/shm.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index 7a56c5c8a5..b202711989 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -140,14 +140,14 @@ def teardown() -> None: # Teardown Data Safe Haven SHM infrastructure. try: + config = SHMConfig.from_remote(context) + shm_infra = ImperativeSHM(context, config) console.print( "Tearing down the Safe Haven Management environment will permanently delete all associated resources, including remotely stored configurations." ) - if not console.confirm("Do you wish to continue?", default_to_yes=True): + if not console.confirm("Do you wish to continue tearing down the SHM?", default_to_yes=True): logger.info("SHM teardown cancelled by user.") raise typer.Exit(0) - config = SHMConfig.from_remote(context) - shm_infra = ImperativeSHM(context, config) shm_infra.teardown() except DataSafeHavenError as exc: logger.critical("Could not teardown Safe Haven Management environment.") From 59ca7b09b46cad235dd72d2a6e017735e572ad7c Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 28 Oct 2024 16:12:09 +0000 Subject: [PATCH 07/28] Fix linting --- data_safe_haven/commands/shm.py | 4 +++- data_safe_haven/commands/sre.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index b202711989..ff73884db9 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -145,7 +145,9 @@ def teardown() -> None: console.print( "Tearing down the Safe Haven Management environment will permanently delete all associated resources, including remotely stored configurations." ) - if not console.confirm("Do you wish to continue tearing down the SHM?", default_to_yes=True): + if not console.confirm( + "Do you wish to continue tearing down the SHM?", default_to_yes=True + ): logger.info("SHM teardown cancelled by user.") raise typer.Exit(0) shm_infra.teardown() diff --git a/data_safe_haven/commands/sre.py b/data_safe_haven/commands/sre.py index c4519d573e..453794d525 100644 --- a/data_safe_haven/commands/sre.py +++ b/data_safe_haven/commands/sre.py @@ -204,7 +204,9 @@ def teardown( "including all data stored in the environment.\n" "Ensure that any desired outputs have been extracted before continuing." ) - if not console.confirm("Do you wish to continue tearing down the SRE?", default_to_yes=True): + if not console.confirm( + "Do you wish to continue tearing down the SRE?", default_to_yes=True + ): logger.info("SRE teardown cancelled by user.") raise typer.Exit(0) From 20a8722345928cf70d73caad3ce33b4a7ec21ca0 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Mon, 28 Oct 2024 16:17:52 +0000 Subject: [PATCH 08/28] remove superfluous pulumi reference --- data_safe_haven/infrastructure/programs/imperative_shm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data_safe_haven/infrastructure/programs/imperative_shm.py b/data_safe_haven/infrastructure/programs/imperative_shm.py index 7c8eab3f98..5cbb679b73 100644 --- a/data_safe_haven/infrastructure/programs/imperative_shm.py +++ b/data_safe_haven/infrastructure/programs/imperative_shm.py @@ -175,7 +175,7 @@ def teardown(self) -> None: pulumi_config = DSHPulumiConfig.from_remote(self.context) deployed = pulumi_config.project_names if deployed: - logger.info(f"Found deployed Pulumi SREs: {deployed}.") + logger.info(f"Found deployed SREs: {deployed}.") msg = "Deployed SREs must be torn down before the SHM can be torn down." raise DataSafeHavenAzureError(msg) try: From 78033544a9be300317d7aa6730acf94d1c83422d Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 09:57:12 +0000 Subject: [PATCH 09/28] Fix tests for teardown --- tests/commands/test_shm.py | 4 ++++ tests/commands/test_sre.py | 3 +++ 2 files changed, 7 insertions(+) diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index 8258d2a16a..3366ad7cb4 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -1,3 +1,5 @@ +from rich.prompt import Confirm + from data_safe_haven.commands.shm import shm_command_group @@ -41,10 +43,12 @@ def test_infrastructure_auth_failure( class TestTeardownSHM: def test_teardown( self, + mocker, runner, mock_imperative_shm_teardown_then_exit, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 ): + mocker.patch.object(Confirm, "ask", return_value="yes") result = runner.invoke(shm_command_group, ["teardown"]) assert result.exit_code == 1 assert "mock teardown" in result.stdout diff --git a/tests/commands/test_sre.py b/tests/commands/test_sre.py index 6c4a13f545..431f783d85 100644 --- a/tests/commands/test_sre.py +++ b/tests/commands/test_sre.py @@ -1,5 +1,6 @@ from pytest import CaptureFixture, LogCaptureFixture from pytest_mock import MockerFixture +from rich.prompt import Confirm from typer.testing import CliRunner from data_safe_haven.commands.sre import sre_command_group @@ -103,6 +104,7 @@ def test_no_shm( class TestTeardownSRE: def test_teardown( self, + mocker: MockerFixture, runner: CliRunner, mock_graph_api_token, # noqa: ARG002 mock_ip_1_2_3_4, # noqa: ARG002 @@ -111,6 +113,7 @@ def test_teardown( mock_sre_config_from_remote, # noqa: ARG002 mock_sre_project_manager_teardown_then_exit, # noqa: ARG002 ) -> None: + mocker.patch.object(Confirm, "ask", return_value="yes") result = runner.invoke(sre_command_group, ["teardown", "sandbox"]) assert result.exit_code == 1 assert "mock teardown" in result.stdout From becfeaef74fa9403e55c81ffcf43a707976df319 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 12:27:46 +0000 Subject: [PATCH 10/28] use runner input instead of patching confirm --- tests/commands/test_sre.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/commands/test_sre.py b/tests/commands/test_sre.py index 431f783d85..aa74ccd428 100644 --- a/tests/commands/test_sre.py +++ b/tests/commands/test_sre.py @@ -113,8 +113,7 @@ def test_teardown( mock_sre_config_from_remote, # noqa: ARG002 mock_sre_project_manager_teardown_then_exit, # noqa: ARG002 ) -> None: - mocker.patch.object(Confirm, "ask", return_value="yes") - result = runner.invoke(sre_command_group, ["teardown", "sandbox"]) + result = runner.invoke(sre_command_group, ["teardown", "sandbox"], input="y") assert result.exit_code == 1 assert "mock teardown" in result.stdout From 418b7914921967f26a15ec019f134037a8e74378 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 12:28:05 +0000 Subject: [PATCH 11/28] remove unnecessary import --- tests/commands/test_sre.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/commands/test_sre.py b/tests/commands/test_sre.py index aa74ccd428..6f373da351 100644 --- a/tests/commands/test_sre.py +++ b/tests/commands/test_sre.py @@ -1,6 +1,5 @@ from pytest import CaptureFixture, LogCaptureFixture from pytest_mock import MockerFixture -from rich.prompt import Confirm from typer.testing import CliRunner from data_safe_haven.commands.sre import sre_command_group From f124e9c41864b7cc2bb48c335157b642b15077c6 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 12:28:41 +0000 Subject: [PATCH 12/28] Use runner for input and add test for when SREs are deployed --- tests/commands/test_shm.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index 3366ad7cb4..17ac1f178c 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -1,7 +1,5 @@ -from rich.prompt import Confirm - from data_safe_haven.commands.shm import shm_command_group - +from data_safe_haven.config import DSHPulumiConfig class TestDeploySHM: def test_infrastructure_deploy( @@ -48,8 +46,7 @@ def test_teardown( mock_imperative_shm_teardown_then_exit, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 ): - mocker.patch.object(Confirm, "ask", return_value="yes") - result = runner.invoke(shm_command_group, ["teardown"]) + result = runner.invoke(shm_command_group, ["teardown"], input="y") assert result.exit_code == 1 assert "mock teardown" in result.stdout @@ -73,3 +70,15 @@ def test_auth_failure( assert "mock get_credential\n" in result.stdout assert "mock get_credential error" in result.stdout assert "Could not teardown Safe Haven Management environment." in result.stdout + + def test_teardown_sres_exist( + self, + mocker, + runner, + mock_azuresdk_get_subscription_name, # noqa: ARG002 + mock_pulumi_config_from_remote, # noqa: ARG002 + mock_shm_config_from_remote, # noqa: ARG002 + ): + result = runner.invoke(shm_command_group, ["teardown"], input="y") + assert result.exit_code == 1 + assert "Found deployed SREs" in result.stdout \ No newline at end of file From 309f621bf88250e150e6bde1a9c94ab163a0451b Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:19:27 +0000 Subject: [PATCH 13/28] Add test of SRE teardown cancellation --- tests/commands/test_sre.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/commands/test_sre.py b/tests/commands/test_sre.py index 18d6c6daa1..4cee2b1cce 100644 --- a/tests/commands/test_sre.py +++ b/tests/commands/test_sre.py @@ -100,11 +100,9 @@ def test_no_shm( class TestTeardownSRE: def test_teardown( self, - mocker: MockerFixture, runner: CliRunner, mock_ip_1_2_3_4, # noqa: ARG002 mock_pulumi_config_from_remote, # noqa: ARG002 - mock_shm_config_from_remote, # noqa: ARG002 mock_sre_config_from_remote, # noqa: ARG002 mock_sre_project_manager_teardown_then_exit, # noqa: ARG002 ) -> None: @@ -139,3 +137,15 @@ def test_auth_failure( assert result.exit_code == 1 assert "mock get_credential\n" in result.stdout assert "mock get_credential error" in result.stdout + + def test_teardown_cancelled( + self, + runner: CliRunner, + mock_ip_1_2_3_4, # noqa: ARG002 + mock_pulumi_config_from_remote, # noqa: ARG002 + mock_sre_config_from_remote, # noqa: ARG002 + mock_sre_project_manager_teardown_then_exit, # noqa: ARG002 + ) -> None: + result = runner.invoke(sre_command_group, ["teardown", "sandbox"], input="n") + assert result.exit_code == 0 + assert "cancelled by user" in result.stdout \ No newline at end of file From 58c48807e4094509beec7831ffde97bca6d89fd3 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:21:48 +0000 Subject: [PATCH 14/28] remove unnecessary import and mocker --- tests/commands/test_shm.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index 36b62c7524..e78609146a 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -1,5 +1,5 @@ from data_safe_haven.commands.shm import shm_command_group -from data_safe_haven.config import DSHPulumiConfig + class TestDeploySHM: def test_infrastructure_deploy( @@ -72,12 +72,11 @@ def test_auth_failure( def test_teardown_sres_exist( self, - mocker, runner, - mock_azuresdk_get_subscription_name, # noqa: ARG002 + mock_azuresdk_get_subscription_name, # noqa: ARG002 mock_pulumi_config_from_remote, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 ): result = runner.invoke(shm_command_group, ["teardown"], input="y") assert result.exit_code == 1 - assert "Found deployed SREs" in result.stdout \ No newline at end of file + assert "Found deployed SREs" in result.stdout From 597fa1d40cb34463157643863185bd5eb5e2603a Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:21:58 +0000 Subject: [PATCH 15/28] Fix linting --- tests/commands/test_sre.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/commands/test_sre.py b/tests/commands/test_sre.py index 4cee2b1cce..a13518a878 100644 --- a/tests/commands/test_sre.py +++ b/tests/commands/test_sre.py @@ -148,4 +148,4 @@ def test_teardown_cancelled( ) -> None: result = runner.invoke(sre_command_group, ["teardown", "sandbox"], input="n") assert result.exit_code == 0 - assert "cancelled by user" in result.stdout \ No newline at end of file + assert "cancelled by user" in result.stdout From bb89362a5e6ce42435069c605176b3c086e36478 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:22:29 +0000 Subject: [PATCH 16/28] Remove unnecessary mocker --- tests/commands/test_shm.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index e78609146a..38f53cdae5 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -40,7 +40,6 @@ def test_infrastructure_auth_failure( class TestTeardownSHM: def test_teardown( self, - mocker, runner, mock_imperative_shm_teardown_then_exit, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 From 2e9746ad01c15e37da9982b6bc07c755bc0df52c Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:27:03 +0000 Subject: [PATCH 17/28] Print confirmation of cancellation to user rather than log --- data_safe_haven/commands/shm.py | 2 +- data_safe_haven/commands/sre.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index ff73884db9..3dc4e07c12 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -148,7 +148,7 @@ def teardown() -> None: if not console.confirm( "Do you wish to continue tearing down the SHM?", default_to_yes=True ): - logger.info("SHM teardown cancelled by user.") + console.print("SHM teardown cancelled by user.") raise typer.Exit(0) shm_infra.teardown() except DataSafeHavenError as exc: diff --git a/data_safe_haven/commands/sre.py b/data_safe_haven/commands/sre.py index 8e6b3036fd..3d3d29db15 100644 --- a/data_safe_haven/commands/sre.py +++ b/data_safe_haven/commands/sre.py @@ -199,7 +199,7 @@ def teardown( if not console.confirm( "Do you wish to continue tearing down the SRE?", default_to_yes=True ): - logger.info("SRE teardown cancelled by user.") + console.print("SRE teardown cancelled by user.") raise typer.Exit(0) # Check whether current IP address is authorised to take administrator actions From 44ee3f7a409ecb4b102d0ee7b48b3a40dac847e8 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:28:10 +0000 Subject: [PATCH 18/28] Default to false and stopping teardown --- data_safe_haven/commands/shm.py | 2 +- data_safe_haven/commands/sre.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index 3dc4e07c12..87ffe210c7 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -146,7 +146,7 @@ def teardown() -> None: "Tearing down the Safe Haven Management environment will permanently delete all associated resources, including remotely stored configurations." ) if not console.confirm( - "Do you wish to continue tearing down the SHM?", default_to_yes=True + "Do you wish to continue tearing down the SHM?", default_to_yes=False ): console.print("SHM teardown cancelled by user.") raise typer.Exit(0) diff --git a/data_safe_haven/commands/sre.py b/data_safe_haven/commands/sre.py index 3d3d29db15..8c3e0b5cdc 100644 --- a/data_safe_haven/commands/sre.py +++ b/data_safe_haven/commands/sre.py @@ -197,7 +197,7 @@ def teardown( "Ensure that any desired outputs have been extracted before continuing." ) if not console.confirm( - "Do you wish to continue tearing down the SRE?", default_to_yes=True + "Do you wish to continue tearing down the SRE?", default_to_yes=False ): console.print("SRE teardown cancelled by user.") raise typer.Exit(0) From f65b9bcaf46ae673b2e4a92e42b1d91e8861198f Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 13:29:21 +0000 Subject: [PATCH 19/28] Add test for user SHM teardown cancellation --- tests/commands/test_shm.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index 38f53cdae5..0ec3d9a55a 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -79,3 +79,14 @@ def test_teardown_sres_exist( result = runner.invoke(shm_command_group, ["teardown"], input="y") assert result.exit_code == 1 assert "Found deployed SREs" in result.stdout + + def test_teardown_user_cancelled( + self, + runner, + mock_azuresdk_get_subscription_name, # noqa: ARG002 + mock_pulumi_config_from_remote, # noqa: ARG002 + mock_shm_config_from_remote, # noqa: ARG002 + ): + result = runner.invoke(shm_command_group, ["teardown"], input="n") + assert result.exit_code == 0 + assert "cancelled" in result.stdout From 05b6962dd40e525fce487d223f580f3335f469b0 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 15:10:14 +0000 Subject: [PATCH 20/28] add remote_fails pulumi config fixture --- tests/commands/conftest.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/commands/conftest.py b/tests/commands/conftest.py index dc6811ff9f..15f47ba987 100644 --- a/tests/commands/conftest.py +++ b/tests/commands/conftest.py @@ -95,6 +95,15 @@ def mock_pulumi_config_from_remote(mocker, pulumi_config): mocker.patch.object(DSHPulumiConfig, "from_remote", return_value=pulumi_config) +@fixture +def mock_pulumi_config_from_remote_fails(mocker): + mocker.patch.object( + DSHPulumiConfig, + "from_remote", + return_value=DataSafeHavenAzureError("mock from_remote failure"), + ) + + @fixture def mock_pulumi_config_from_remote_or_create(mocker, pulumi_config_empty): mocker.patch.object( From 22d7f3bce5e4bf78e8be9227e2d5dcb5a82aa1ce Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 15:10:27 +0000 Subject: [PATCH 21/28] add test for no pulumi config --- tests/commands/test_shm.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index 0ec3d9a55a..49d24f917f 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -90,3 +90,15 @@ def test_teardown_user_cancelled( result = runner.invoke(shm_command_group, ["teardown"], input="n") assert result.exit_code == 0 assert "cancelled" in result.stdout + + def test_teardown_no_pulumi_config( + self, + runner, + mock_azuresdk_get_subscription_name, # noqa: ARG002 + mock_pulumi_config_from_remote_fails, # noqa: ARG002 + mock_shm_config_from_remote, # noqa: ARG002 + mock_imperative_shm_teardown_then_exit, # noqa: ARG002 + ): + result = runner.invoke(shm_command_group, ["teardown"], input="y") + assert result.exit_code == 1 + assert "mock teardown" in result.stdout From db0448aa41291e0e3438cabb2bf83d85ef4361e7 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Tue, 29 Oct 2024 15:10:46 +0000 Subject: [PATCH 22/28] Catch errors when no pulumi config present --- data_safe_haven/infrastructure/programs/imperative_shm.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/data_safe_haven/infrastructure/programs/imperative_shm.py b/data_safe_haven/infrastructure/programs/imperative_shm.py index c4c0f5e761..023e015789 100644 --- a/data_safe_haven/infrastructure/programs/imperative_shm.py +++ b/data_safe_haven/infrastructure/programs/imperative_shm.py @@ -178,8 +178,12 @@ def teardown(self) -> None: DataSafeHavenAzureError if any resources cannot be destroyed """ logger = get_logger() - pulumi_config = DSHPulumiConfig.from_remote(self.context) - deployed = pulumi_config.project_names + try: + pulumi_config = DSHPulumiConfig.from_remote(self.context) + deployed = pulumi_config.project_names + except DataSafeHavenAzureError: + deployed = None + pass if deployed: logger.info(f"Found deployed SREs: {deployed}.") msg = "Deployed SREs must be torn down before the SHM can be torn down." From daa2015440d74fc41d9bcf1c4ded80d72b82bccd Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 30 Oct 2024 10:46:23 +0000 Subject: [PATCH 23/28] Catch errors when no SHM is deployed --- data_safe_haven/commands/shm.py | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index 87ffe210c7..f8701f1ca5 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -140,17 +140,23 @@ def teardown() -> None: # Teardown Data Safe Haven SHM infrastructure. try: - config = SHMConfig.from_remote(context) - shm_infra = ImperativeSHM(context, config) - console.print( - "Tearing down the Safe Haven Management environment will permanently delete all associated resources, including remotely stored configurations." - ) - if not console.confirm( - "Do you wish to continue tearing down the SHM?", default_to_yes=False - ): - console.print("SHM teardown cancelled by user.") - raise typer.Exit(0) - shm_infra.teardown() + if SHMConfig.remote_exists(context): + config = SHMConfig.from_remote(context) + shm_infra = ImperativeSHM(context, config) + console.print( + "Tearing down the Safe Haven Management environment will permanently delete all associated resources, including remotely stored configurations." + ) + if not console.confirm( + "Do you wish to continue tearing down the SHM?", default_to_yes=False + ): + console.print("SHM teardown cancelled by user.") + raise typer.Exit(0) + shm_infra.teardown() + else: + logger.critical( + f"No deployed SHM found for context [green]{context.name}." + ) + raise typer.Exit(1) except DataSafeHavenError as exc: logger.critical("Could not teardown Safe Haven Management environment.") raise typer.Exit(1) from exc From c4d50b0ac683b38b4899602004f53c9704f5f8c4 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 30 Oct 2024 10:47:20 +0000 Subject: [PATCH 24/28] fix linting --- data_safe_haven/commands/shm.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/data_safe_haven/commands/shm.py b/data_safe_haven/commands/shm.py index f8701f1ca5..c25012d46d 100644 --- a/data_safe_haven/commands/shm.py +++ b/data_safe_haven/commands/shm.py @@ -153,9 +153,7 @@ def teardown() -> None: raise typer.Exit(0) shm_infra.teardown() else: - logger.critical( - f"No deployed SHM found for context [green]{context.name}." - ) + logger.critical(f"No deployed SHM found for context [green]{context.name}.") raise typer.Exit(1) except DataSafeHavenError as exc: logger.critical("Could not teardown Safe Haven Management environment.") From 1096cd5498d7344f33d5694aa1455b6ad08f569a Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 30 Oct 2024 10:47:28 +0000 Subject: [PATCH 25/28] fix tests --- tests/commands/test_shm.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index 49d24f917f..b1b8ec7ab0 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -43,6 +43,7 @@ def test_teardown( runner, mock_imperative_shm_teardown_then_exit, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 + mock_shm_config_remote_exists, # noqa: ARG002 ): result = runner.invoke(shm_command_group, ["teardown"], input="y") assert result.exit_code == 1 @@ -62,6 +63,7 @@ def test_auth_failure( self, runner, mock_azuresdk_get_credential_failure, # noqa: ARG002 + mock_shm_config_remote_exists, # noqa: ARG002 ): result = runner.invoke(shm_command_group, ["teardown"]) assert result.exit_code == 1 @@ -75,6 +77,7 @@ def test_teardown_sres_exist( mock_azuresdk_get_subscription_name, # noqa: ARG002 mock_pulumi_config_from_remote, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 + mock_shm_config_remote_exists, # noqa: ARG002 ): result = runner.invoke(shm_command_group, ["teardown"], input="y") assert result.exit_code == 1 @@ -86,6 +89,7 @@ def test_teardown_user_cancelled( mock_azuresdk_get_subscription_name, # noqa: ARG002 mock_pulumi_config_from_remote, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 + mock_shm_config_remote_exists, # noqa: ARG002 ): result = runner.invoke(shm_command_group, ["teardown"], input="n") assert result.exit_code == 0 @@ -98,6 +102,7 @@ def test_teardown_no_pulumi_config( mock_pulumi_config_from_remote_fails, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 mock_imperative_shm_teardown_then_exit, # noqa: ARG002 + mock_shm_config_remote_exists, # noqa: ARG002 ): result = runner.invoke(shm_command_group, ["teardown"], input="y") assert result.exit_code == 1 From 6111140a57954fcaefd7542ffda2d508e805bb9d Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 30 Oct 2024 12:00:38 +0000 Subject: [PATCH 26/28] Check if pulumi config exists rather than catch errors --- .../infrastructure/programs/imperative_shm.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/data_safe_haven/infrastructure/programs/imperative_shm.py b/data_safe_haven/infrastructure/programs/imperative_shm.py index 023e015789..f233c7c7fe 100644 --- a/data_safe_haven/infrastructure/programs/imperative_shm.py +++ b/data_safe_haven/infrastructure/programs/imperative_shm.py @@ -178,16 +178,13 @@ def teardown(self) -> None: DataSafeHavenAzureError if any resources cannot be destroyed """ logger = get_logger() - try: + if DSHPulumiConfig.remote_exists(self.context): pulumi_config = DSHPulumiConfig.from_remote(self.context) deployed = pulumi_config.project_names - except DataSafeHavenAzureError: - deployed = None - pass - if deployed: - logger.info(f"Found deployed SREs: {deployed}.") - msg = "Deployed SREs must be torn down before the SHM can be torn down." - raise DataSafeHavenAzureError(msg) + if deployed: + logger.info(f"Found deployed SREs: {deployed}.") + msg = "Deployed SREs must be torn down before the SHM can be torn down." + raise DataSafeHavenAzureError(msg) try: logger.info( f"Removing [green]{self.context.description}[/] resource group {self.context.resource_group_name}." From 29fa7f5fea9b9fbc1fdc7a7fa56d56db319f5228 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 30 Oct 2024 13:38:49 +0000 Subject: [PATCH 27/28] add pulumi config remote_exists fixture --- tests/commands/conftest.py | 3 +++ tests/commands/test_shm.py | 1 + 2 files changed, 4 insertions(+) diff --git a/tests/commands/conftest.py b/tests/commands/conftest.py index 15f47ba987..8630cf193f 100644 --- a/tests/commands/conftest.py +++ b/tests/commands/conftest.py @@ -122,6 +122,9 @@ def mock_pulumi_config_no_key_from_remote(mocker, pulumi_config_no_key): def mock_pulumi_config_upload(mocker): mocker.patch.object(DSHPulumiConfig, "upload", return_value=None) +@fixture +def mock_pulumi_config_remote_exists(mocker): + mocker.patch.object(DSHPulumiConfig, "remote_exists", return_value=True) @fixture def mock_shm_config_from_remote(mocker, shm_config): diff --git a/tests/commands/test_shm.py b/tests/commands/test_shm.py index b1b8ec7ab0..e8f3919ed9 100644 --- a/tests/commands/test_shm.py +++ b/tests/commands/test_shm.py @@ -76,6 +76,7 @@ def test_teardown_sres_exist( runner, mock_azuresdk_get_subscription_name, # noqa: ARG002 mock_pulumi_config_from_remote, # noqa: ARG002 + mock_pulumi_config_remote_exists, # noqa: ARG002 mock_shm_config_from_remote, # noqa: ARG002 mock_shm_config_remote_exists, # noqa: ARG002 ): From 938bc78c2cfa2cb5c59ddbed690c87b1e70173a5 Mon Sep 17 00:00:00 2001 From: Matt Craddock <5796417+craddm@users.noreply.github.com> Date: Wed, 30 Oct 2024 13:42:44 +0000 Subject: [PATCH 28/28] fix linting --- tests/commands/conftest.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/commands/conftest.py b/tests/commands/conftest.py index 8630cf193f..d675398bfc 100644 --- a/tests/commands/conftest.py +++ b/tests/commands/conftest.py @@ -122,10 +122,12 @@ def mock_pulumi_config_no_key_from_remote(mocker, pulumi_config_no_key): def mock_pulumi_config_upload(mocker): mocker.patch.object(DSHPulumiConfig, "upload", return_value=None) + @fixture def mock_pulumi_config_remote_exists(mocker): mocker.patch.object(DSHPulumiConfig, "remote_exists", return_value=True) + @fixture def mock_shm_config_from_remote(mocker, shm_config): mocker.patch.object(SHMConfig, "from_remote", return_value=shm_config)