Repository: canonical/operator

Issue:
Cannot call Secret.set_info and Secret.set_content in the same hook
If `Secret.set_info()` and `Secret.set_content()` are both called in the same hook, then whichever is first is ignored.

I expect that from a Juju point of view, this is because `secret_set` has been called twice and so it assumes that the later call replaces the earlier one, rather than trying to merge the two together. This is particularly unexpected behaviour in ops because we have two different methods exposing parts of `secret_set`, so a charmer doesn't necessarily know that it's the same underlying Juju hook tool.

This can be reproduced with this small charm:

```python
class SecretInfoAndContentSetCharm(ops.CharmBase):
    """Charm the application."""

    def __init__(self, framework: ops.Framework):
        super().__init__(framework)
        framework.observe(self.on.start, self._on_start)
        framework.observe(self.on.set_action, self._on_set)

    def _on_start(self, event: ops.StartEvent):
        self.unit.add_secret(label="my-secret", content={"foo": "bar"})
        self.unit.status = ops.ActiveStatus()

    def _on_set(self, event):
        secret = self.model.get_secret(label="my-secret")
        secret.set_content(event.params)
        secret.set_info(description="hello world")
```

If you call the action (`juju run unit-name/0 set one=two`) with the code like it is above, then the description will be set, but the content will not. If you swap the code so that the `set_content` and `set_info` calls are around the other way, then the action will set the content but not the description.

Discussion:
jameinel: also, what is the behavior if you:
```
secret-set $ID foo=bar
secret-set $ID baz=bing
```
I *think* the behavior is that content is replaced, and not additive. 

As demonstrated with:
```
$ juju exec --unit ubuntu/0 "secret-set secret:cr87vfcuth6s6ib4phog foo=bar; secret-set secret:cr87vfcuth6s6ib4phog baz=bing"
$ juju show-secret cr87vfcuth6s6ib4phog --reveal
cr87vfcuth6s6ib4phog:
  revision: 52
  owner: ubuntu-lite
  label: foo
  created: 2024-08-29T14:03:42Z
  updated: 2024-09-17T23:20:21Z
  content:
    baz: bing
```

Note that this *is* different from `relation-set`. Which instead uses:
```
Setting an empty string causes the setting to be removed. Duplicate settings
are not allowed.
```
But there if you do:
```
$ juju exec --unit dummy-source/0 "relation-set -r :0 foo=bar; relation-set -r :0 baz=bing"
$ juju exec --unit dummy-source/0 "relation-get -r :0 - dummy-source/0"
baz: bing
egress-subnets: 10.190.220.126/32
foo: bar
ingress-address: 10.190.220.126
private-address: 10.190.220.126
```

I don't think it is predictable that `secret-set` behaves differently than `relation-set`, but I'm not sure if that ship has sailed.

jameinel: Partly the reason why secret-set has to behave differently is that we don't want to cache the known existing values to do the merge operation because of their sensitivit. So we push that onto the charm author. It could be better to find a way to keep doing that for labels, etc.

Though in my testing, calling `secret-set $ID --label=a`; `secret-set $ID --description=blah` does what you would expect:

```
$ juju exec --unit ubuntu-lite/0 "secret-set secret:cr87vfcuth6s6ib4phog --label='foo'; secret-set secret:cr87vfcuth6s6ib4phog --description='some text'"
$ juju show-secret cr87vfcuth6s6ib4phog --reveal
cr87vfcuth6s6ib4phog:
  revision: 52
  owner: ubuntu-lite
  description: some text
  label: foo
  created: 2024-08-29T14:03:42Z
  updated: 2024-09-17T23:27:38Z
  content:
    baz: bing
```

The content hasn't been updated, but the label and description are changed.

tonyandrewmeyer: > Though in my testing, calling `secret-set $ID --label=a`; `secret-set $ID --description=blah` does what you would expect:
> 
> ```
> $ juju exec --unit ubuntu-lite/0 "secret-set secret:cr87vfcuth6s6ib4phog --label='foo'; secret-set secret:cr87vfcuth6s6ib4phog --description='some text'"
> $ juju show-secret cr87vfcuth6s6ib4phog --reveal
> cr87vfcuth6s6ib4phog:
>   revision: 52
>   owner: ubuntu-lite
>   description: some text
>   label: foo
>   created: 2024-08-29T14:03:42Z
>   updated: 2024-09-17T23:27:38Z
>   content:
>     baz: bing
> ```
> 
> The content hasn't been updated, but the label and description are changed.

Isn't that because `label` is special? For example (this is Juju 3.6b1 with LXD, if that matters):

```
$ juju exec --unit secret-info-and-content-set/5 "secret-set secret:crl1juhtvhl39c5lur90 --expire=3600s; secret-set secret:crl1juhtvhl39c5lur90 --description='some text'"
$ juju show-secret crl1juhtvhl39c5lur90 --reveal
crl1juhtvhl39c5lur90:
  revision: 1
  owner: secret-info-and-content-set/5
  description: some text
  label: my-secret
  created: 2024-09-18T00:08:27Z
  updated: 2024-09-18T00:09:53Z
  content:
    foo: bar
```

No expiry, but the description is set. And in the other order:

```
$ juju exec --unit secret-info-and-content-set/5 "secret-set secret:crl1juhtvhl39c5lur90 --description='some other text'; secret-set secret:crl1juhtvhl39c5lur90 --expire=1800s;"
$ juju show-secret crl1juhtvhl39c5lur90 --reveal
crl1juhtvhl39c5lur90:
  revision: 1
  expires: 2024-09-18T00:41:10Z
  owner: secret-info-and-content-set/5
  description: some text
  label: my-secret
  created: 2024-09-18T00:08:27Z
  updated: 2024-09-18T00:11:10Z
  content:
    foo: bar
```

