Some Scenario Secrets Snafus

[benhoyt]

When writing a bunch of Scenario tests today, I found a few issues with secrets:

• [ ] Secret.changed_event and related properties have a typo in the field name: https://github.com/canonical/ops-scenario/pull/94
• [ ] Secret.granted doesn't seem to be used or enforced; the docs imply you need it (which seems correct), but secrets-related tests work fine without it, and from grepping the code it looks like granted is not referenced at all.
• [ ] _MockModelBackend._get_secret needs to update the label if both "id" and "label" are specified in the secret-get call. This is how Juju works; Harness does this here. A possible diff to fix this is shown below.
• [ ] Relatedly, the "bypass frozen metadata" in the existing implementation gives me the heebie-jeebies. Won't that break the immutability/hashing contract, and perhaps the concept of Scenario state? (See your docs.) I wonder if we could revisit this? Maybe Secret shouldn't be frozen, or maybe it should be modelled differently.
• [ ] The line object.__setattr__(self, "contents"[revision + 1], content) in Secret._update_metadata looks like a typo. Perhaps it should be self.contents[revision + 1] = content or similar?

Possible diff to make _get_secret apply label

diff --git a/scenario/mocking.py b/scenario/mocking.py
index e5bd429..e933758 100644
--- a/scenario/mocking.py
+++ b/scenario/mocking.py
@@ -145,7 +145,7 @@ class _MockModelBackend(_ModelBackend):
             # but ops.Secret will prepend a "secret:" prefix to that ID.
             # we allow getting secret by either version.
             try:
-                return next(
+                secret = next(
                     filter(
                         lambda s: canonicalize_id(s.id) == canonicalize_id(id),
                         self._state.secrets,
@@ -153,6 +153,10 @@ class _MockModelBackend(_ModelBackend):
                 )
             except StopIteration:
                 raise SecretNotFoundError()
+            if label:
+                # bypass frozen metadata
+                object.__setattr__(secret, "label", label)
+            return secret
         elif label:
             try:
                 return next(filter(lambda s: s.label == label, self._state.secrets))

[PietroPasotti]

Secrets are known to be buggy on main; look at #86 for the state of the art. v6 branch should contain that as well, so I'd recommending testing against v6 if you're looking at model consistency.

Regarding the implementation I'm on board, and the bypass-frozen- happens in multiple places in state.py. I'm also not happy about that but didn't find a neater solution yet.

[benhoyt]

Secrets are known to be buggy on main; look at https://github.com/canonical/ops-scenario/pull/86 for the state of the art. v6 branch should contain that as well, so I'd recommending testing against v6 if you're looking at model consistency.

Oh, sorry, I'd forgotten about that PR. I'm on leave tomorrow and Friday, but I'll re-test this stuff against v6 on Monday and see if any of the issues are still there.

[tonyandrewmeyer]

• [ ] Secret.changed_event and related properties have a typo in the field name: Fix Event field name ("name" -> "path") #94

Already fixed.

• [ ] Secret.granted doesn't seem to be used or enforced; the docs imply you need it (which seems correct), but secrets-related tests work fine without it, and from grepping the code it looks like granted is not referenced at all.

Already fixed.

• [ ] _MockModelBackend._get_secret needs to update the label if both "id" and "label" are specified in the secret-get call. This is how Juju works; Harness does this here. A possible diff to fix this is shown below.

Will fix.

• [ ] Relatedly, the "bypass frozen metadata" in the existing implementation gives me the heebie-jeebies. Won't that break the immutability/hashing contract, and perhaps the concept of Scenario state? (See your docs.) I wonder if we could revisit this? Maybe Secret shouldn't be frozen, or maybe it should be modelled differently.

Pietro and I don't love these either, for what it's worth. It should violate the immutability, because the objects being changed should only be the ones in the output state, before it's given back to the caller. However, it would be good to have a cleaner method for this. I'll open an issue for that.

• [ ] The line object.__setattr__(self, "contents"[revision + 1], content) in Secret._update_metadata looks like a typo. Perhaps it should be self.contents[revision + 1] = content or similar?

Already fixed.