Merge branch '7.0' into context-is-manager

README.md

@@ -488,15 +488,15 @@ remote_unit_2_is_joining_event = ctx.on.relation_joined(relation, remote_unit=2)
 Simplifying a bit the Juju "spaces" model, each integration endpoint a charm defines in its metadata is associated with a network. Regardless of whether there is a living relation over that endpoint, that is.  
 
 If your charm has a relation `"foo"` (defined in its metadata), then the charm will be able at runtime to do `self.model.get_binding("foo").network`.
-The network you'll get by doing so is heavily defaulted (see `state.Network.default`) and good for most use-cases because the charm should typically not be concerned about what IP it gets. 
+The network you'll get by doing so is heavily defaulted (see `state.Network`) and good for most use-cases because the charm should typically not be concerned about what IP it gets. 
 
 On top of the relation-provided network bindings, a charm can also define some `extra-bindings` in its metadata and access them at runtime. Note that this is a deprecated feature that should not be relied upon. For completeness, we support it in Scenario.
 
 If you want to, you can override any of these relation or extra-binding associated networks with a custom one by passing it to `State.networks`.
 
 ```python
 state = scenario.State(networks={
-  scenario.Network.default("foo", private_address='192.0.2.1')
+  scenario.Network("foo", [BindAddress([Address('192.0.2.1')])])
 })
 ```
 
@@ -804,22 +804,27 @@ Scenario has secrets. Here's how you use them.
 state = scenario.State(
     secrets={
         scenario.Secret(
-            {0: {'key': 'public'}},
-            id='foo',
-        ),
-    },
+            tracked_content={'key': 'public'},
+            latest_content={'key': 'public', 'cert': 'private'},
+        )
+    }
 )
 ```
 
-The only mandatory arguments to Secret are its secret ID (which should be unique) and its 'contents': that is, a mapping
-from revision numbers (integers) to a `str:str` dict representing the payload of the revision.
+The only mandatory arguments to Secret is the `tracked_content` dict: a `str:str`
+mapping representing the content of the revision. If there is a newer revision
+of the content than the one the unit that's handling the event is tracking, then
+`latest_content` should also be provided - if it's not, then Scenario assumes
+that `latest_content` is the `tracked_content`. If there are other revisions of
+the content, simply don't include them: the unit has no way of knowing about
+these.
 
 There are three cases:
 - the secret is owned by this app but not this unit, in which case this charm can only manage it if we are the leader
 - the secret is owned by this unit, in which case this charm can always manage it (leader or not)
-- (default) the secret is not owned by this app nor unit, which means we can't manage it but only view it
+- (default) the secret is not owned by this app nor unit, which means we can't manage it but only view it (this includes user secrets)
 
-Thus by default, the secret is not owned by **this charm**, but, implicitly, by some unknown 'other charm', and that other charm has granted us view rights.
+Thus by default, the secret is not owned by **this charm**, but, implicitly, by some unknown 'other charm' (or a user), and that other has granted us view rights.
 
 The presence of the secret in `State.secrets` entails that we have access to it, either as owners or as grantees. Therefore, if we're not owners, we must be grantees. Absence of a Secret from the known secrets list means we are not entitled to obtaining it in any way. The charm, indeed, shouldn't even know it exists.
 
@@ -830,32 +835,52 @@ If this charm does not own the secret, but also it was not granted view rights b
 To specify a secret owned by this unit (or app):
 
 ```python
