Repository: charmed-kubernetes/pytest-operator (language: python)

Issue:
Teardown storage
Hello!

I'm running some tests on an Openstack cloud and I've noticed that the `ops_test` fixture does not teardown the storage created by Juju on the model. 

AFAIK the `destroy_storage` parameter on the `OpsTest.forget_model` method could be the solution, but it is not currently exposed to the user. I couldn't come up with a nice API for exposing this feature in this library (maybe another fixture?). I'd be willing to implement this functionality, but I'm not sure how to expose it to the users. 


Discussion / hints:
Maybe adding a command-line parameter such as `--destroy-storage` will be the best way to expose this feature.

Fix patch:
diff --git a/docs/reference.md b/docs/reference.md
index e4dc990..b586d7e 100644
--- a/docs/reference.md
+++ b/docs/reference.md
@@ -28,6 +28,10 @@ on the specified controller.
 
 Keep any automatically created models.
 
+### `--destroy-storage`
+
+Destroy storage allocated on the created models.
+
 ### `--model-config`
 
 Path to a yaml file which will be applied to the model on creation.
@@ -368,7 +372,7 @@ applied if the marked test method fails or errors.
 Log a summary of the status of the model. This is automatically called before the model
 is cleaned up.
 
-#### `async def track_model(self, alias: str, model_name: Optional[str] = None, cloud_name: Optional[str] = None, use_existing: Optional[bool] = None, keep: Optional[ModelKeep, str, bool, None] = ModelKeep.IF_EXISTS, **kwargs,) -> Model`
+#### `async def track_model(self, alias: str, model_name: Optional[str] = None, cloud_name: Optional[str] = None, use_existing: Optional[bool] = None, keep: Optional[ModelKeep, str, bool, None] = ModelKeep.IF_EXISTS, destroy_storage: bool = False, **kwargs,) -> Model`
 
 Indicate to `ops_test` to track a new model which is automatically created in juju or an existing juju model referenced by model_name.
 This allows `ops_test` to track multiple models on various clouds by a unique alias name.
@@ -391,6 +395,7 @@ This allows `ops_test` to track multiple models on various clouds by a unique al
   * `None` (default): inherit boolean value of `use_existing`
   * `False`: `ops_test` will destroy at the end of testing
   * `True`: `ops_test` won't destroy at the end of testing
+* `destroy_storage`: Wether the storage should be destroyed with the model, defaults to `False`.
 
 ##### Examples:
 


Test patch:
diff --git a/pytest_operator/plugin.py b/pytest_operator/plugin.py
index a11e674..311bcdc 100644
--- a/pytest_operator/plugin.py
+++ b/pytest_operator/plugin.py
@@ -9,14 +9,14 @@
 import logging
 import os
 import re
-import shutil
 import shlex
+import shutil
 import subprocess
 import sys
 import textwrap
 from collections import OrderedDict
-from functools import cached_property
 from fnmatch import fnmatch
+from functools import cached_property
 from pathlib import Path
 from random import choices
 from string import ascii_lowercase, digits, hexdigits
@@ -26,16 +26,16 @@
     Iterable,
     List,
     Literal,
-    MutableMapping,
     Mapping,
+    MutableMapping,
     Optional,
-    TypeVar,
     Tuple,
+    TypeVar,
     Union,
 )
-from urllib.request import urlretrieve, urlopen
-from urllib.parse import urlencode
 from urllib.error import HTTPError
+from urllib.parse import urlencode
+from urllib.request import urlopen, urlretrieve
 from zipfile import Path as ZipPath
 
 import jinja2
@@ -45,13 +45,13 @@
 import yaml
 from _pytest.config import Config
 from _pytest.config.argparsing import Parser
-from kubernetes import client as k8s_client
-from kubernetes.client import Configuration as K8sConfiguration
 from juju.client import client
 from juju.client.jujudata import FileJujuData
-from juju.exceptions import DeadEntityException
 from juju.errors import JujuError
-from juju.model import Model, Controller, websockets
+from juju.exceptions import DeadEntityException
+from juju.model import Controller, Model, websockets
+from kubernetes import client as k8s_client
+from kubernetes.client import Configuration as K8sConfiguration
 
 log = logging.getLogger(__name__)
 
@@ -86,6 +86,12 @@ def pytest_addoption(parser: Parser):
         action="store_true",
         help="Keep models handled by opstest, can be overriden in track_model",
     )