The description hasn't changed, but the expiry is set. Compared to the single call where both update:

```
$ juju exec --unit secret-info-and-content-set/5 "secret-set secret:crl1juhtvhl39c5lur90 --description='some other text still' --expire=900s;"
$ juju show-secret crl1juhtvhl39c5lur90 --reveal
crl1juhtvhl39c5lur90:
  revision: 1
  expires: 2024-09-18T00:28:10Z
  owner: secret-info-and-content-set/5
  description: some other text still
  label: my-secret
  created: 2024-09-18T00:08:27Z
  updated: 2024-09-18T00:13:10Z
  content:
    foo: bar
```
jameinel: > No expiry, but the description is set. And in the other order:

sigh... that seems quite inconsistent. Do we have a bug on Juju about this?
I understand why we don't want to merge secret content, but for everything else I'd rather have multiple calls actually not replace valid values with empty values.
tonyandrewmeyer: > I like the fix, however, it seems to me we should do it in class `Model`, rather than in `_ModelBackend`.

The catch with that is that `Model` doesn't do `secret-set` calls, `Secret` does. `Secret` does not currently have a link to a `Model` object, only to a `_ModelBackend`. If we want this done in `Model`, then that would mean adjusting `Secret` to get access to a `Model` object (which would also mean adjusting `Unit` and `Application`, which create `Secret` objects).

We can't do it in individual `Secret` objects because you could have two `Secret` objects that are the same underlying secret and they should also presumably not clobber each other. I guess it could be done at the `Secret` class level?

> They're supposed to be to 1:1 wrappers around the Juju hook tools.

This is a pretty convincing argument.

> Might be good to discuss during daily.

Sure :). Also about whether we should be caching content when setting and John's comments :).
tonyandrewmeyer: > Do we have a bug on Juju about this?

No. We (Charm-Tech) did have a talk about this issue broadly with other people in the Juju team (it would either have been at the Charm-Tech/Juju APAC sync or Charm-Tech/Ian sync, I can't remember which). The conclusion there was that `secret-set` behaves consistently within itself (each call replaces any previous call in the same hook execution), even though it's different from `relation-set` (as you described), and the problem arises from ops splitting `secret-set` up into two separate methods, one for the metadata and one for the content. So we felt like this should be fixed in ops, because ops was at fault for the limitation of not being able to set both metadata and content in the same hook.

I didn't realise that `label` was different at the time - I only found that out while testing things while working on this PR.

> Partly the reason why secret-set has to behave differently is that we don't want to cache the known existing values to do the merge operation because of their sensitivity.

It's cached somewhere though, right? Because if I do a `secret-set` and then a `secret-get --peek` and then crash the hook I get the new content back in the peek but it's not saved. Is the secret backend handling this rather than Juju? Like Juju does a transaction with the secret backend that then isn't committed/is rolled back if handling the hook fails?

Even if the caching is in a transaction of sorts in the secret backend, doesn't that still make it available to Juju? `secret-get --peek` works somehow :).

> I understand why we don't want to merge secret content, but for everything else I'd rather have multiple calls actually not replace valid values with empty values.

I can open a bug for this and let the Juju'ers hammer out whether it should happen :).
jameinel: On Tue, Sep 17, 2024 at 9:15 PM Tony Meyer ***@***.***> wrote:

> Do we have a bug on Juju about this?
>
> No. We (Charm-Tech) did have a talk about this issue broadly with other
> people in the Juju team (it would either have been at the Charm-Tech/Juju
> APAC sync or Charm-Tech/Ian sync, I can't remember which). The conclusion
> there was that secret-set behaves consistently within itself (each call
> replaces any previous call in the same hook execution), even though it's
> different from relation-set (as you described), and the problem arises
> from ops splitting secret-set up into two separate methods, one for the
> metadata and one for the content. So we felt like this should be fixed in
> ops, because ops was at fault for the limitation of not being able to set
> both metadata and content in the same hook.
>
> I didn't realise that label was different at the time - I only found that
> out while testing things while working on this PR.
>
> Partly the reason why secret-set has to behave differently is that we
> don't want to cache the known existing values to do the merge operation
> because of their sensitivity.
>
> It's cached somewhere though, right? Because if I do a secret-set and
> then a secret-get --peek and then crash the hook I get the new content
> back in the peek but it's not saved. Is the secret backend handling this
> rather than Juju? Like Juju does a transaction with the secret backend that
> then isn't committed/is rolled back if handling the hook fails?
>
> Even if the caching is in a transaction of sorts in the secret backend,
> doesn't that still make it available to Juju? secret-get --peek works
> somehow :).
>

I think it is cached only in the memory of the unit agent, which is why it
rolls back if you crash the hook.
John
=:->


> I understand why we don't want to merge secret content, but for everything
> else I'd rather have multiple calls actually not replace valid values with
> empty values.
>
> I can open a bug for this and let the Juju'ers hammer out whether it
> should happen :).
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/1373#issuecomment-2357292487>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7MW4Z525LMTTGDJSY3ZXDH2FAVCNFSM6AAAAABOKR3KJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJXGI4TENBYG4>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>

tonyandrewmeyer: > Tony will try the dict at the class level, if that doesn't seem great, then explicitly wire the secret_cache dict through Model/Application/Unit/Secret.