+rel = scenario.Relation("web")
 state = scenario.State(
     secrets={
         scenario.Secret(
-            {0: {'key': 'private'}},
-            id='foo',
+            {'key': 'private'},
             owner='unit',  # or 'app'
-            remote_grants={0: {"remote"}}
-            # the secret owner has granted access to the "remote" app over some relation with ID 0
-        ),
-    },
+            # The secret owner has granted access to the "remote" app over some relation:
+            remote_grants={rel.id: {"remote"}}
+        )
+    }
 )
 ```
 
-To specify a secret owned by some other application and give this unit (or app) access to it:
+To specify a secret owned by some other application, or a user secret, and give this unit (or app) access to it:
 
 ```python
 state = scenario.State(
     secrets={
         scenario.Secret(
-            {0: {'key': 'public'}},
-            id='foo',
+            {'key': 'public'},
             # owner=None, which is the default
-            revision=0,  # the revision that this unit (or app) is currently tracking
-        ),
-    },
+        )
+    }
+)
+```
+
+When handling the `secret-expired` and `secret-remove` events, the charm must remove the specified revision of the secret. For `secret-remove`, the revision will no longer be in the `State`, because it's no longer in use (which is why the `secret-remove` event was triggered). To ensure that the charm is removing the secret, check the context for the history of secret removal:
+
+```python
+class SecretCharm(ops.CharmBase):
+    def __init__(self, framework):
+        super().__init__(framework)
+        self.framework.observe(self.on.secret_remove, self._on_secret_remove)
+
+    def _on_secret_remove(self, event):
+        event.secret.remove_revision(event.revision)
+
+
+ctx = scenario.Context(SecretCharm, meta={"name": "foo"})
+secret = scenario.Secret({"password": "xxxxxxxx"}, owner="app")
+old_revision = 42
+state = ctx.run(
+    ctx.on.secret_remove(secret, revision=old_revision),
+    scenario.State(leader=True, secrets={secret})
 )
