[RPD-242] Consolidate matcha destroy and matcha destroy full into one command (#141)

* Consolidate destroy and destroy full

* Remove tests that no longer describe the expected functionality

* RPD-242 fixes to remove destroy full

* RPD-242 added downloading matcha.state

* RPD-242 fix for missing _state

* RPD-242 fixing tests

* RPD-242 updated docstring

---------

Co-authored-by: Jonathan Carlton <jonathan.carlton@hotmail.com>

Author: Christopher-Norman

src/matcha_ml/cli/cli.py

@@ -20,7 +20,6 @@
     build_resource_output,
     hide_sensitive_in_output,
 )
-from matcha_ml.cli.ui.status_message_builders import build_warning_status
 from matcha_ml.core import core
 from matcha_ml.errors import MatchaError, MatchaInputError
 from matcha_ml.services.analytics_service import AnalyticsEvent, track
@@ -105,24 +104,11 @@ def get(
 
 @app.command()
 @track(event_name=AnalyticsEvent.DESTROY)
-def destroy(
-    full: Optional[str] = typer.Argument(
-        default=None,
-        help="When the full command is specified, the resource group and all its provisioned resources will be destroyed together.",
-    ),
-) -> None:
+def destroy() -> None:
     """Destroy the provisioned cloud resources."""
     remote_state_manager = RemoteStateManager()
-    if full:
-        destroy_resources(resources=STATE_RESOURCE_MSG + RESOURCE_MSG)
-        remote_state_manager.deprovision_remote_state()
-    else:
-        print_status(
-            build_warning_status(
-                "Warning: a storage container holding Matcha state information will persist. To destroy ALL cloud resources, run 'matcha destroy full'."
-            )
-        )
-        destroy_resources(resources=RESOURCE_MSG)
+    destroy_resources(resources=STATE_RESOURCE_MSG + RESOURCE_MSG)
+    remote_state_manager.deprovision_remote_state()
 
 
 @app.command()

src/matcha_ml/cli/destroy.py

@@ -27,7 +27,7 @@ def destroy_resources(resources: List[Tuple[str, str]]) -> None:
     remote_state = RemoteStateManager()
     if not remote_state.is_state_provisioned():
         print_error(
-            "Error - matcha state has not been initialized, nothing to destroy."
+            "Error - resources that have not been provisioned cannot be destroy. Run 'matcha provision' to get started!"
         )
         raise typer.Exit()
 

src/matcha_ml/cli/provision.py

@@ -77,29 +77,21 @@ def provision_resources(
         typer.Exit: if approval for removing a stale state is not given by user.
     """
     remote_state_manager = RemoteStateManager()
+    is_provisioned = remote_state_manager.is_state_provisioned()
 
-    if remote_state_manager.is_state_stale():
+    if is_provisioned:
         print_status(
             build_warning_status(
-                "Matcha has detected a stale state file. This means that your local configuration is out of sync with the remote state, the resource group may have been removed."
+                "WARNING - Matcha has detected that there are resources already provisioned. Use 'matcha destroy' to remove the existing resources before trying to provision again."
             )
         )
+        raise typer.Exit()
 
-        if not typer.confirm(
-            "Do you want to remove the existing local config and continue?"
-        ):
-            raise typer.Exit()
-
-        remote_state_manager.remove_matcha_config()
-        # Re-initialise remote state manager with empty state file
-        remote_state_manager = RemoteStateManager()
-
-    # Check whether remote state storage has been provisioned
-    if not remote_state_manager.is_state_provisioned():
+    if not is_provisioned:
         location, prefix, _ = fill_provision_variables(
             location=location, prefix=prefix, password="temp"
         )
-        # Provision a state storage if it's not provisioned
+
         remote_state_manager.provision_remote_state(location, prefix)
 
     with remote_state_manager.use_lock(), remote_state_manager.use_remote_state():
@@ -117,15 +109,14 @@ def provision_resources(
         # Create a azure template object
         azure_template = AzureTemplate()
 
-        if not azure_template.reuse_configuration(destination):
-            location, prefix, password = fill_provision_variables(
-                location, prefix, password
-            )
+        location, prefix, password = fill_provision_variables(
+            location, prefix, password
+        )
 
-            config = azure_template.build_template_configuration(
-                location=location, prefix=prefix, password=password
-            )
-            azure_template.build_template(config, template, destination, verbose)
+        config = azure_template.build_template_configuration(
+            location=location, prefix=prefix, password=password
+        )
+        azure_template.build_template(config, template, destination, verbose)
 
         # Initializes the infrastructure provisioning process.
         if template_runner.is_approved(verb="provision", resources=RESOURCE_MSG):

src/matcha_ml/core/core.py

@@ -37,9 +37,11 @@ def get(
         )
 
     if not matcha_state_service.state_file_exists:
-        raise MatchaError(
-            f"Error - matcha.state file does not exist at {os.path.join(os.getcwd(), '.matcha', 'infrastructure')} . Please run 'matcha provision' before trying to get the resource."
-        )
+        # if the state file doesn't exist, then download it from the remote
+        remote_state.download(os.getcwd())
+
+        # reinitialise to create the necessary variables
+        matcha_state_service = MatchaStateService()
 
     with remote_state.use_lock():
         local_hash = matcha_state_service.get_hash_local_state()
@@ -50,6 +52,8 @@ def get(
         if local_hash != remote_hash:
             remote_state.download(os.getcwd())
 
+            matcha_state_service = MatchaStateService()
+
         if resource_name:
             get_command_validation(
                 resource_name,

src/matcha_ml/runners/azure_runner.py

@@ -150,6 +150,6 @@ def deprovision(self) -> None:
         """Destroy the provisioned resources."""
         self._check_matcha_directory_exists()
         self._check_terraform_installation()
-        self._initialize_terraform(msg="Matcha")
+        self._initialize_terraform(msg="Matcha", destroy=True)
         self._destroy_terraform(msg="Matcha")
         self._write_outputs_state_cloud_only()

src/matcha_ml/runners/base_runner.py

@@ -71,20 +71,22 @@ def _validate_kubeconfig(self, base_path: str = ".kube/config") -> None:
         """
         self.tfs.verify_kubectl_config_file(base_path)
 
-    def _initialize_terraform(self, msg: str = "") -> None:
+    def _initialize_terraform(self, msg: str = "", destroy: bool = False) -> None:
         """Run terraform init to initialize Terraform .
 
         Raises:
             MatchaTerraformError: if 'terraform init' failed.
             msg (str) : Message to display. Default is empty string.
+            destroy (bool): whether this function is being called in a destructive context
         """
         if self.tf_state_dir.exists():
-            # this directory gets created after a successful init command
-            print_status(
-                build_status(
-                    f"matcha {Emojis.MATCHA.value} has already been initialized. Skipping this step..."
+            if not destroy:
+                # this directory gets created after a successful init command
+                print_status(
+                    build_status(
+                        f"matcha {Emojis.MATCHA.value} has already been initialized. Skipping this step..."
+                    )
                 )
-            )
 
         else:
             print_status(

src/matcha_ml/state/remote_state_manager.py

@@ -21,7 +21,7 @@
 DEFAULT_CONFIG_NAME = "matcha.config.json"
 ALREADY_LOCKED_MESSAGE = (
     "Remote state is already locked, maybe someone else is using matcha?"
-    "If you think this is a mistake, you can unlock the state by running 'matcha force-unlock'."
+    " If you think this is a mistake, you can unlock the state by running 'matcha force-unlock'."
 )
 
 

src/matcha_ml/templates/azure_template.py

@@ -3,11 +3,6 @@
 import os
 from typing import Optional
 
-import typer
-
-from matcha_ml.cli._validation import check_current_deployment_exists
-from matcha_ml.cli.ui.print_messages import print_status
-from matcha_ml.state import MatchaStateService
 from matcha_ml.templates.base_template import BaseTemplate, TemplateVariables
 
 SUBMODULE_NAMES = [
@@ -39,35 +34,6 @@ def __init__(self) -> None:
         """
         super().__init__(SUBMODULE_NAMES)
 
-    def reuse_configuration(self, path: str) -> bool:
-        """Check if a configuration already exists, and prompt user to override or reuse it.
-
-        Args:
-            path (str): path to the infrastructure configuration
-
-        Returns:
-            bool: decision to reuse the existing configuration
-        """
-        if os.path.exists(path):
-            if check_current_deployment_exists():
-                matcha_state_service = MatchaStateService()
-                resource_group_name = (
-                    matcha_state_service.fetch_resources_from_state_file(
-                        "cloud", "prefix"
-                    )["cloud"]["prefix"]
-                )
-                warning_msg = f"\nWARNING: Matcha has detected that a deployment already exists in Azure with the resource group name '{resource_group_name}'. Use 'matcha destroy' to remove these resources before trying to provision."
-                confirmation_msg = "\nIf you continue, you will create a orphan resource. You should destroy the resources before proceeding.\n\nDo you want to override the existing configuration?"
-            else:
-                warning_msg = "\nMatcha has detected that the you already have resources configured for provisioning."
-                confirmation_msg = "\nIf you choose to override the existing configuration, the existing configuration will be deleted. Otherwise, the configuration will be reused.\n\nDo you want to override the existing configuration?"
-
-            print_status(warning_msg)
-
-            return not typer.confirm(confirmation_msg)
-        else:
-            return False
-
     def build_template(
         self,
         config: TemplateVariables,

tests/test_cli/test_destroy.py

@@ -1,25 +1,22 @@
 """Test suite to test the destroy command and all its subcommands."""
-import json
 import os
 from typing import Iterable
 from unittest.mock import MagicMock, patch
 
 import pytest
 
 from matcha_ml.cli.cli import app
-from matcha_ml.services.terraform_service import TerraformConfig
 
 
 @pytest.fixture(autouse=True)
-def mock_provisioned_remote_state() -> Iterable[MagicMock]:
+def mock_remote_state_manager() -> Iterable[MagicMock]:
     """Mock remote state manager to have state provisioned.
 
-    Returns:
+    Yields:
         MagicMock: mock of an RemoteStateManager instance
     """
     with patch("matcha_ml.cli.destroy.RemoteStateManager") as mock_state_manager_class:
         mock_state_manager = mock_state_manager_class.return_value
-        mock_state_manager.is_state_provisioned.return_value = True
         yield mock_state_manager
 
 
@@ -40,90 +37,43 @@ def test_cli_destroy_command_help(runner):
 
 
 def test_cli_destroy_command_with_no_provisioned_resources(
-    runner, matcha_testing_directory, mock_provisioned_remote_state: MagicMock
+    runner, matcha_testing_directory, mock_remote_state_manager: MagicMock
 ):
-    """Test provision command when there no existing resources deployed.
+    """Test destroy command when there no existing resources deployed.
 
     Args:
         runner (CliRunner): typer CLI runner
         matcha_testing_directory (str): temporary working directory.
-        mock_provisioned_remote_state (MagicMock): mock of an RemoteStateManager instance
+        mock_remote_state_manager (MagicMock): mock of an RemoteStateManager instance
     """
     os.chdir(matcha_testing_directory)
 
-    # Invoke destroy command
-    with patch(
-        "matcha_ml.templates.azure_template.check_current_deployment_exists"
-    ) as check_deployment_exists:
-        check_deployment_exists.return_value = False
-        result = runner.invoke(app, ["destroy"])
+    mock_remote_state_manager.is_state_provisioned.return_value = True
+
+    result = runner.invoke(app, ["destroy"])
 
     assert (
         "Error - you cannot destroy resources that have not been provisioned yet."
         in result.stdout
     )
 
-    mock_provisioned_remote_state.use_lock.assert_called_once()
+    mock_remote_state_manager.use_lock.assert_called_once()
 
 
-def test_cli_destroy_command_updates_matcha_state(runner, matcha_testing_directory):
-    """Test provision command updates the matcha state file when resources (excluding resource group and state bucket) are destroyed.
+def test_cli_destroy_with_nothing_provisioned(
+    runner, mock_remote_state_manager: MagicMock
+):
+    """Test the destroy command with no resources exist at all.
 
     Args:
-        runner (CliRunner): typer CLI runner
-        matcha_testing_directory (str): temporary working directory.
+        runner (CliRunner): typer CLI runner.
+        mock_remote_state_manager (MagicMock): mock of a RemoteStateManager instance.
     """
-    os.chdir(matcha_testing_directory)
+    mock_remote_state_manager.is_state_provisioned.return_value = False
 
-    matcha_infrastructure_dir = os.path.join(".matcha", "infrastructure", "resources")
-    matcha_state_file_path = os.path.join(".matcha", "infrastructure", "matcha.state")
-
-    os.makedirs(matcha_infrastructure_dir)
-
-    state_file_resources = {
-        "cloud": {
-            "prefix": "test",
-            "location": "ukwest",
-            "flavor": "azure",
-            "resource-group-name": "test_resources",
-        },
-        "id": {"matcha_uuid": "TestStateID"},
-        "container-registry": {
-            "flavor": "azure",
-            "registry-name": "azure_registry_name",
-            "registry-url": "azure_container_registry",
-        },
-        "experiment-tracker": {"flavor": "mlflow", "tracking-url": "mlflow_test_url"},
-    }
-
-    with open(matcha_state_file_path, "w") as f:
-        json.dump(state_file_resources, f)
-
-    expected_matcha_state_contents = {
-        "cloud": {
-            "prefix": "test",
-            "location": "ukwest",
-            "flavor": "azure",
-            "resource-group-name": "test_resources",
-        },
-        "id": {"matcha_uuid": "TestStateID"},
-    }
+    result = runner.invoke(app, ["destroy"])
 
-    # Invoke destroy command
-    with patch(
-        "matcha_ml.cli.destroy.check_current_deployment_exists"
-    ) as check_deployment_exists, patch.object(
-        TerraformConfig, "state_file", matcha_state_file_path
-    ):
-        # mock_az_runner.return_value.state_file = matcha_state_file_path
-        check_deployment_exists.return_value = True
-        runner.invoke(
-            app,
-            ["destroy"],
-            input="Y\n",
-        )
-
-    with open(matcha_state_file_path) as f:
-        matcha_state_data = json.load(f)
-
-    assert matcha_state_data == expected_matcha_state_contents
+    assert (
+        "Error - resources that have not been provisioned cannot be destroy."
+        in result.stdout
+    )

tests/test_cli/test_get.py

@@ -136,27 +136,6 @@ def test_cli_get_command_help(runner: CliRunner):
     assert "Get information for the provisioned resources." in result.stdout
 
 
-def test_cli_get_command_with_no_state_file(
-    runner: CliRunner, mock_provisioned_remote_state: MagicMock
-):
-    """Test cli for get command when the state file does not exist.
-
-    Args:
-        runner (CliRunner): typer CLI runner
-        mock_provisioned_remote_state (MagicMock): mock of an RemoteStateManager instance
-    """
-    state_file_path = os.path.join(".matcha", "infrastructure", "matcha.state")
-    os.remove(state_file_path)
-
-    # Invoke get command
-    result = runner.invoke(app, ["get"])
-
-    assert result.exit_code == 0
-    assert "Error - matcha.state file does not exist at" in str(result.stdout)
-
-    mock_provisioned_remote_state.use_lock.assert_not_called()
-
-
 def test_cli_get_command_hide_sensitive(
     runner: CliRunner,
     expected_output_lines: List[str],