+    parser.addoption(
+        "--destroy-storage",
+        action="store_true",
+        help="Destroy storage created in models handled by opstest,"
+        "can be overriden in track_model",
+    )
     parser.addoption(
         "--destructive-mode",
         action="store_true",
@@ -412,6 +418,7 @@ class ModelInUseError(Exception):
 class ModelState:
     model: Model
     keep: bool
+    destroy_storage: bool
     controller_name: str
     cloud_name: Optional[str]
     model_name: str
@@ -502,6 +509,7 @@ def __init__(self, request, tmp_path_factory):
         self._init_cloud_name: Optional[str] = request.config.option.cloud
         self._init_model_name: Optional[str] = request.config.option.model
         self._init_keep_model: bool = request.config.option.keep_models
+        self._init_destroy_storage: bool = request.config.option.destroy_storage
 
         # These may be modified by _setup_model
         self.controller_name = request.config.option.controller
@@ -611,6 +619,18 @@ def keep_model(self) -> bool:
         current_state = self.current_alias and self._models.get(self.current_alias)
         return current_state.keep if current_state else self._init_keep_model
 
+    @property
+    def destroy_storage(self) -> bool:
+        """
+        Represents whether the current model storage should be destroyed after tests.
+        """
+        current_state = self.current_alias and self._models.get(self.current_alias)
+        return (
+            current_state.destroy_storage
+            if current_state
+            else self._init_destroy_storage
+        )
+
     def _generate_name(self, kind: str) -> str:
         module_name = self.request.module.__name__.rpartition(".")[-1]
         suffix = "".join(choices(ascii_lowercase + digits, k=4))
@@ -679,7 +699,9 @@ async def juju(self, *args, **kwargs):
 
         return await self.run("juju", *args, **kwargs)
 
-    async def _add_model(self, cloud_name, model_name, keep=False, **kwargs):
+    async def _add_model(
+        self, cloud_name, model_name, keep=False, destroy_storage=False, **kwargs
+    ):
         """
         Creates a model used by the test framework which would normally be destroyed
         after the tests are run in the module.
@@ -711,7 +733,13 @@ async def _add_model(self, cloud_name, model_name, keep=False, **kwargs):
         # update the cache from the controller.
         await self.juju("models")
         state = ModelState(
-            model, keep, controller_name, cloud_name, model_name, timeout=timeout
+            model,
+            keep,
+            destroy_storage,
+            controller_name,
+            cloud_name,
+            model_name,
+            timeout=timeout,
         )
         state.config = await model.get_config()
         return state
@@ -724,13 +752,17 @@ async def _model_exists(self, model_name: str) -> bool:
         return model_name in all_models
 
     @staticmethod
-    async def _connect_to_model(controller_name, model_name, keep=True):
+    async def _connect_to_model(
+        controller_name, model_name, keep=True, destroy_storage=False
+    ):
         """
         Makes a reference to an existing model used by the test framework
         which will not be destroyed after the tests are run in the module.
         """
         model = Model()
-        state = ModelState(model, keep, controller_name, None, model_name)
+        state = ModelState(
+            model, keep, destroy_storage, controller_name, None, model_name
+        )
         log.info(
             "Connecting to existing model %s on unspecified cloud", state.full_name
         )
@@ -771,6 +803,7 @@ async def _setup_model(self):
             model_name=self._init_model_name or self.default_model_name,
             cloud_name=self._init_cloud_name,
             keep=self._init_model_name is not None,
+            destroy_storage=self._init_destroy_storage,
             config=self.read_model_config(self._init_model_config),
         )
 
@@ -782,6 +815,7 @@ async def track_model(
         model_name: Optional[str] = None,
         cloud_name: Optional[str] = None,
         use_existing: Optional[bool] = None,
+        destroy_storage: Optional[bool] = None,
         keep: Union[ModelKeep, str, bool, None] = ModelKeep.IF_EXISTS,
         **kwargs,
     ) -> Model:
@@ -794,6 +828,9 @@ async def track_model(
                                          None will craft a unique name
         @param Optional[str] cloud_name: cloud name in which to add a new model,
                                          None will use current cloud
+        @param Optional[bool] destroy_storage: wether the storage should be destroyed
+                                               with the model, None defaults to the
+                                               pytest config flag `--destroy-storage`
         @param Optional[bool] use_existing:
                None:  True if model_name exists on this controller
                False: create a new model and keep=False, unless keep=True explicitly set
@@ -836,6 +873,10 @@ async def track_model(
         elif keep is None:
             keep_val = self._init_keep_model or bool(use_existing)
 
+        destroy_storage_val = (
+            self._init_destroy_storage if destroy_storage is None else destroy_storage
+        )
+
         if use_existing:
             if not model_name:
                 raise NotImplementedError(
@@ -848,7 +889,7 @@ async def track_model(
             cloud_name = cloud_name or self.cloud_name
             model_name = model_name or self._generate_name(kind="model")
             model_state = await self._add_model(
-                cloud_name, model_name, keep_val, **kwargs
+                cloud_name, model_name, keep_val, destroy_storage_val, **kwargs
             )
         self._models[alias] = model_state
         if ops_cloud := self._clouds.get(cloud_name):
@@ -889,8 +930,8 @@ async def forget_model(
         self,
         alias: Optional[str] = None,
         timeout: Optional[Timeout] = None,
+        destroy_storage: Optional[bool] = None,
         allow_failure: bool = True,
-        destroy_storage: bool = False,
     ):
         """
         Forget a model and wait for it to be removed from the controller.
@@ -930,6 +971,9 @@ async def is_model_alive():
 
             if not self.keep_model:
                 await self._reset(model, allow_failure, timeout=timeout)
+                destroy_storage = (
+                    self.destroy_storage if destroy_storage is None else destroy_storage
+                )
                 await self._controller.destroy_model(
                     model_name,
                     force=True,
diff --git a/tests/unit/test_pytest_operator.py b/tests/unit/test_pytest_operator.py
index 9182058..971f8a8 100644
--- a/tests/unit/test_pytest_operator.py
+++ b/tests/unit/test_pytest_operator.py
@@ -382,7 +382,7 @@ async def test_crash_dump_mode(
     ops_test._current_alias = "main"
     ops_test._models = {
         ops_test.current_alias: plugin.ModelState(
-            model, keep_models, "test", "local", "model"
+            model, keep_models, False, "test", "local", "model"
         )
     }
     ops_test.crash_dump_output = None
@@ -503,6 +503,7 @@ def setup_request(request, mock_juju):
     mock_request.config.option.model_alias = "main"
     mock_request.config.option.model_config = None
     mock_request.config.option.keep_models = False
+    mock_request.config.option.destroy_storage = False
     yield mock_request
 
 
@@ -521,6 +522,9 @@ async def test_fixture_set_up_existing_model(
     assert ops_test.cloud_name is None
     assert ops_test.model_name == "this-model"
     assert ops_test.keep_model is True, "Model should be kept if it already exists"
+    assert (
+        ops_test.destroy_storage is False
+    ), "Storage should not be destroyed by default"
     assert len(ops_test.models) == 1
 
 
@@ -582,6 +586,37 @@ async def test_model_keep_options(
         ), f"{ops_test.model_full_name} should follow configured keep"
 
 
+@patch("pytest_operator.plugin.OpsTest.forget_model", AsyncMock())
+@patch("pytest_operator.plugin.OpsTest.run", AsyncMock())
+@pytest.mark.parametrize(
+    "global_flag, destroy_storage, expected",
+    [
+        (False, None, False),
+        (False, True, True),
+        (False, False, False),
+        (True, None, True),
+        (True, True, True),
+        (True, False, False),
+    ],
+)
+async def test_destroy_storage_options(
+    global_flag, destroy_storage, expected, setup_request, tmp_path_factory
+):
+    setup_request.config.option.destroy_storage = global_flag
+    ops_test = plugin.OpsTest(setup_request, tmp_path_factory)
+    await ops_test._setup_model()
+    with ops_test.model_context("main"):
+        assert ops_test.destroy_storage is bool(
+            global_flag
+        ), "main model should follow global flag"
+
+    await ops_test.track_model("secondary", destroy_storage=destroy_storage)
+    with ops_test.model_context("secondary"):
+        assert (
+            ops_test.destroy_storage is expected
+        ), f"{ops_test.model_full_name} should follow configured destroy_storage"
+
+
 @patch("pytest_operator.plugin.OpsTest.default_model_name", new_callable=PropertyMock)
 @patch("pytest_operator.plugin.OpsTest.juju", autospec=True)
 async def test_fixture_set_up_automatic_model(