The implementation, including ops tests themselves, is ok ([commit](https://github.com/canonical/operator/pull/1373/commits/3752955b8c00bbfe72dc3ad120235faa6e84bcff).

For charm unit tests, Scenario can just reset the cache for each `run`. Harness is much trickier, because there isn't really a defined way of saying "this is a new Juju hook execution". We could reset each time you make a new `Harness` object (or each time the charm is instantiated, which is roughly the same) but it does increase the behaviour that is unexpectedly different from running against an actual Juju. I'm concerned that this would end up strangely leaking secret info/metadata between tests, although maybe the answer is to just make a new `Harness` every time you want a new Juju hook execution.

I'll have a look at the alternative of passing a cache down to `Application` and `Unit` and through those to `Secret`.
benhoyt: > I'll have a look at the alternative of passing a cache down to Application and Unit and through those to Secret.

Sorry, I jumped the gun on that review. Thanks!
tonyandrewmeyer: > I'll have a look at the alternative of passing a cache down to `Application` and `Unit` and through those to `Secret`.

There was less wiring here than I anticipated, because I forgot we already have `_ModelCache`, and it seems reasonable to me to store the cache there. I think this version is much better.

> Can we make sure to super-tox this?

Yes, will do. I didn't with the first version because a `_ModelBackend` change wouldn't change anything for unit tests, but this change potentially does.
tonyandrewmeyer: > > Can we make sure to super-tox this?
> 
> Yes, will do. I didn't with the first version because a `_ModelBackend` change wouldn't change anything for unit tests, but this change potentially does.

There are no new failures. That's to be expected, I think, because the Harness behaviour was already what we are changing this to, so as long as `Model` is now doing what `Harness` already did, the test behaviour isn't going to change.

All of the `set_info` use I can find is in the tls-certificates lib, and that does do both `set_content` and `set_info` together (although perhaps not in an active code path, based on the comment from #1288?). As far as I can tell, the existing code will not work correctly, and this change will make it work correctly, but I haven't found any tests around it (and if there were tests, then you'd think that someone would have noticed this already).
tonyandrewmeyer: I opened [a Juju ticket](https://bugs.launchpad.net/juju/+bug/2081034) where the Juju team can decide if they want to keep the existing `secret-set` behaviour or improve it to match the new ops behaviour.

Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index d4ed43f..1649f28 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -1,6 +1,6 @@
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.4.0
+    rev: v4.5.0
     hooks:
       - id: check-added-large-files
       - id: check-ast
@@ -24,3 +24,10 @@ repos:
         args: [ --preview ]
       - id: ruff-format
         args: [ --preview ]
+  # Spellcheck the code.
+  - repo: https://github.com/codespell-project/codespell
+    rev: v2.2.4
+    hooks:
+    - id: codespell
+      additional_dependencies:
+        - tomli
diff --git a/CHANGES.md b/CHANGES.md
index bcf3a1c..69d6d69 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -159,7 +159,7 @@
 
 ## Features
 
-* `StopEvent`, `RemoveEvent`, and all `LifeCycleEvent`s are no longer deferrable, and will raise a `RuntimeError` if `defer()` is called on the event object (#1122)
+* `StopEvent`, `RemoveEvent`, and all `LifecycleEvent`s are no longer deferrable, and will raise a `RuntimeError` if `defer()` is called on the event object (#1122)
 * Add `ActionEvent.id`, exposing the JUJU_ACTION_UUID environment variable (#1124)
 * Add support for creating `pebble.Plan` objects by passing in a `pebble.PlanDict`, the
   ability to compare two `Plan` objects with `==`, and the ability to create an empty Plan with `Plan()` (#1134)
diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md
index b5bc1b1..0732215 100644
--- a/CODE_OF_CONDUCT.md
+++ b/CODE_OF_CONDUCT.md
@@ -6,7 +6,7 @@ In the interest of fostering an open and welcoming environment, we as
 contributors and maintainers pledge to making participation in our project and
 our community a harassment-free experience for everyone, regardless of age, body
 size, disability, ethnicity, sex characteristics, gender identity and expression,
-level of experience, education, socio-economic status, nationality, personal
+level of experience, education, socioeconomic status, nationality, personal
 appearance, race, religion, or sexual identity and orientation.
 
 ## Our Standards
diff --git a/docs/index.rst b/docs/index.rst
index 84f58b3..c27e2bb 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -2,10 +2,31 @@
 ops library API reference
 =========================
 
+The `ops` library is a Python framework for writing and testing Juju charms.
+
+  See more: `Charm SDK documentation <https://juju.is/docs/sdk>`_
+
+The library provides:
+
+- :ref:`ops_main_entry_point`, used to initialise and run your charm;
+- :ref:`ops_module`, the API to respond to Juju events and manage the application;
+- :ref:`ops_pebble_module`, the Pebble client, a low-level API for Kubernetes containers;
+- :ref:`ops_testing_module`, the framework for unit testing charms in a simulated environment;
+
+You can structure your charm however you like, but with the `ops` library, you
+get a framework that promotes consistency and readability by following best
+practices. It also helps you organise your code better by separating different
+aspects of the charm, such as managing the application’s state, handling
+integrations with other services, and making the charm easier to test.
+
+
 .. toctree::
    :maxdepth: 2
    :caption: Contents:
 
+
+.. _ops_module:
+
 ops module
 ==========
 
@@ -13,8 +34,13 @@ ops module
    :exclude-members: main
 
 
+.. _ops_main_entry_point:
+
 ops.main entry point
 ====================
+
+The main entry point to initialise and run your charm.
+
 .. autofunction:: ops.main
 
 
@@ -25,12 +51,16 @@ legacy main module
    :noindex:
 
 
+.. _ops_pebble_module:
+
 ops.pebble module
 =================
 
 .. automodule:: ops.pebble
 
 
+.. _ops_testing_module:
+
 ops.testing module
 ==================
 
diff --git a/ops/__init__.py b/ops/__init__.py
index 3108647..0e34ef1 100644
--- a/ops/__init__.py
+++ b/ops/__init__.py
@@ -12,31 +12,42 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""The ops library: a Python framework for writing Juju charms.
+"""The API to respond to Juju events and manage the application.
 
-The ops library is a Python framework (`available on PyPI`_) for developing
-and testing Juju charms in a consistent way, using standard Python constructs
-to allow for clean, maintainable, and reusable code.
+This module provides code freatures to your charm, including:
 
-A charm is an operator -- business logic encapsulated in a reusable software
-package that automates every aspect of an application's life.
+- :class:`~ops.CharmBase`, the base class for charms and :class:`~ops.Object`,
+  the base class for charm libraries.
+- :class:`~ops.framework.EventBase` class and individual event types, like
+  the :class:`~ops.ActionEvent` class.
+- :class:`~ops.Framework` class, accessible as ``self.framework`` in a charm,
+  the main interface for the charm to `ops` library infrastructure, including:
 
-Charms written with ops support Kubernetes using Juju's "sidecar charm"
-pattern, as well as charms that deploy to Linux-based machines and containers.
+  - :attr:`~ops.Framework.on` shorthand property used to
+    :meth:`~ops.Framework.observe` and react to Juju events.
+  - :attr:`~ops.Framework.model` attribute to get hold of the Model instance.
 
-Charms should do one thing and do it well. Each charm drives a single
-application and can be integrated with other charms to deliver a complex
-system. A charm handles creating the application in addition to scaling,
-configuration, optimisation, networking, service mesh, observability, and other
-day-2 operations specific to the application.
+- :class:`~ops.model.Model` class that represents the Juju model, accessible as
+  ``self.model`` in a charm, including:
 
-The ops library is part of the Charm SDK (the other part being Charmcraft).
-Full developer documentation for the Charm SDK is available at
-https://juju.is/docs/sdk.
+  - :attr:`~ops.Model.app` attribute, representing the application associated
+    with the charm.
+  - :attr:`~ops.Model.unit` attribute, representing the unit of the application
+    the charm is running on.
+  - :attr:`~ops.Model.relations` attribute, which provides access to relations
+    (integrations) defined in the charm, allowing interaction with other applications.
 
-To learn more about Juju, visit https://juju.is/docs/olm.
+- :class:`~ops.Container` class to control Kubernetes workloads, including:
 
-.. _available on PyPI: https://pypi.org/project/ops/
+  - :meth:`~ops.Container.add_layer` and :meth:`~ops.Container.replan` methods
+    to update Pebble configuration.
+  - :meth:`~ops.Container.pull` and :meth:`~ops.Container.push` methods to copy
+    data to and from a container, respectively.
+  - :meth:`~ops.Container.exec` method to run arbitrary commands inside the
+    container.
+
+- :class:`~ops.StatusBase` class and individual status types, like the
+  :class:`~ops.ActiveStatus` class.
 """
 
 # The "from .X import Y" imports below don't explicitly tell Pyright (or MyPy)
diff --git a/ops/_private/harness.py b/ops/_private/harness.py
index 0933440..c762b45 100644
--- a/ops/_private/harness.py
+++ b/ops/_private/harness.py
@@ -380,7 +380,7 @@ class Harness(Generic[CharmType]):
         if self._charm is not None:
             raise RuntimeError('cannot call the begin method on the harness more than once')
 
-        # The Framework adds attributes to class objects for events, etc. As such, we can't re-use
+        # The Framework adds attributes to class objects for events, etc. As such, we can't reuse
         # the original class against multiple Frameworks. So create a locally defined class
         # and register it.
         # TODO: jam 2020-03-16 We are looking to changes this to Instance attributes instead of
diff --git a/ops/charm.py b/ops/charm.py
index edef8db..4d58008 100644
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -941,7 +941,12 @@ class SecretEvent(HookEvent):
         until :meth:`Secret.get_content()` is called.
         """
         backend = self.framework.model._backend
-        return model.Secret(backend=backend, id=self._id, label=self._label)
+        return model.Secret(
+            backend=backend,
+            id=self._id,
+            label=self._label,
+            _secret_set_cache=self.framework.model._cache._secret_set_cache,
+        )
 
     def snapshot(self) -> Dict[str, Any]:
         """Used by the framework to serialize the event to disk.
@@ -1105,12 +1110,13 @@ class CollectStatusEvent(LifecycleEvent):
 
     The order of priorities is as follows, from highest to lowest:
 
-    * error
     * blocked
     * maintenance
     * waiting
     * active
-    * unknown
+
+    It is an error to call :meth:`add_status` with an instance of
+    :class:`ErrorStatus` or :class:`UnknownStatus`.
 
     If there are multiple statuses with the same priority, the first one added
     wins (and if an event is observed multiple times, the handlers are called
@@ -1151,6 +1157,10 @@ class CollectStatusEvent(LifecycleEvent):
         """
         if not isinstance(status, model.StatusBase):
             raise TypeError(f'status should be a StatusBase, not {type(status).__name__}')
+        if status.name not in model._SETTABLE_STATUS_NAMES:
+            raise model.InvalidStatusError(
+                f'status.name must be in {model._SETTABLE_STATUS_NAMES}, not {status.name!r}'
+            )
         model_ = self.framework.model
         if self.handle.kind == 'collect_app_status':
             if not isinstance(status, model.ActiveStatus):
diff --git a/ops/framework.py b/ops/framework.py
index 11c09e5..0fd0895 100644
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -518,7 +518,11 @@ class PrefixedEvents:
 
 
 class LifecycleEvent(EventBase):
-    """Events tied to the lifecycle of the Framework object."""
+    """Events tied to the lifecycle of the Framework object.
+
+    Note: Here, "lifecycle" was poorly named and has nothing to do with the
+    Juju [application] lifecycle.
+    """
 
     def defer(self) -> NoReturn:
         """Lifecycle events are not deferrable like other events.
diff --git a/ops/jujuversion.py b/ops/jujuversion.py
index 8e25b3e..ea27eb3 100644
--- a/ops/jujuversion.py
+++ b/ops/jujuversion.py
@@ -144,3 +144,13 @@ class JujuVersion:
             # releases have the change (3.2.1 tag was never released).
             return False
         return True
+
+    @property
+    def supports_pebble_log_forwarding(self) -> bool:
+        """Report whether the Pebble bundled with this Juju version supports log forwarding."""
+        # Log forwarding was available from Pebble 1.4, but labels were added in
+        # 1.6, and that's when this became really useful, so we use that as a
+        # cutoff. Juju 3.4.0 was the first to have Pebble 1.6 (actually 1.7).
+        # https://github.com/canonical/pebble/releases/tag/v1.6.0
+        # https://github.com/juju/juju/blob/e1b7dcd7390348c37f8b860011e7436e6ed3f4cc/go.mod#L27
+        return (self.major, self.minor, self.patch) >= (3, 4, 0)
diff --git a/ops/model.py b/ops/model.py
index 335dd9a..970a08e 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -14,6 +14,7 @@
 
 """Representations of Juju's model, application, unit, and other entities."""
 
+import collections
 import dataclasses
 import datetime
 import enum
@@ -304,7 +305,13 @@ class Model:
             # Canonicalize to "secret:<id>" form for consistency in backend calls.
             id = Secret._canonicalize_id(id, self.uuid)
         content = self._backend.secret_get(id=id, label=label)
-        return Secret(self._backend, id=id, label=label, content=content)
+        return Secret(
+            self._backend,
+            id=id,
+            label=label,
+            content=content,
+            _secret_set_cache=self._cache._secret_set_cache,
+        )
 
     def get_cloud_spec(self) -> 'CloudSpec':
         """Get details of the cloud in which the model is deployed.
@@ -333,6 +340,9 @@ class _ModelCache:
     def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'):
         self._meta = meta
         self._backend = backend
+        self._secret_set_cache: collections.defaultdict[str, Dict[str, Any]] = (
+            collections.defaultdict(dict)
+        )
         self._weakrefs: _WeakCacheType = weakref.WeakValueDictionary()
 
     @typing.overload
@@ -502,7 +512,13 @@ class Application:
             rotate=rotate,
             owner='application',
         )
-        return Secret(self._backend, id=id, label=label, content=content)
+        return Secret(
+            self._backend,
+            id=id,
+            label=label,
+            content=content,
+            _secret_set_cache=self._cache._secret_set_cache,
+        )
 
 
 def _calculate_expiry(
@@ -685,7 +701,13 @@ class Unit:
             rotate=rotate,
             owner='unit',
         )
-        return Secret(self._backend, id=id, label=label, content=content)
+        return Secret(
+            self._backend,
+            id=id,
+            label=label,
+            content=content,
+            _secret_set_cache=self._cache._secret_set_cache,
+        )
 
     def open_port(
         self, protocol: typing.Literal['tcp', 'udp', 'icmp'], port: Optional[int] = None
@@ -1270,6 +1292,7 @@ class Secret:
         id: Optional[str] = None,
         label: Optional[str] = None,
         content: Optional[Dict[str, str]] = None,
+        _secret_set_cache: Optional['collections.defaultdict[str, Dict[str, Any]]'] = None,
     ):
         if not (id or label):
             raise TypeError('Must provide an id or label, or both')
@@ -1279,6 +1302,9 @@ class Secret:
         self._id = id
         self._label = label
         self._content = content
+        self._secret_set_cache: collections.defaultdict[str, Dict[str, Any]] = (
+            collections.defaultdict(dict) if _secret_set_cache is None else _secret_set_cache
+        )
 
     def __repr__(self):
         fields: List[str] = []
@@ -1483,7 +1509,26 @@ class Secret:
         self._validate_content(content)
         if self._id is None:
             self._id = self.get_info().id
-        self._backend.secret_set(typing.cast(str, self.id), content=content)
+
+        # If `_backend.secret_set` has already been called, this call will
+        # overwrite anything done in the previous call (even if it was in a
+        # different event handler, as long as it was in the same Juju hook
+        # context). We want to provide more predictable behavior, so we cache
+        # the values and provide them in subsequent calls.
+        cached_data = self._secret_set_cache[self._id]
+        expire = _calculate_expiry(cached_data['expire']) if 'expire' in cached_data else None
+        # Don't store the actual secret content in memory, but put a sentinel
+        # there to indicate that the content has been set during this hook.
+        cached_data['content'] = object()
+
+        self._backend.secret_set(
+            typing.cast(str, self.id),
+            content=content,
+            label=cached_data.get('label'),
+            description=cached_data.get('description'),
+            expire=expire,
+            rotate=cached_data.get('rotate'),
+        )
         # We do not need to invalidate the cache here, as the content is the
         # same until `refresh` is used, at which point the cache is invalidated.
 
@@ -1513,12 +1558,33 @@ class Secret:
         """
         if label is None and description is None and expire is None and rotate is None:
             raise TypeError(
-                'Must provide a label, description, expiration time, ' 'or rotation policy'
+                'Must provide a label, description, expiration time, or rotation policy'
             )
         if self._id is None:
             self._id = self.get_info().id
+
+        # If `_backend.secret_set` has already been called, this call will
+        # overwrite anything done in the previous call (even if it was in a
+        # different event handler, as long as it was in the same Juju hook
+        # context). We want to provide more predictable behavior, so we cache
+        # the values and provide them in subsequent calls.
+        cached_data = self._secret_set_cache[self._id]
+        label = cached_data.get('label') if label is None else label
+        cached_data['label'] = label
+        description = cached_data.get('description') if description is None else description
+        cached_data['description'] = description
+        expire = cached_data.get('expire') if expire is None else expire
+        cached_data['expire'] = expire
+        rotate = cached_data.get('rotate') if rotate is None else rotate
+        cached_data['rotate'] = rotate
+        # Get the previous content from the unit agent's cache.
+        content = (
+            self._backend.secret_get(id=self._id, peek=True) if 'content' in cached_data else None
+        )
+
         self._backend.secret_set(
             typing.cast(str, self.id),
+            content=content,
             label=label,
             description=description,
             expire=_calculate_expiry(expire),
@@ -1907,6 +1973,9 @@ class StatusBase:
             raise TypeError('cannot instantiate a base class')
         self.message = message
 
+    def __init_subclass__(cls):
+        StatusBase._register(cls)
+
     def __eq__(self, other: 'StatusBase') -> bool:
         if not isinstance(self, type(other)):
             return False
@@ -1938,14 +2007,21 @@ class StatusBase:
 
     @classmethod
     def register(cls, child: Type['StatusBase']):
-        """Register a Status for the child's name."""
+        """.. deprecated:: 2.17.0 Deprecated - this was for internal use only."""
+        warnings.warn(
+            'StatusBase.register is for internal use only', DeprecationWarning, stacklevel=2
+        )
+        cls._register(child)
+        return child
+
+    @classmethod
+    def _register(cls, child: Type['StatusBase']) -> None:
         if not (hasattr(child, 'name') and isinstance(child.name, str)):
             raise TypeError(
                 f"Can't register StatusBase subclass {child}: ",
                 'missing required `name: str` class attribute',
             )
         cls._statuses[child.name] = child
-        return child
 
     _priorities = {
         'error': 5,
@@ -1965,7 +2041,6 @@ class StatusBase:
         return max(statuses, key=lambda status: cls._priorities.get(status.name, 0))
 
 
-@StatusBase.register
 class UnknownStatus(StatusBase):
     """The unit status is unknown.
 
@@ -1986,7 +2061,6 @@ class UnknownStatus(StatusBase):
         return 'UnknownStatus()'
 
 
-@StatusBase.register
 class ErrorStatus(StatusBase):
     """The unit status is error.
 
@@ -2000,7 +2074,6 @@ class ErrorStatus(StatusBase):
     name = 'error'
 
 
-@StatusBase.register
 class ActiveStatus(StatusBase):
     """The unit or application is ready and active.
 
@@ -2016,7 +2089,6 @@ class ActiveStatus(StatusBase):
         super().__init__(message)
 
 
-@StatusBase.register
 class BlockedStatus(StatusBase):
     """The unit or application requires manual intervention.
 
@@ -2027,7 +2099,6 @@ class BlockedStatus(StatusBase):
     name = 'blocked'
 
 
-@StatusBase.register
 class MaintenanceStatus(StatusBase):
     """The unit or application is performing maintenance tasks.
 
@@ -2041,7 +2112,6 @@ class MaintenanceStatus(StatusBase):
     name = 'maintenance'
 
 
-@StatusBase.register
 class WaitingStatus(StatusBase):
     """The unit or application is waiting on a charm it's integrated with.
 
@@ -2317,7 +2387,11 @@ class Container:
         self._pebble.start_services(service_names)
 
     def restart(self, *service_names: str):
-        """Restart the given service(s) by name."""
+        """Restart the given service(s) by name.
+
+        Listed running services will be stopped and restarted, and listed stopped
+        services will be started.
+        """
         if not service_names:
             raise TypeError('restart expected at least 1 argument, got 0')
 
@@ -3125,7 +3199,7 @@ def _format_action_result_dict(
     parent_key: Optional[str] = None,
     output: Optional[Dict[str, str]] = None,
 ) -> Dict[str, str]:
-    """Turn a nested dictionary into a flattened dictionary, using '.' as a key seperator.
+    """Turn a nested dictionary into a flattened dictionary, using '.' as a key separator.
 
     This is used to allow nested dictionaries to be translated into the dotted format required by
     the Juju `action-set` hook tool in order to set nested data on an action.
@@ -3664,7 +3738,8 @@ class _ModelBackend:
         if expire is not None:
             args.extend(['--expire', expire.isoformat()])
         if rotate is not None:
-            args += ['--rotate', rotate.value]
+            args.extend(['--rotate', rotate.value])
+
         with tempfile.TemporaryDirectory() as tmp:
             # The content is None or has already been validated with Secret._validate_content
             for k, v in (content or {}).items():
@@ -3691,9 +3766,9 @@ class _ModelBackend:
         if expire is not None:
             args.extend(['--expire', expire.isoformat()])
         if rotate is not None:
-            args += ['--rotate', rotate.value]
+            args.extend(['--rotate', rotate.value])
         if owner is not None:
-            args += ['--owner', owner]
+            args.extend(['--owner', owner])
         with tempfile.TemporaryDirectory() as tmp:
             # The content has already been validated with Secret._validate_content
             for k, v in content.items():
@@ -3965,7 +4040,7 @@ class CloudSpec:
     """A list of CA certificates."""
 
     skip_tls_verify: bool = False
-    """Whether to skip TLS verfication."""
+    """Whether to skip TLS verification."""
 
     is_controller_cloud: bool = False
     """If this is the cloud used by the controller, defaults to False."""
diff --git a/ops/pebble.py b/ops/pebble.py
index 7859415..5c642fd 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -12,9 +12,21 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""Client for the Pebble API (HTTP over Unix socket).
+"""Client for the Pebble API.
 
-For a command-line interface for local testing, see test/pebble_cli.py.
+This module provides a way to interact with Pebble, including:
+
+- :class:`~ops.pebble.Client`; communicates directly with the Pebble API.
+- :class:`~ops.pebble.Layer` class to define Pebble configuration layers,
+  including:
+
+  - :class:`~ops.pebble.Check` class to represent Pebble checks.
+  - :class:`~ops.pebble.LogTarget` class to represent Pebble log targets.
+  - :class:`~ops.pebble.Service` class to represent Pebble service descriptions.
+
+For a command-line interface for local testing, see ``test/pebble_cli.py``.
+
+  See more: `Pebble documentation <https://canonical-pebble.readthedocs-hosted.com/>`_
 """
 
 from __future__ import annotations
@@ -2187,6 +2199,9 @@ class Client:
     ) -> ChangeID:
         """Restart services by name and wait (poll) for them to be started.
 
+        Listed running services will be stopped and restarted, and listed stopped
+        services will be started.
+
         Args:
             services: Non-empty list of services to restart.
             timeout: Seconds before restart change is considered timed out (float). If
diff --git a/pyproject.toml b/pyproject.toml
index 4bfa2d1..2379629 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -224,3 +224,7 @@ reportUnnecessaryComparison = false
 reportUnnecessaryTypeIgnoreComment = "error"
 disableBytesTypePromotions = false
 stubPath = ""
+
+[tool.codespell]
+skip = './docs/_build,.venv,venv,build'
+quiet-level = 3
diff --git a/tox.ini b/tox.ini
index cf097a1..505aa79 100644
--- a/tox.ini
+++ b/tox.ini
@@ -68,9 +68,11 @@ commands =
 description = Check code against coding style standards
 deps =
     ruff==0.4.5
+    codespell==2.3.0
 commands =
     ruff check --preview
     ruff format --preview --check
+    codespell
 
 [testenv:static]
 description = Run static type checker


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index a633be3..e93ba7b 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -11,8 +11,26 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+"""Framework for unit testing charms in a simulated Juju environment.
 
-"""Infrastructure to build unit tests for charms using the ops library."""
+The module includes:
+
+- :class:`ops.testing.Harness`, a class to set up the simulated environment,
+  that provides:
+
+  - :meth:`~ops.testing.Harness.add_relation` method, to declare a relation
+    (integration) with another app.
+  - :meth:`~ops.testing.Harness.begin` and :meth:`~ops.testing.Harness.cleanup`
+    methods to start and end the testing lifecycle.
+  - :meth:`~ops.testing.Harness.evaluate_status` method, which aggregates the
+    status of the charm after test interactions.
+  - :attr:`~ops.testing.Harness.model` attribute, which exposes e.g. the
+    :attr:`~ops.Model.unit` attribute for detailed assertions on the unit's state.
+
+.. note::
+    Unit testing is only one aspect of a comprehensive testing strategy. For more
+    on testing charms, see `Charm SDK | Testing <https://juju.is/docs/sdk/testing>`_.
+"""
 
 from ._private.harness import (
     ActionFailed,
diff --git a/test/test_charm.py b/test/test_charm.py
index 0939fbb..ecb9cb5 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -797,6 +797,32 @@ def test_secret_events(request: pytest.FixtureRequest):
     ]
 
 
+def test_secret_event_caches_secret_set(request: pytest.FixtureRequest, fake_script: FakeScript):
+    class MyCharm(ops.CharmBase):
+        def __init__(self, framework: ops.Framework):
+            super().__init__(framework)
+            self.secrets: typing.List[ops.Secret] = []
+            self.framework.observe(self.on.secret_changed, self.on_secret_changed)
+
+        def on_secret_changed(self, event: ops.SecretChangedEvent):
+            event.secret.set_info(description='desc')
+            event.secret.set_content({'key': 'value'})
+            self.secrets.append(event.secret)
+
+    fake_script.write('secret-get', """echo '{"key": "value"}'""")
+    fake_script.write('secret-set', 'exit 0')
+
+    framework = create_framework(request)
+    charm = MyCharm(framework)
+
+    charm.on.secret_changed.emit('secret:changed', None)
+    charm.on.secret_changed.emit('secret:changed', None)
+    cache = charm.secrets[0]._secret_set_cache
+    assert cache is charm.secrets[1]._secret_set_cache
+    assert charm.secrets[0]._secret_set_cache['secret:changed']['description'] == 'desc'
+    assert 'content' in cache['secret:changed']
+
+
 def test_collect_app_status_leader(request: pytest.FixtureRequest, fake_script: FakeScript):
     class MyCharm(ops.CharmBase):
         def __init__(self, framework: ops.Framework):
@@ -959,7 +985,6 @@ def test_add_status_type_error(request: pytest.FixtureRequest, fake_script: Fake
         (['waiting', 'blocked'], 'blocked'),
         (['waiting', 'maintenance'], 'maintenance'),
         (['active', 'waiting'], 'waiting'),
-        (['active', 'unknown'], 'active'),
     ],
 )
 def test_collect_status_priority_valid(
@@ -994,6 +1019,7 @@ def test_collect_status_priority_valid(
     [
         ['blocked', 'error'],
         ['unknown'],
+        ['active', 'unknown'],
     ],
 )
 def test_collect_status_priority_invalid(
@@ -1015,7 +1041,7 @@ def test_collect_status_priority_invalid(
 
     framework = create_framework(request)
     charm = MyCharm(framework, statuses=statuses)
-    with pytest.raises(ModelError):
+    with pytest.raises(ops.InvalidStatusError):
         ops.charm._evaluate_status(charm)
 
 
diff --git a/test/test_jujuversion.py b/test/test_jujuversion.py
index 713248b..fe2e457 100644
--- a/test/test_jujuversion.py
+++ b/test/test_jujuversion.py
@@ -100,6 +100,20 @@ def test_supports_exec_service_context():
     assert ops.JujuVersion('3.4.0').supports_exec_service_context
 
 
+@pytest.mark.parametrize(
+    'version,has_support',
+    [
+        ('2.9.50', False),
+        ('3.3.6', False),
+        ('3.4.0', True),
+        ('3.4.1', True),
+        ('4.0.0', True),
+    ],
+)
+def test_supports_pebble_log_forwarding(version: str, has_support: bool):
+    assert ops.JujuVersion(version).supports_pebble_log_forwarding is has_support
+
+
 @pytest.mark.parametrize(
     'invalid_version',
     [
diff --git a/test/test_model.py b/test/test_model.py
index 0eadd4d..7eb2ffa 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -820,11 +820,19 @@ class TestModel:
         with pytest.raises(TypeError):
             ops.StatusBase('test')
 
-        class NoNameStatus(ops.StatusBase):
-            pass
+        with pytest.raises(TypeError):
+            # TypeError due to missing `name` attribute
+            class NoNameStatus(ops.StatusBase):  # pyright: ignore[reportUnusedClass]
+                pass
 
         with pytest.raises(TypeError):
-            ops.StatusBase.register(NoNameStatus)
+            # TypeError due to non str type `name` attribute
+            class NonStringNameStatus(ops.StatusBase):  # pyright: ignore[reportUnusedClass]
+                name = None  # pyright: ignore[reportAssignmentType]
+
+    def test_base_status_register_is_deprecated(self):
+        with pytest.deprecated_call():
+            ops.StatusBase.register(ops.ActiveStatus)
 
     def test_status_repr(self):
         test_cases = {
@@ -3242,7 +3250,7 @@ class TestModelBackend:
         monkeypatch.setenv('JUJU_RELATION_ID', 'x:5')
         monkeypatch.setenv('JUJU_REMOTE_APP', 'remoteapp1')
         assert self.backend.relation_remote_app_name(5) == 'remoteapp1'
-        os.environ['JUJU_RELATION_ID'] = '5'
+        monkeypatch.setenv('JUJU_RELATION_ID', '5')
         assert self.backend.relation_remote_app_name(5) == 'remoteapp1'
 
     def test_relation_remote_app_name_script_success(
@@ -3266,7 +3274,7 @@ echo '"remoteapp2"'
 
         # JUJU_RELATION_ID unset but JUJU_REMOTE_APP set
         monkeypatch.delenv('JUJU_RELATION_ID')
-        os.environ['JUJU_REMOTE_APP'] = 'remoteapp1'
+        monkeypatch.setenv('JUJU_REMOTE_APP', 'remoteapp1')
         assert self.backend.relation_remote_app_name(5) == 'remoteapp2'
 
         # Both set, but JUJU_RELATION_ID a different relation
@@ -3823,6 +3831,67 @@ class TestSecretClass:
         with pytest.raises(TypeError):
             secret.set_info()  # no args provided
 
+    def test_set_content_then_info(self, model: ops.Model, fake_script: FakeScript):
+        fake_script.write('secret-set', """exit 0""")
+        fake_script.write('secret-get', """echo '{"foo": "bar"}'""")
+
+        secret = self.make_secret(model, id='q')
+        secret.set_content({'foo': 'bar'})
+        description = 'desc'
+        secret.set_info(description=description)
+
+        calls = fake_script.calls(clear=True)
+        assert calls[0][:-1] == ['secret-set', f'secret://{model._backend.model_uuid}/q']
+        assert calls[0][-1].startswith('foo#file=') and calls[0][-1].endswith('/foo')
+        assert calls[1] == [
+            'secret-get',
+            f'secret://{model._backend.model_uuid}/q',
+            '--peek',
+            '--format=json',
+        ]
+        assert calls[2][:-1] == [
+            'secret-set',
+            f'secret://{model._backend.model_uuid}/q',
+            '--description',
+            description,
+        ]
+        assert calls[2][-1].startswith('foo#file=') and calls[0][-1].endswith('/foo')
+
+    def test_set_info_then_content(self, model: ops.Model, fake_script: FakeScript):
+        fake_script.write('secret-set', """exit 0""")
+
+        secret = self.make_secret(model, id='q')
+        description = 'desc'
+        secret.set_info(description=description)
+        secret.set_content({'foo': 'bar'})
+
+        calls = fake_script.calls(clear=True)
+        assert calls[0] == [
+            'secret-set',
+            f'secret://{model._backend.model_uuid}/q',
+            '--description',
+            description,
+        ]
+        assert calls[1][:-1] == [
+            'secret-set',
+            f'secret://{model._backend.model_uuid}/q',
+            '--description',
+            description,
+        ]
+        assert calls[1][-1].startswith('foo#file=') and calls[1][-1].endswith('/foo')
+
+    def test_set_content_aggregates(self, model: ops.Model, fake_script: FakeScript):
+        fake_script.write('secret-set', """exit 0""")
+
+        secret = self.make_secret(model, id='q')
+        secret.set_content({'foo': 'bar'})
+        secret.set_content({'baz': 'qux', 'foo': 'newbar'})
+
+        calls = fake_script.calls(clear=True)
+        assert calls[0][:-1] == ['secret-set', f'secret://{model._backend.model_uuid}/q']
+        assert calls[0][:-1] == ['secret-set', f'secret://{model._backend.model_uuid}/q']
+        assert fake_script.secrets() == {'foo': 'newbar', 'baz': 'qux'}
+
     def test_grant(self, model: ops.Model, fake_script: FakeScript):
         fake_script.write('relation-list', """echo '[]'""")
         fake_script.write('secret-grant', """exit 0""")
diff --git a/test/test_testing.py b/test/test_testing.py
index 4c88f23..965ebc8 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -3232,6 +3232,9 @@ class TestHarness:
 
     def test_evaluate_status(self):
         class TestCharm(ops.CharmBase):
+            app_status_to_add: ops.StatusBase
+            unit_status_to_add: ops.StatusBase
+
             def __init__(self, framework: ops.Framework):
                 super().__init__(framework)
                 self.framework.observe(self.on.collect_app_status, self._on_collect_app_status)