+assert ctx.removed_secret_revisions == [old_revision]
 ```
 
 ## StoredState

scenario/context.py

@@ -326,6 +326,7 @@ class Context:
     - :attr:`app_status_history`: record of the app statuses the charm has set
     - :attr:`unit_status_history`: record of the unit statuses the charm has set
     - :attr:`workload_version_history`: record of the workload versions the charm has set
+    - :attr:`removed_secret_revisions`: record of the secret revisions the charm has removed
     - :attr:`emitted_events`: record of the events (including custom) that the charm has processed
     - :attr:`action_logs`: logs associated with the action output, set by the charm with
         :meth:`ops.ActionEvent.log`
@@ -450,6 +451,7 @@ def __init__(
         self.app_status_history: List["_EntityStatus"] = []
         self.unit_status_history: List["_EntityStatus"] = []
         self.workload_version_history: List[str] = []
+        self.removed_secret_revisions: List[int] = []
         self.emitted_events: List[EventBase] = []
         self.requested_storages: Dict[str, int] = {}
 

scenario/mocking.py

@@ -2,7 +2,6 @@
 # Copyright 2023 Canonical Ltd.
 # See LICENSE file for licensing details.
 import datetime
-import random
 import shutil
 from io import StringIO
 from pathlib import Path
@@ -202,21 +201,16 @@ def _get_secret(self, id=None, label=None):
             return secrets[0]
 
         elif label:
-            secrets = [s for s in self._state.secrets if s.label == label]
-            if not secrets:
-                raise SecretNotFoundError(label)
-            return secrets[0]
+            try:
+                return self._state.get_secret(label=label)
+            except KeyError:
+                raise SecretNotFoundError(label) from None
 
         else:
             # if all goes well, this should never be reached. ops.model.Secret will check upon
             # instantiation that either an id or a label are set, and raise a TypeError if not.
             raise RuntimeError("need id or label.")
 
-    @staticmethod
-    def _generate_secret_id():
-        id = "".join(map(str, [random.choice(list(range(10))) for _ in range(20)]))
-        return f"secret:{id}"
-
     def _check_app_data_access(self, is_app: bool):
         if not isinstance(is_app, bool):
             raise TypeError("is_app parameter to relation_get must be a boolean")
@@ -317,7 +311,7 @@ def network_get(self, binding_name: str, relation_id: Optional[int] = None):
         try:
             network = self._state.get_network(binding_name)
         except KeyError:
-            network = Network.default("default")  # The name is not used in the output.
+            network = Network("default")  # The name is not used in the output.
         return network.hook_tool_output_fmt()
 
     # setter methods: these can mutate the state.
@@ -368,10 +362,8 @@ def secret_add(
     ) -> str:
         from scenario.state import Secret
 
-        secret_id = self._generate_secret_id()
         secret = Secret(
-            id=secret_id,
-            contents={0: content},
+            content,
             label=label,
             description=description,
             expire=expire,
@@ -381,7 +373,7 @@ def secret_add(
         secrets = set(self._state.secrets)
         secrets.add(secret)
         self._state._update_secrets(frozenset(secrets))
-        return secret_id
+        return secret.id
 
     def _check_can_manage_secret(
         self,
@@ -411,19 +403,19 @@ def secret_get(
         secret = self._get_secret(id, label)
         juju_version = self._context.juju_version
         if not (juju_version == "3.1.7" or juju_version >= "3.3.1"):
-            # in this medieval juju chapter,
+            # In this medieval Juju chapter,
             # secret owners always used to track the latest revision.
             # ref: https://bugs.launchpad.net/juju/+bug/2037120
             if secret.owner is not None:
                 refresh = True
 
-        revision = secret.revision
         if peek or refresh:
-            revision = max(secret.contents.keys())
             if refresh:
-                secret._set_revision(revision)
+                secret._track_latest_revision()
+            assert secret.latest_content is not None
+            return secret.latest_content
 
-        return secret.contents[revision]
+        return secret.tracked_content
 
     def secret_info_get(
         self,
@@ -439,7 +431,7 @@ def secret_info_get(
         return SecretInfo(
             id=secret.id,
             label=secret.label,
-            revision=max(secret.contents),
+            revision=secret._latest_revision,
             expires=secret.expire,
             rotation=secret.rotate,
             rotates=None,  # not implemented yet.
@@ -458,6 +450,15 @@ def secret_set(
         secret = self._get_secret(id, label)
         self._check_can_manage_secret(secret)
 
+        if content == secret.latest_content:
+            # In Juju 3.6 and higher, this is a no-op, but it's good to warn
+            # charmers if they are doing this, because it's not generally good
+            # practice.
+            # https://bugs.launchpad.net/juju/+bug/2069238
+            logger.warning(
+                f"secret {id} contents set to the existing value: new revision not needed",
+            )
+
         secret._update_metadata(
             content=content,
             label=label,
@@ -496,10 +497,32 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None):
         secret = self._get_secret(id)
         self._check_can_manage_secret(secret)
 
-        if revision:
-            del secret.contents[revision]
-        else:
-            secret.contents.clear()
+        # Removing all revisions means that the secret is removed, so is no
+        # longer in the state.
+        if revision is None:
+            secrets = set(self._state.secrets)
+            secrets.remove(secret)
+            self._state._update_secrets(frozenset(secrets))
+            return
+
+        # Juju does not prevent removing the tracked or latest revision, but it
+        # is always a mistake to do this. Rather than having the state model a
+        # secret where the tracked/latest revision cannot be retrieved but the
+        # secret still exists, we raise instead, so that charms know that there
+        # is a problem with their code.
+        if revision in (secret._tracked_revision, secret._latest_revision):
+            raise ValueError(
+                "Charms should not remove the latest revision of a secret. "
+                "Add a new revision with `set_content()` instead, and the previous "
+                "revision will be cleaned up by the secret owner when no longer in use.",
+            )
+
+        # For all other revisions, the content is not visible to the charm
+        # (this is as designed: the secret is being removed, so it should no
+        # longer be in use). That means that the state does not need to be
+        # modified - however, unit tests should be able to verify that the remove call was
+        # executed, so we track that in a history list in the context.
+        self._context.removed_secret_revisions.append(revision)
 
     def relation_remote_app_name(
         self,