From 18d0af6b15437468e0e24bd65e461f029f353633 Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Thu, 8 Jun 2023 11:28:55 +1200
Subject: [PATCH 1/8] Test commit

---
 requirements-dev.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/requirements-dev.txt b/requirements-dev.txt
index 18abf8073..2be8be103 100644
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -13,3 +13,4 @@ coverage[toml]==7.0.5
 typing_extensions==4.2.0
 
 -r requirements.txt
+

From 807b8589e008fabfa3dee52f0f167fee841cb1fd Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Thu, 8 Jun 2023 15:52:07 +1200
Subject: [PATCH 2/8] Update Pyright to latest version, 1.1.313 (and fix new
 errors)

Boy this was more painful than I expected. Lots of fighting with the
compiler. I ended up disabling a few more Pyright issues, specifically
these seemed more trouble than they're worth:

* reportPrivateUsage: we do this often in the codebase, for example
  charm.py pokes at _ModelBackend stuff
* reportUnnecessaryIsInstance: we do lots of isinstance checks to
  detect type issues at runtime (we've done this since the beginning,
  and it's useful for people not using type checking)
* reportUnnecessaryComparison: similar to the above, but for checking
  "if non_optional_value is None" and the like
---
 ops/charm.py         |  8 ++---
 ops/framework.py     | 72 +++++++++++++++++++-------------------
 ops/jujuversion.py   |  2 +-
 ops/log.py           |  2 +-
 ops/main.py          |  2 +-
 ops/model.py         | 83 ++++++++++++++++++++------------------------
 ops/pebble.py        |  8 ++---
 ops/storage.py       |  2 +-
 ops/testing.py       | 50 +++++++++++++-------------
 pyproject.toml       |  3 ++
 requirements-dev.txt |  2 +-
 11 files changed, 115 insertions(+), 119 deletions(-)

diff --git a/ops/charm.py b/ops/charm.py
index 6698219c8..1f8ef823c 100755
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -219,7 +219,7 @@ def set_results(self, results: '_SerializedData'):
         Args:
             results: The result of the action as a Dict
         """
-        self.framework.model._backend.action_set(results)   # pyright: reportPrivateUsage=false
+        self.framework.model._backend.action_set(results)
 
     def log(self, message: str):
         """Send a message that a user will see while the action is running.
@@ -227,7 +227,7 @@ def log(self, message: str):
         Args:
             message: The message for the user.
         """
-        self.framework.model._backend.action_log(message)  # pyright: reportPrivateUsage=false
+        self.framework.model._backend.action_log(message)
 
     def fail(self, message: str = ''):
         """Report that this action has failed.
@@ -235,7 +235,7 @@ def fail(self, message: str = ''):
         Args:
             message: Optional message to record why it has failed.
         """
-        self.framework.model._backend.action_fail(message)  # pyright: reportPrivateUsage=false
+        self.framework.model._backend.action_fail(message)
 
 
 class InstallEvent(HookEvent):
@@ -1141,7 +1141,7 @@ def from_yaml(
         meta = cast('_CharmMetaDict', yaml.safe_load(metadata))
         raw_actions = {}
         if actions is not None:
-            raw_actions = cast(Dict[str, '_ActionMetaDict'], yaml.safe_load(actions))
+            raw_actions = cast(Optional[Dict[str, Any]], yaml.safe_load(actions))
             if raw_actions is None:
                 raw_actions = {}
         return cls(meta, raw_actions)
diff --git a/ops/framework.py b/ops/framework.py
index 6057f3558..438853023 100755
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -182,8 +182,8 @@ def from_path(cls, path: str) -> 'Handle':
                     good = True
             if not good:
                 raise RuntimeError(f"attempted to restore invalid handle path {path}")
-            handle = Handle(handle, kind, key)  # pyright: reportUnboundVariable=false
-        return handle
+            handle = Handle(handle, kind, key)  # type: ignore
+        return typing.cast(Handle, handle)
 
 
 class EventBase:
@@ -196,7 +196,7 @@ class EventBase:
     # after being loaded from snapshot, or by `BoundEvent.emit()` if this
     # event is being fired for the first time.
     # TODO this is hard to debug, this should be refactored
-    framework: 'Framework' = None
+    framework: 'Framework' = None  # type: ignore
 
     def __init__(self, handle: Handle):
         self.handle = handle
@@ -321,7 +321,7 @@ def __get__(self, emitter: Optional['Object'],
         framework = getattr(emitter, 'framework', None)
         if framework is not None:
             framework.register_type(self.event_type, emitter, self.event_kind)
-        return BoundEvent(emitter, self.event_type, self.event_kind)
+        return BoundEvent(emitter, self.event_type, typing.cast(str, self.event_kind))
 
 
 class BoundEvent(Generic[_EventType]):
@@ -387,7 +387,7 @@ class Object:
     been created.
 
     """
-    handle_kind: str = HandleKind()
+    handle_kind: str = HandleKind()  # type: ignore
 
     if TYPE_CHECKING:
         # to help the type checker and IDEs:
@@ -396,8 +396,8 @@ class Object:
         def on(self) -> 'ObjectEvents': ...  # noqa
 
     def __init__(self, parent: Union['Framework', 'Object'], key: Optional[str]):
-        self.framework: Framework = None
-        self.handle: Handle = None
+        self.framework: Framework = None  # type: ignore
+        self.handle: Handle = None  # type: ignore
 
         kind = self.handle_kind
         if isinstance(parent, Framework):
@@ -405,11 +405,11 @@ def __init__(self, parent: Union['Framework', 'Object'], key: Optional[str]):
             # Avoid Framework instances having a circular reference to themselves.
             if self.framework is self:
                 self.framework = weakref.proxy(self.framework)
-            self.handle = Handle(None, kind, key)
+            self.handle = Handle(None, kind, typing.cast(str, key))
         else:
             self.framework = parent.framework
-            self.handle = Handle(parent, kind, key)
-        self.framework._track(self)
+            self.handle = Handle(parent, kind, typing.cast(str, key))
+        self.framework._track(self)  # type: ignore
 
         # TODO Detect conflicting handles here.
 
@@ -556,17 +556,17 @@ def __str__(self):
 class Framework(Object):
     """Main interface from the Charm to the Operator Framework internals."""
 
-    on = FrameworkEvents()
+    on = FrameworkEvents()  # type: ignore
 
     # Override properties from Object so that we can set them in __init__.
-    model: 'Model' = None
-    meta: 'CharmMeta' = None
-    charm_dir: 'Path' = None
+    model: 'Model' = None  # type: ignore
+    meta: 'CharmMeta' = None  # type: ignore
+    charm_dir: 'Path' = None  # type: ignore
 
     # to help the type checker and IDEs:
 
     if TYPE_CHECKING:
-        _stored: 'StoredStateData' = None
+        _stored: 'StoredStateData' = None  # type: ignore
         @property
         def on(self) -> 'FrameworkEvents': ...  # noqa
 
@@ -593,9 +593,9 @@ def __init__(self, storage: Union[SQLiteStorage, JujuStorage],
         # [(observer_path, method_name, parent_path, event_key)]
         self._observers: _ObserverPath = []
         # {observer_path: observing Object}
-        self._observer: _PathToObjectMapping = weakref.WeakValueDictionary()
+        self._observer: _PathToObjectMapping = weakref.WeakValueDictionary()  # type: ignore
         # {object_path: object}
-        self._objects: _PathToSerializableMapping = weakref.WeakValueDictionary()
+        self._objects: _PathToSerializableMapping = weakref.WeakValueDictionary()  # type: ignore
         # {(parent_path, kind): cls}
         # (parent_path, kind) is the address of _this_ object: the parent path
         # plus a 'kind' string that is the name of this object.
@@ -606,7 +606,8 @@ def __init__(self, storage: Union[SQLiteStorage, JujuStorage],
             logger.warning(
                 "deprecated: Framework now takes a Storage not a path")
             storage = SQLiteStorage(storage)
-        self._storage: 'SQLiteStorage' = storage
+        # TODO(benhoyt): should probably have a Storage protocol
+        self._storage: 'SQLiteStorage' = storage  # type: ignore
 
         # We can't use the higher-level StoredState because it relies on events.
         self.register_type(StoredStateData, None, StoredStateData.handle_kind)
@@ -678,7 +679,7 @@ def commit(self):
         self._storage.commit()
 
     def register_type(self, cls: 'Type[_Serializable]', parent: Optional['_ParentHandle'],
-                      kind: str = None):
+                      kind: Optional[str] = None):
         """Register a type to a handle."""
         parent_path: Optional[str] = None
         if isinstance(parent, Object):
@@ -686,8 +687,8 @@ def register_type(self, cls: 'Type[_Serializable]', parent: Optional['_ParentHan
         elif isinstance(parent, Handle):
             parent_path = parent.path
 
-        kind: str = kind or cls.handle_kind
-        self._type_registry[(parent_path, kind)] = cls
+        kind_: str = kind or cls.handle_kind
+        self._type_registry[(parent_path, kind_)] = cls
         self._type_known.add(cls)
 
     def save_snapshot(self, value: Union["StoredStateData", "EventBase"]):
@@ -715,12 +716,13 @@ def load_snapshot(self, handle: Handle) -> '_Serializable':
         parent_path = None
         if handle.parent:
             parent_path = handle.parent.path
-        cls: Type[_Serializable] = self._type_registry.get((parent_path, handle.kind))
-        if not cls:
+        cls_or_none = self._type_registry.get((parent_path, handle.kind))
+        if not cls_or_none:
             raise NoTypeError(handle.path)
+        cls: Type[_Serializable] = cls_or_none
         data = self._storage.load_snapshot(handle.path)
         obj = cls.__new__(cls)
-        obj.framework = self
+        obj.framework = self  # type: ignore
         obj.handle = handle
         obj.restore(data)
         self._track(obj)
@@ -764,7 +766,7 @@ class SomeObject:
         event_kind = bound_event.event_kind
         emitter = bound_event.emitter
 
-        self.register_type(event_type, emitter, event_kind)
+        self.register_type(event_type, emitter, event_kind)  # type: ignore
 
         if hasattr(emitter, "handle"):
             emitter_path = emitter.handle.path
@@ -869,7 +871,7 @@ def _event_context(self, event_name: str):
 
         self._event_name = old_event_name
 
-    def _reemit(self, single_event_path: str = None):
+    def _reemit(self, single_event_path: Optional[str] = None):
         last_event_path = None
         deferred = True
         for event_path, observer_path, method_name in self._storage.notices(single_event_path):
@@ -924,7 +926,7 @@ def _reemit(self, single_event_path: str = None):
                 self._storage.drop_notice(event_path, observer_path, method_name)
             # We intentionally consider this event to be dead and reload it from
             # scratch in the next path.
-            self.framework._forget(event)
+            self.framework._forget(event)  # type: ignore
 
         if not deferred and last_event_path is not None:
             self._storage.drop_snapshot(last_event_path)
@@ -949,7 +951,7 @@ def breakpoint(self, name: Optional[str] = None):
         """
         # If given, validate the name comply with all the rules
         if name is not None:
-            if not isinstance(name, str):  # pyright: reportUnnecessaryIsInstance=false
+            if not isinstance(name, str):
                 raise TypeError('breakpoint names must be strings')
             if name in ('hook', 'all'):
                 raise ValueError('breakpoint names "all" and "hook" are reserved')
@@ -1030,12 +1032,10 @@ class BoundStoredState:
     if TYPE_CHECKING:
         # to help the type checker and IDEs:
         @property
-        def _data(self) -> StoredStateData:
-            pass
+        def _data(self) -> StoredStateData: ...  # noqa, type: ignore
 
         @property
-        def _attr_name(self) -> str:
-            pass  # pyright: reportGeneralTypeIssues=false
+        def _attr_name(self) -> str: ...  # noqa, type: ignore
 
     def __init__(self, parent: Object, attr_name: str):
         parent.framework.register_type(StoredStateData, parent)
@@ -1054,7 +1054,7 @@ def __init__(self, parent: Object, attr_name: str):
 
     if TYPE_CHECKING:
         @typing.overload
-        def __getattr__(self, key: Literal['on']) -> ObjectEvents:
+        def __getattr__(self, key: Literal['on']) -> ObjectEvents:  # type: ignore
             pass
 
     def __getattr__(self, key: str) -> Union['_StorableType', 'StoredObject', ObjectEvents]:
@@ -1130,7 +1130,7 @@ def __get__(
             pass
 
     def __get__(self,
-                parent: '_ObjectType',
+                parent: Optional['_ObjectType'],
                 parent_type: 'Type[_ObjectType]') -> Union['StoredState',
                                                            BoundStoredState]:
         if self.parent_type is not None and self.parent_type not in parent_type.mro():
@@ -1195,13 +1195,13 @@ def _unwrap_stored(parent_data: StoredStateData,
                    value: Union['_StoredObject', '_StorableType']
                    ) -> '_StorableType':
     if isinstance(value, (StoredDict, StoredList, StoredSet)):
-        return value._under  # pyright: reportPrivateUsage=false
+        return value._under
     return typing.cast('_StorableType', value)
 
 
 def _wrapped_repr(obj: '_StoredObject') -> str:
     t = type(obj)
-    if obj._under:  # pyright: reportPrivateUsage=false
+    if obj._under:
         return f"{t.__module__}.{t.__name__}({obj._under!r})"  # type: ignore
     else:
         return f"{t.__module__}.{t.__name__}()"
diff --git a/ops/jujuversion.py b/ops/jujuversion.py
index bbdfaadd5..a042222d8 100755
--- a/ops/jujuversion.py
+++ b/ops/jujuversion.py
@@ -61,7 +61,7 @@ def __eq__(self, other: Union[str, 'JujuVersion']) -> bool:
             return True
         if isinstance(other, str):
             other = type(self)(other)
-        elif not isinstance(other, JujuVersion):  # pyright: reportUnnecessaryIsInstance=false
+        elif not isinstance(other, JujuVersion):
             raise RuntimeError(f'cannot compare Juju version "{self}" with "{other}"')
         return (
             self.major == other.major
diff --git a/ops/log.py b/ops/log.py
index b816a1eb7..9432b6f78 100644
--- a/ops/log.py
+++ b/ops/log.py
@@ -22,7 +22,7 @@
     from types import TracebackType
     from typing import Type
 
-    from ops.model import _ModelBackend  # pyright: reportPrivateUsage=false
+    from ops.model import _ModelBackend
 
 
 class JujuLogHandler(logging.Handler):
diff --git a/ops/main.py b/ops/main.py
index 88681fb04..c826938fb 100755
--- a/ops/main.py
+++ b/ops/main.py
@@ -376,7 +376,7 @@ def main(charm_class: Type[ops.charm.CharmBase],
     """
     charm_dir = _get_charm_dir()
 
-    model_backend = ops.model._ModelBackend()  # pyright: reportPrivateUsage=false
+    model_backend = ops.model._ModelBackend()
     debug = ('JUJU_DEBUG' in os.environ)
     setup_root_logging(model_backend, debug=debug)
     logger.debug("Operator Framework %s up and running.", ops.__version__)  # type:ignore
diff --git a/ops/model.py b/ops/model.py
index 08ee06a50..d6fe1e90a 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -25,13 +25,13 @@
 import re
 import shutil
 import stat
+import subprocess
 import tempfile
 import time
 import typing
 import weakref
 from abc import ABC, abstractmethod
 from pathlib import Path
-from subprocess import PIPE, CalledProcessError, run
 from typing import (
     Any,
     BinaryIO,
@@ -58,16 +58,6 @@
 from ops.jujuversion import JujuVersion
 
 if typing.TYPE_CHECKING:
-    from pebble import (  # pyright: reportMissingTypeStubs=false
-        CheckInfo,
-        CheckLevel,
-        Client,
-        ExecProcess,
-        FileInfo,
-        LayerDict,
-        Plan,
-        ServiceInfo,
-    )
     from typing_extensions import TypedDict
 
     from ops.framework import _SerializedData
@@ -90,7 +80,7 @@
     _StatusDict = TypedDict('_StatusDict', {'status': str, 'message': str})
 
     # the data structure we can use to initialize pebble layers with.
-    _Layer = Union[str, LayerDict, pebble.Layer]
+    _Layer = Union[str, pebble.LayerDict, pebble.Layer]
 
     # mapping from relation name to a list of relation objects
     _RelationMapping_Raw = Dict[str, Optional[List['Relation']]]
@@ -757,7 +747,7 @@ def _get_unique(self, relation_name: str, relation_id: Optional[int] = None):
                                 self._our_unit, self._backend, self._cache)
         relations = self[relation_name]
         num_related = len(relations)
-        self._backend._validate_relation_access(  # pyright: reportPrivateUsage=false
+        self._backend._validate_relation_access(
             relation_name, relations)
         if num_related == 0:
             return None
@@ -1370,7 +1360,7 @@ def _hook_is_running(self) -> bool:
         # this flag controls whether the access we have to RelationDataContent
         # is 'strict' aka the same as a deployed charm would have, or whether it is
         # unrestricted, allowing test code to read/write databags at will.
-        return bool(self._backend._hook_is_running)  # pyright: reportPrivateUsage=false
+        return bool(self._backend._hook_is_running)
 
     def _load(self) -> '_RelationDataContent_Raw':
         """Load the data from the current entity / relation."""
@@ -1694,7 +1684,7 @@ def __init__(self, storage_names: Iterable[str], backend: '_ModelBackend'):
         self._storage_map: _StorageDictType = {storage_name: None
                                                for storage_name in storage_names}
 
-    def __contains__(self, key: str):  # pyright: reportIncompatibleMethodOverride=false
+    def __contains__(self, key: str):  # pyright: ignore[reportIncompatibleMethodOverride]
         return key in self._storage_map
 
     def __len__(self):
@@ -1816,18 +1806,13 @@ class Container:
     """
 
     def __init__(self, name: str, backend: '_ModelBackend',
-                 pebble_client: Optional['Client'] = None):
+                 pebble_client: Optional['pebble.Client'] = None):
         self.name = name
 
         if pebble_client is None:
             socket_path = f'/charm/containers/{name}/pebble.socket'
             pebble_client = backend.get_pebble(socket_path)
-        self._pebble: 'Client' = pebble_client
-
-    @property
-    def pebble(self) -> 'Client':
-        """The low-level :class:`ops.pebble.Client` instance for this container."""
-        return self._pebble
+        self._pebble: 'pebble.Client' = pebble_client
 
     def can_connect(self) -> bool:
         """Report whether the Pebble API is reachable in the container.
@@ -1925,7 +1910,7 @@ def add_layer(self, label: str, layer: '_Layer', *, combine: bool = False):
         """
         self._pebble.add_layer(label, layer, combine=combine)
 
-    def get_plan(self) -> 'Plan':
+    def get_plan(self) -> 'pebble.Plan':
         """Get the combined Pebble configuration.
 
         This will immediately reflect changes from any previous
@@ -1944,7 +1929,7 @@ def get_services(self, *service_names: str) -> '_ServiceInfoMapping':
         services = self._pebble.get_services(names)
         return ServiceInfoMapping(services)
 
-    def get_service(self, service_name: str) -> 'ServiceInfo':
+    def get_service(self, service_name: str) -> 'pebble.ServiceInfo':
         """Get status information for a single named service.
 
         Raises :class:`ModelError` if service_name is not found.
@@ -1959,7 +1944,7 @@ def get_service(self, service_name: str) -> 'ServiceInfo':
     def get_checks(
             self,
             *check_names: str,
-            level: Optional['CheckLevel'] = None) -> 'CheckInfoMapping':
+            level: Optional['pebble.CheckLevel'] = None) -> 'CheckInfoMapping':
         """Fetch and return a mapping of check information indexed by check name.
 
         Args:
@@ -1971,7 +1956,7 @@ def get_checks(
         checks = self._pebble.get_checks(names=check_names or None, level=level)
         return CheckInfoMapping(checks)
 
-    def get_check(self, check_name: str) -> 'CheckInfo':
+    def get_check(self, check_name: str) -> 'pebble.CheckInfo':
         """Get check information for a single named check.
 
         Raises :class:`ModelError` if check_name is not found.
@@ -2040,7 +2025,7 @@ def push(self,
                           group_id=group_id, group=group)
 
     def list_files(self, path: StrOrPath, *, pattern: Optional[str] = None,
-                   itself: bool = False) -> List['FileInfo']:
+                   itself: bool = False) -> List['pebble.FileInfo']:
         """Return list of directory entries from given path on remote system.
 
         Despite the name, this method returns a list of files *and*
@@ -2206,7 +2191,7 @@ def pull_path(self,
             raise MultiPushPullError('failed to pull one or more files', errors)
 
     @staticmethod
-    def _build_fileinfo(path: StrOrPath) -> 'FileInfo':
+    def _build_fileinfo(path: StrOrPath) -> 'pebble.FileInfo':
         """Constructs a FileInfo object by stat'ing a local path."""
         path = Path(path)
         if path.is_symlink():
@@ -2235,8 +2220,8 @@ def _build_fileinfo(path: StrOrPath) -> 'FileInfo':
 
     @staticmethod
     def _list_recursive(list_func: Callable[[Path],
-                        Iterable['FileInfo']],
-                        path: Path) -> Generator['FileInfo', None, None]:
+                        Iterable['pebble.FileInfo']],
+                        path: Path) -> Generator['pebble.FileInfo', None, None]:
         """Recursively lists all files under path using the given list_func.
 
         Args:
@@ -2348,7 +2333,7 @@ def exec(
         stderr: Optional[Union[TextIO, BinaryIO]] = None,
         encoding: str = 'utf-8',
         combine_stderr: bool = False
-    ) -> 'ExecProcess':
+    ) -> 'pebble.ExecProcess':
         """Execute the given command on the remote system.
 
         See :meth:`ops.pebble.Client.exec` for documentation of the parameters
@@ -2387,6 +2372,12 @@ def send_signal(self, sig: Union[int, str], *service_names: str):
 
         self._pebble.send_signal(sig, service_names)
 
+    # Define this last to avoid clashes with the imported "pebble" module
+    @property
+    def pebble(self) -> 'pebble.Client':
+        """The low-level :class:`ops.pebble.Client` instance for this container."""
+        return self._pebble
+
 
 class ContainerMapping(Mapping[str, Container]):
     """Map of container names to Container objects.
@@ -2411,14 +2402,14 @@ def __repr__(self):
         return repr(self._containers)
 
 
-class ServiceInfoMapping(Mapping[str, 'ServiceInfo']):
+class ServiceInfoMapping(Mapping[str, 'pebble.ServiceInfo']):
     """Map of service names to :class:`ops.pebble.ServiceInfo` objects.
 
     This is done as a mapping object rather than a plain dictionary so that we
     can extend it later, and so it's not mutable.
     """
 
-    def __init__(self, services: Iterable['ServiceInfo']):
+    def __init__(self, services: Iterable['pebble.ServiceInfo']):
         self._services = {s.name: s for s in services}
 
     def __getitem__(self, key: str):
@@ -2434,14 +2425,14 @@ def __repr__(self):
         return repr(self._services)
 
 
-class CheckInfoMapping(Mapping[str, 'CheckInfo']):
+class CheckInfoMapping(Mapping[str, 'pebble.CheckInfo']):
     """Map of check names to :class:`ops.pebble.CheckInfo` objects.
 
     This is done as a mapping object rather than a plain dictionary so that we
     can extend it later, and so it's not mutable.
     """
 
-    def __init__(self, checks: Iterable['CheckInfo']):
+    def __init__(self, checks: Iterable['pebble.CheckInfo']):
         self._checks = {c.name: c for c in checks}
 
     def __getitem__(self, key: str):
@@ -2603,8 +2594,8 @@ def __init__(self, unit_name: Optional[str] = None,
 
     def _run(self, *args: str, return_output: bool = False,
              use_json: bool = False, input_stream: Optional[str] = None
-             ) -> Union[str, 'JsonObject', None]:
-        kwargs = dict(stdout=PIPE, stderr=PIPE, check=True, encoding='utf-8')
+             ) -> Union[str, 'JsonObject']:
+        kwargs = dict(stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True, encoding='utf-8')
         if input_stream:
             kwargs.update({"input": input_stream})
         which_cmd = shutil.which(args[0])
@@ -2613,19 +2604,21 @@ def _run(self, *args: str, return_output: bool = False,
         args = (which_cmd,) + args[1:]
         if use_json:
             args += ('--format=json',)
+        # TODO(benhoyt): all the "type: ignore"s below kinda suck, but I've
+        #                been fighting with Pyright for half an hour now...
         try:
-            result = run(args, **kwargs)
-        except CalledProcessError as e:
+            result = subprocess.run(args, **kwargs)  # type: ignore
+        except subprocess.CalledProcessError as e:
             raise ModelError(e.stderr)
         if return_output:
-            if result.stdout is None:
+            if result.stdout is None:  # type: ignore
                 return ''
             else:
-                text = typing.cast(str, result.stdout)
+                text: str = result.stdout  # type: ignore
                 if use_json:
-                    return json.loads(text)
+                    return json.loads(text)  # type: ignore
                 else:
-                    return text
+                    return text  # type: ignore
 
     @staticmethod
     def _is_relation_not_found(model_error: Exception) -> bool:
@@ -2925,7 +2918,7 @@ def add_metrics(self, metrics: Mapping[str, 'Numerical'],
         cmd.extend(metric_args)
         self._run(*cmd)
 
-    def get_pebble(self, socket_path: str) -> 'Client':
+    def get_pebble(self, socket_path: str) -> 'pebble.Client':
         """Create a pebble.Client instance from given socket path."""
         return pebble.Client(socket_path=socket_path)
 
@@ -3121,7 +3114,7 @@ def validate_metric_label(cls, label_name: str):
 
     @classmethod
     def format_metric_value(cls, value: 'Numerical'):
-        if not isinstance(value, (int, float)):  # pyright: reportUnnecessaryIsInstance=false
+        if not isinstance(value, (int, float)):
             raise ModelError('invalid metric value {!r} provided:'
                              ' must be a positive finite float'.format(value))
 
diff --git a/ops/pebble.py b/ops/pebble.py
index b5179657a..42f51719e 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -1305,10 +1305,10 @@ def _reader_to_websocket(reader: '_WebsocketReader',
 
 
 def _websocket_to_writer(ws: '_WebSocket', writer: '_WebsocketWriter',
-                         encoding: str):
+                         encoding: Optional[str]):
     """Receive messages from websocket (until end signal) and write to writer."""
     while True:
-        chunk: _StrOrBytes = ws.recv()
+        chunk: _StrOrBytes = typing.cast('_StrOrBytes', ws.recv())
 
         if isinstance(chunk, str):
             try:
@@ -1371,7 +1371,7 @@ def read(self, n: int = -1) -> '_StrOrBytes':
             return b''
 
         while not self.remaining:
-            chunk: _StrOrBytes = self.ws.recv()
+            chunk = typing.cast('_StrOrBytes', self.ws.recv())
 
             if isinstance(chunk, str):
                 try:
@@ -2318,7 +2318,7 @@ def send_signal(self, sig: Union[int, str], services: Iterable[str]):
             raise TypeError('services must be of type Iterable[str], '
                             'not {}'.format(type(services).__name__))
         for s in services:
-            if not isinstance(s, str):  # pyright: reportUnnecessaryIsInstance=false
+            if not isinstance(s, str):
                 raise TypeError(f'service names must be str, not {type(s).__name__}')
 
         if isinstance(sig, int):
diff --git a/ops/storage.py b/ops/storage.py
index b4478493d..bebeb0655 100755
--- a/ops/storage.py
+++ b/ops/storage.py
@@ -24,7 +24,7 @@
 from pathlib import Path
 from typing import Any, Callable, Generator, List, Optional, Tuple, Union
 
-import yaml  # pyright: reportMissingModuleSource=false
+import yaml  # pyright: ignore[reportMissingModuleSource]
 
 logger = logging.getLogger()
 
diff --git a/ops/testing.py b/ops/testing.py
index cdde47020..f2d4d5ddf 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -210,7 +210,7 @@ def _event_context(self, event_name: str):
         >>>         harness.charm.event_handler(event)
 
         """
-        return self._framework._event_context(event_name)  # pyright: reportPrivateUsage=false
+        return self._framework._event_context(event_name)
 
     def set_can_connect(self, container: Union[str, model.Container], val: bool):
         """Change the simulated connection status of a container's underlying Pebble client.
@@ -269,7 +269,7 @@ class TestCharm(self._charm_cls):  # type: ignore
         # Note: jam 2020-03-01 This is so that errors in testing say MyCharm has no attribute foo,
         # rather than TestCharm has no attribute foo.
         TestCharm.__name__ = self._charm_cls.__name__
-        self._charm = TestCharm(self._framework)
+        self._charm = TestCharm(self._framework)  # type: ignore
 
     def begin_with_initial_hooks(self) -> None:
         """Called when you want the Harness to fire the same hooks that Juju would fire at startup.
@@ -359,7 +359,7 @@ def begin_with_initial_hooks(self) -> None:
         post_setup_sts = self._backend.status_get()
         if post_setup_sts.get("status") == "maintenance" and not post_setup_sts.get("message"):
             self._backend.status_set("unknown", "", is_app=False)
-        all_ids = list(self._backend._relation_names.items())  # pyright:ReportPrivateUsage=false
+        all_ids = list(self._backend._relation_names.items())
         random.shuffle(all_ids)
         for rel_id, rel_name in all_ids:
             rel_app_and_units = self._backend._relation_app_and_units[rel_id]
@@ -439,7 +439,7 @@ def _get_config(self, charm_config: Optional['YAMLStringOrFile']):
         assert isinstance(charm_config, str)  # type guard
         config = yaml.safe_load(charm_config)
 
-        if not isinstance(config, dict):  # pyright: reportUnnecessaryIsInstance=false
+        if not isinstance(config, dict):
             raise TypeError(config)
         return cast('RawConfig', config)
 
@@ -587,12 +587,12 @@ def detach_storage(self, storage_id: str) -> None:
             raise RuntimeError('cannot detach storage before Harness is initialised')
         storage_name, storage_index = storage_id.split('/', 1)
         storage_index = int(storage_index)
-        storage_attached = self._backend._storage_is_attached(  # pyright:ReportPrivateUsage=false
+        storage_attached = self._backend._storage_is_attached(
             storage_name, storage_index)
         if storage_attached and self._hooks_enabled:
             self.charm.on[storage_name].storage_detaching.emit(
                 model.Storage(storage_name, storage_index, self._backend))
-        self._backend._storage_detach(storage_id)  # pyright:ReportPrivateUsage=false
+        self._backend._storage_detach(storage_id)
 
     def attach_storage(self, storage_id: str) -> None:
         """Attach a storage device.
@@ -637,12 +637,12 @@ def remove_storage(self, storage_id: str) -> None:
         if storage_name not in self._meta.storages:
             raise RuntimeError(
                 f"the key '{storage_name}' is not specified as a storage key in metadata")
-        is_attached = self._backend._storage_is_attached(  # pyright:ReportPrivateUsage=false
+        is_attached = self._backend._storage_is_attached(
             storage_name, storage_index)
         if self._charm is not None and self._hooks_enabled and is_attached:
             self.charm.on[storage_name].storage_detaching.emit(
                 model.Storage(storage_name, storage_index, self._backend))
-        self._backend._storage_remove(storage_id)  # pyright:ReportPrivateUsage=false
+        self._backend._storage_remove(storage_id)
 
     def add_relation(self, relation_name: str, remote_app: str) -> int:
         """Declare that there is a new relation between this app and `remote_app`.
@@ -660,22 +660,22 @@ def add_relation(self, relation_name: str, remote_app: str) -> int:
             The relation_id created by this add_relation.
         """
         relation_id = self._next_relation_id()
-        self._backend._relation_ids_map.setdefault(  # pyright:ReportPrivateUsage=false
+        self._backend._relation_ids_map.setdefault(
             relation_name, []).append(relation_id)
         self._backend._relation_names[relation_id] = relation_name
-        self._backend._relation_list_map[relation_id] = []  # pyright:ReportPrivateUsage=false
-        self._backend._relation_data_raw[relation_id] = {  # pyright:ReportPrivateUsage=false
+        self._backend._relation_list_map[relation_id] = []
+        self._backend._relation_data_raw[relation_id] = {
             remote_app: {},
             self._backend.unit_name: {},
             self._backend.app_name: {}}
 
-        self._backend._relation_app_and_units[relation_id] = {  # pyright:ReportPrivateUsage=false
+        self._backend._relation_app_and_units[relation_id] = {
             "app": remote_app,
             "units": [],
         }
         # Reload the relation_ids list
         if self._model is not None:
-            self._model.relations._invalidate(relation_name)  # pyright:ReportPrivateUsage=false
+            self._model.relations._invalidate(relation_name)
         self._emit_relation_created(relation_name, relation_id, remote_app)
         return relation_id
 
@@ -688,25 +688,25 @@ def remove_relation(self, relation_id: int) -> None:
         Raises:
             RelationNotFoundError: if relation id is not valid
         """
-        rel_names = self._backend._relation_names   # pyright:ReportPrivateUsage=false
+        rel_names = self._backend._relation_names
         try:
             relation_name = rel_names[relation_id]
             remote_app = self._backend.relation_remote_app_name(relation_id)
         except KeyError as e:
             raise model.RelationNotFoundError from e
 
-        rel_list_map = self._backend._relation_list_map  # pyright:ReportPrivateUsage=false
+        rel_list_map = self._backend._relation_list_map
         for unit_name in rel_list_map[relation_id].copy():
             self.remove_relation_unit(relation_id, unit_name)
 
         self._emit_relation_broken(relation_name, relation_id, remote_app)
         if self._model is not None:
-            self._model.relations._invalidate(relation_name)  # pyright:ReportPrivateUsage=false
+            self._model.relations._invalidate(relation_name)
 
-        self._backend._relation_app_and_units.pop(relation_id)  # pyright:ReportPrivateUsage=false
-        self._backend._relation_data_raw.pop(relation_id)  # pyright:ReportPrivateUsage=false
+        self._backend._relation_app_and_units.pop(relation_id)
+        self._backend._relation_data_raw.pop(relation_id)
         rel_list_map.pop(relation_id)
-        ids_map = self._backend._relation_ids_map  # pyright:ReportPrivateUsage=false
+        ids_map = self._backend._relation_ids_map
         ids_map[relation_name].remove(relation_id)
         rel_names.pop(relation_id)
 
@@ -771,7 +771,7 @@ def add_relation_unit(self, relation_id: int, remote_unit_name: str) -> None:
                 'Remote unit name invalid: the remote application of {} is called {!r}; '
                 'the remote unit name should be {}/<some-number>, not {!r}.'
                 ''.format(relation_name, app.name, app.name, remote_unit_name))
-        app_and_units = self._backend._relation_app_and_units  # pyright: ReportPrivateUsage=false
+        app_and_units = self._backend._relation_app_and_units
         app_and_units[relation_id]["units"].append(remote_unit_name)
         # Make sure that the Model reloads the relation_list for this relation_id, as well as
         # reloading the relation data for this unit.
@@ -1576,7 +1576,7 @@ class _Secret:
     grants: Dict[int, Set[str]] = dataclasses.field(default_factory=dict)
 
 
-@_copy_docstrings(model._ModelBackend)  # pyright: reportPrivateUsage=false
+@_copy_docstrings(model._ModelBackend)
 @_record_calls
 class _TestingModelBackend:
     """This conforms to the interface for ModelBackend but provides canned data.
@@ -1840,7 +1840,7 @@ def _storage_detach(self, storage_id: str):
         index = int(index)
 
         for client in self._pebble_clients.values():
-            client._fs.remove_mount(name)  # pyright: ReportPrivateUsage=false
+            client._fs.remove_mount(name)
 
         if self._storage_is_attached(name, index):
             self._storage_attached[name].remove(index)
@@ -1857,7 +1857,7 @@ def _storage_attach(self, storage_id: str):
                 if mount.storage != name:
                     continue
                 for index, store in self._storage_list[mount.storage].items():
-                    fs = client._fs  # pyright: reportPrivateUsage=false
+                    fs = client._fs
                     fs.add_mount(mount.storage, mount.location, store['location'])
 
         index = int(index)
@@ -2203,7 +2203,7 @@ def __init__(self, backend: _TestingModelBackend):
         self._backend = backend
 
     def _check_connection(self):
-        if not self._backend._can_connect(self):  # pyright: reportPrivateUsage=false
+        if not self._backend._can_connect(self):
             msg = ('Cannot connect to Pebble; did you forget to call '
                    'begin_with_initial_hooks() or set_can_connect()?')
             raise pebble.ConnectionError(msg)
@@ -2828,7 +2828,7 @@ def list_dir(self, path: '_StringOrPath') -> List['_FileOrDir']:
                 raise FileNotFoundError(str(current_dir.path / token))
             if isinstance(current_dir, _File):
                 raise NotADirectoryError(str(current_dir.path))
-            if not isinstance(current_dir, _Directory):
+            if not isinstance(current_dir, _Directory):  # type: ignore
                 # For now, ignoring other possible cases besides File and Directory (e.g. Symlink).
                 raise NotImplementedError()
         return list(current_dir)
diff --git a/pyproject.toml b/pyproject.toml
index f0720054a..00d8c9efd 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -35,4 +35,7 @@ typeCheckingMode = "strict"
 reportIncompatibleMethodOverride = false
 reportImportCycles = false
 reportMissingModuleSource = false
+reportPrivateUsage = false
+reportUnnecessaryIsInstance = false
+reportUnnecessaryComparison = false
 stubPath = ""
diff --git a/requirements-dev.txt b/requirements-dev.txt
index 2be8be103..cc3958409 100644
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -7,7 +7,7 @@ flake8-builtins==2.1.0
 pyproject-flake8==4.0.1
 pep8-naming==0.13.2
 pytest==7.2.1
-pyright==1.1.291
+pyright==1.1.313
 pytest-operator==0.23.0
 coverage[toml]==7.0.5
 typing_extensions==4.2.0

From 237ca7af1b650dc9d85795c87e79b4bcf2877e6a Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Thu, 8 Jun 2023 15:52:57 +1200
Subject: [PATCH 3/8] Undo test commit

---
 requirements-dev.txt | 1 -
 1 file changed, 1 deletion(-)

diff --git a/requirements-dev.txt b/requirements-dev.txt
index cc3958409..320fe0324 100644
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -13,4 +13,3 @@ coverage[toml]==7.0.5
 typing_extensions==4.2.0
 
 -r requirements.txt
-

From 13a36e07008195281f9c721ddc97c6b9de37dd9e Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Thu, 8 Jun 2023 17:16:15 +1200
Subject: [PATCH 4/8] Avoid failing fast to see how many Observability charms
 fails

---
 .github/workflows/observability-charm-tests.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.github/workflows/observability-charm-tests.yaml b/.github/workflows/observability-charm-tests.yaml
index 4a9bee43a..7a66f6b6f 100644
--- a/.github/workflows/observability-charm-tests.yaml
+++ b/.github/workflows/observability-charm-tests.yaml
@@ -7,6 +7,7 @@ jobs:
     runs-on: ubuntu-latest
 
     strategy:
+      fail-fast: false
       matrix:
         charm-repo:
           - "canonical/alertmanager-k8s-operator"

From 98c28c82d00b065f8fd4ca73629e0ecb785f6bb5 Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Fri, 9 Jun 2023 17:46:04 +1200
Subject: [PATCH 5/8] Fix issue with Pebble.exec and Container.exec types

Currently Pyright isn't (usually?) able to find the return type of
Container.exec, I think due to an import ordering thing? As such,
charms that use it get an Any type and static checks pass.

However, as soon as Pyright *can* find the return type, it's not really
correct, and we get errors like shown below.

We need to use Generic[AnyStr] and various overloads to ensure the
caller gets an ExecProcess[str] if they call exec() with "encoding" set
(the default), or ExecProcess[bytes] if they call exec() with
"encoding" set to None.

$ tox -e static-charm
static-charm: commands[0]> pyright /home/ben/w/grafana-k8s-operator/src
/home/ben/w/grafana-k8s-operator/src/charm.py
  /home/ben/w/grafana-k8s-operator/src/charm.py:929:28 - error: Argument of type "Literal['Version (\\d*\\.\\d*\\.\\d*)']" cannot be assigned to parameter "pattern" of type "bytes | Pattern[bytes]" in function "search"
    Type "Literal['Version (\\d*\\.\\d*\\.\\d*)']" cannot be assigned to type "bytes | Pattern[bytes]"
      "Literal['Version (\\d*\\.\\d*\\.\\d*)']" is incompatible with "bytes"
      "Literal['Version (\\d*\\.\\d*\\.\\d*)']" is incompatible with "Pattern[bytes]" (reportGeneralTypeIssues)
  /home/ben/w/grafana-k8s-operator/src/charm.py:929:56 - error: Argument of type "_StrOrBytes" cannot be assigned to parameter "string" of type "ReadableBuffer" in function "search"
    Type "_StrOrBytes" cannot be assigned to type "ReadableBuffer"
      Type "str" cannot be assigned to type "ReadableBuffer"
        "str" is incompatible with "ReadOnlyBuffer"
        "str" is incompatible with "bytearray"
        "str" is incompatible with "memoryview"
        "str" is incompatible with "array[Any]"
        "str" is incompatible with "mmap"
        "str" is incompatible with "_CData"
    ... (reportGeneralTypeIssues)
2 errors, 0 warnings, 0 informations
---
 ops/model.py  | 48 +++++++++++++++++++++++++--
 ops/pebble.py | 90 +++++++++++++++++++++++++++++++++++----------------
 2 files changed, 108 insertions(+), 30 deletions(-)

diff --git a/ops/model.py b/ops/model.py
index d6fe1e90a..8a3aab15a 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -2317,7 +2317,9 @@ def remove_path(self, path: str, *, recursive: bool = False):
         """
         self._pebble.remove_path(path, recursive=recursive)
 
-    def exec(
+    # Exec I/O is str if encoding is provided (the default)
+    @typing.overload
+    def exec(  # noqa
         self,
         command: List[str],
         *,
@@ -2333,7 +2335,47 @@ def exec(
         stderr: Optional[Union[TextIO, BinaryIO]] = None,
         encoding: str = 'utf-8',
         combine_stderr: bool = False
-    ) -> 'pebble.ExecProcess':
+    ) -> pebble.ExecProcess[str]:
+        ...
+
+    # Exec I/O is bytes if encoding is explicitly set to None
+    @typing.overload
+    def exec(  # noqa
+        self,
+        command: List[str],
+        *,
+        environment: Optional[Dict[str, str]] = None,
+        working_dir: Optional[str] = None,
+        timeout: Optional[float] = None,
+        user_id: Optional[int] = None,
+        user: Optional[str] = None,
+        group_id: Optional[int] = None,
+        group: Optional[str] = None,
+        stdin: Optional[Union[str, bytes, TextIO, BinaryIO]] = None,
+        stdout: Optional[Union[TextIO, BinaryIO]] = None,
+        stderr: Optional[Union[TextIO, BinaryIO]] = None,
+        encoding: None = None,
+        combine_stderr: bool = False
+    ) -> pebble.ExecProcess[bytes]:
+        ...
+
+    def exec(
+        self,
+        command: List[str],
+        *,
+        environment: Optional[Dict[str, str]] = None,
+        working_dir: Optional[str] = None,
+        timeout: Optional[float] = None,
+        user_id: Optional[int] = None,
+        user: Optional[str] = None,
+        group_id: Optional[int] = None,
+        group: Optional[str] = None,
+        stdin: Optional[Union[str, bytes, TextIO, BinaryIO]] = None,
+        stdout: Optional[Union[TextIO, BinaryIO]] = None,
+        stderr: Optional[Union[TextIO, BinaryIO]] = None,
+        encoding: Optional[str] = 'utf-8',
+        combine_stderr: bool = False
+    ) -> pebble.ExecProcess[Any]:
         """Execute the given command on the remote system.
 
         See :meth:`ops.pebble.Client.exec` for documentation of the parameters
@@ -2351,7 +2393,7 @@ def exec(
             stdin=stdin,
             stdout=stdout,
             stderr=stderr,
-            encoding=encoding,
+            encoding=encoding,  # type: ignore
             combine_stderr=combine_stderr,
         )
 
diff --git a/ops/pebble.py b/ops/pebble.py
index 42f51719e..1fef52cf9 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -44,10 +44,12 @@
 from typing import (
     TYPE_CHECKING,
     Any,
+    AnyStr,
     BinaryIO,
     Callable,
     Dict,
     Generator,
+    Generic,
     Iterable,
     List,
     Optional,
@@ -85,14 +87,6 @@ def read(self, __n: int = ...) -> typing.AnyStr: ...  # for BinaryIO  # noqa
         def write(self, __s: typing.AnyStr) -> int: ...  # noqa
         def __enter__(self) -> typing.IO[typing.AnyStr]: ...  # noqa
 
-    class _Readable(Protocol):
-        def read(self, n: int = -1) -> _StrOrBytes: ...  # noqa
-
-    class _Writeable(Protocol):
-        # We'd need something like io.ReadableBuffer here,
-        # but we can't import that type
-        def write(self, buf: Union[bytes, str, bytearray]) -> int: ...  # noqa
-
     _AnyStrFileLikeIO = Union[_FileLikeIO[bytes], _FileLikeIO[str]]
     _TextOrBinaryIO = Union[TextIO, BinaryIO]
     _IOSource = Union[str, bytes, _AnyStrFileLikeIO]
@@ -366,7 +360,7 @@ def __repr__(self):
         return f'ChangeError({self.err!r}, {self.change!r})'
 
 
-class ExecError(Error):
+class ExecError(Error, Generic[AnyStr]):
     """Raised when a :meth:`Client.exec` command returns a non-zero exit code.
 
     Attributes:
@@ -387,8 +381,8 @@ def __init__(
         self,
         command: List[str],
         exit_code: int,
-        stdout: Optional['_StrOrBytes'],
-        stderr: Optional['_StrOrBytes'],
+        stdout: Optional[AnyStr],
+        stderr: Optional[AnyStr],
     ):
         self.command = command
         self.exit_code = exit_code
@@ -1105,7 +1099,7 @@ def __repr__(self):
                 ).format(self=self)
 
 
-class ExecProcess:
+class ExecProcess(Generic[AnyStr]):
     """Represents a process started by :meth:`Client.exec`.
 
     To avoid deadlocks, most users should use :meth:`wait_output` instead of
@@ -1134,9 +1128,9 @@ class ExecProcess:
 
     def __init__(
         self,
-        stdin: Optional['_Readable'],
-        stdout: Optional['_Writeable'],
-        stderr: Optional['_Writeable'],
+        stdin: Optional[Union[io.BytesIO, io.StringIO]],
+        stdout: Optional[Union[io.BytesIO, io.StringIO]],
+        stderr: Optional[Union[io.BytesIO, io.StringIO]],
         client: 'Client',
         timeout: Optional[float],
         control_ws: '_WebSocket',
@@ -1219,7 +1213,7 @@ def _wait(self) -> int:
             exit_code = change.tasks[0].data.get('exit-code', -1)
         return exit_code
 
-    def wait_output(self) -> Tuple['_StrOrBytes', Optional['_StrOrBytes']]:
+    def wait_output(self) -> Tuple[AnyStr, Optional[AnyStr]]:
         """Wait for the process to finish and return tuple of (stdout, stderr).
 
         If a timeout was specified to the :meth:`Client.exec` call, this waits
@@ -1246,10 +1240,10 @@ def wait_output(self) -> Tuple['_StrOrBytes', Optional['_StrOrBytes']]:
 
         exit_code: int = self._wait()
 
-        out_value: '_StrOrBytes' = out.getvalue()
-        err_value: Optional['_StrOrBytes'] = err.getvalue() if err is not None else None
+        out_value = typing.cast(AnyStr, out.getvalue())
+        err_value = typing.cast(AnyStr, err.getvalue()) if err is not None else None
         if exit_code != 0:
-            raise ExecError(self._command, exit_code, out_value, err_value)
+            raise ExecError[AnyStr](self._command, exit_code, out_value, err_value)
 
         return (out_value, err_value)
 
@@ -2049,6 +2043,48 @@ def remove_path(self, path: str, *, recursive: bool = False):
         resp = self._request('POST', '/v1/files', None, body)
         self._raise_on_path_error(typing.cast('_FilesResponse', resp), path)
 
+    # Exec I/O is str if encoding is provided (the default)
+    @typing.overload
+    def exec(  # noqa
+        self,
+        command: List[str],
+        *,
+        environment: Optional[Dict[str, str]] = None,
+        working_dir: Optional[str] = None,
+        timeout: Optional[float] = None,
+        user_id: Optional[int] = None,
+        user: Optional[str] = None,
+        group_id: Optional[int] = None,
+        group: Optional[str] = None,
+        stdin: Optional[Union[str, bytes, TextIO, BinaryIO]] = None,
+        stdout: Optional[Union[TextIO, BinaryIO]] = None,
+        stderr: Optional[Union[TextIO, BinaryIO]] = None,
+        encoding: str = 'utf-8',
+        combine_stderr: bool = False
+    ) -> ExecProcess[str]:
+        ...
+
+    # Exec I/O is bytes if encoding is explicitly set to None
+    @typing.overload
+    def exec(  # noqa
+        self,
+        command: List[str],
+        *,
+        environment: Optional[Dict[str, str]] = None,
+        working_dir: Optional[str] = None,
+        timeout: Optional[float] = None,
+        user_id: Optional[int] = None,
+        user: Optional[str] = None,
+        group_id: Optional[int] = None,
+        group: Optional[str] = None,
+        stdin: Optional[Union[str, bytes, TextIO, BinaryIO]] = None,
+        stdout: Optional[Union[TextIO, BinaryIO]] = None,
+        stderr: Optional[Union[TextIO, BinaryIO]] = None,
+        encoding: None = None,
+        combine_stderr: bool = False
+    ) -> ExecProcess[bytes]:
+        ...
+
     def exec(
         self,
         command: List[str],
@@ -2060,12 +2096,12 @@ def exec(
         user: Optional[str] = None,
         group_id: Optional[int] = None,
         group: Optional[str] = None,
-        stdin: Optional['_IOSource'] = None,
-        stdout: Optional['_TextOrBinaryIO'] = None,
-        stderr: Optional['_TextOrBinaryIO'] = None,
+        stdin: Optional[Union[str, bytes, TextIO, BinaryIO]] = None,
+        stdout: Optional[Union[TextIO, BinaryIO]] = None,
+        stderr: Optional[Union[TextIO, BinaryIO]] = None,
         encoding: Optional[str] = 'utf-8',
         combine_stderr: bool = False
-    ) -> ExecProcess:
+    ) -> ExecProcess[Any]:
         r"""Execute the given command on the remote system.
 
         Most of the parameters are explained in the "Parameters" section
@@ -2271,10 +2307,10 @@ def _cancel_stdin():
                     process_stderr = io.TextIOWrapper(
                         process_stderr, encoding=encoding, newline='')  # type: ignore
 
-        process = ExecProcess(
-            stdin=process_stdin,
-            stdout=process_stdout,  # type: ignore # doesn't like _Writeable
-            stderr=process_stderr,  # type: ignore # doesn't like _Writeable
+        process: ExecProcess[Any] = ExecProcess(
+            stdin=process_stdin,  # type: ignore
+            stdout=process_stdout,  # type: ignore
+            stderr=process_stderr,  # type: ignore
             client=self,
             timeout=timeout,
             stdio_ws=stdio_ws,

From 5943b3f96a6ea445d196567dd0200010700c363a Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Fri, 9 Jun 2023 17:54:42 +1200
Subject: [PATCH 6/8] Remove now-unnecessary quotes around 'pebble.X' types

---
 ops/model.py | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/ops/model.py b/ops/model.py
index 8a3aab15a..3ecbdc48e 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1806,13 +1806,13 @@ class Container:
     """
 
     def __init__(self, name: str, backend: '_ModelBackend',
-                 pebble_client: Optional['pebble.Client'] = None):
+                 pebble_client: Optional[pebble.Client] = None):
         self.name = name
 
         if pebble_client is None:
             socket_path = f'/charm/containers/{name}/pebble.socket'
             pebble_client = backend.get_pebble(socket_path)
-        self._pebble: 'pebble.Client' = pebble_client
+        self._pebble: pebble.Client = pebble_client
 
     def can_connect(self) -> bool:
         """Report whether the Pebble API is reachable in the container.
@@ -1910,7 +1910,7 @@ def add_layer(self, label: str, layer: '_Layer', *, combine: bool = False):
         """
         self._pebble.add_layer(label, layer, combine=combine)
 
-    def get_plan(self) -> 'pebble.Plan':
+    def get_plan(self) -> pebble.Plan:
         """Get the combined Pebble configuration.
 
         This will immediately reflect changes from any previous
@@ -1929,7 +1929,7 @@ def get_services(self, *service_names: str) -> '_ServiceInfoMapping':
         services = self._pebble.get_services(names)
         return ServiceInfoMapping(services)
 
-    def get_service(self, service_name: str) -> 'pebble.ServiceInfo':
+    def get_service(self, service_name: str) -> pebble.ServiceInfo:
         """Get status information for a single named service.
 
         Raises :class:`ModelError` if service_name is not found.
@@ -1944,7 +1944,7 @@ def get_service(self, service_name: str) -> 'pebble.ServiceInfo':
     def get_checks(
             self,
             *check_names: str,
-            level: Optional['pebble.CheckLevel'] = None) -> 'CheckInfoMapping':
+            level: Optional[pebble.CheckLevel] = None) -> 'CheckInfoMapping':
         """Fetch and return a mapping of check information indexed by check name.
 
         Args:
@@ -1956,7 +1956,7 @@ def get_checks(
         checks = self._pebble.get_checks(names=check_names or None, level=level)
         return CheckInfoMapping(checks)
 
-    def get_check(self, check_name: str) -> 'pebble.CheckInfo':
+    def get_check(self, check_name: str) -> pebble.CheckInfo:
         """Get check information for a single named check.
 
         Raises :class:`ModelError` if check_name is not found.
@@ -2025,7 +2025,7 @@ def push(self,
                           group_id=group_id, group=group)
 
     def list_files(self, path: StrOrPath, *, pattern: Optional[str] = None,
-                   itself: bool = False) -> List['pebble.FileInfo']:
+                   itself: bool = False) -> List[pebble.FileInfo]:
         """Return list of directory entries from given path on remote system.
 
         Despite the name, this method returns a list of files *and*
@@ -2191,7 +2191,7 @@ def pull_path(self,
             raise MultiPushPullError('failed to pull one or more files', errors)
 
     @staticmethod
-    def _build_fileinfo(path: StrOrPath) -> 'pebble.FileInfo':
+    def _build_fileinfo(path: StrOrPath) -> pebble.FileInfo:
         """Constructs a FileInfo object by stat'ing a local path."""
         path = Path(path)
         if path.is_symlink():
@@ -2220,8 +2220,8 @@ def _build_fileinfo(path: StrOrPath) -> 'pebble.FileInfo':
 
     @staticmethod
     def _list_recursive(list_func: Callable[[Path],
-                        Iterable['pebble.FileInfo']],
-                        path: Path) -> Generator['pebble.FileInfo', None, None]:
+                        Iterable[pebble.FileInfo]],
+                        path: Path) -> Generator[pebble.FileInfo, None, None]:
         """Recursively lists all files under path using the given list_func.
 
         Args:
@@ -2416,7 +2416,7 @@ def send_signal(self, sig: Union[int, str], *service_names: str):
 
     # Define this last to avoid clashes with the imported "pebble" module
     @property
-    def pebble(self) -> 'pebble.Client':
+    def pebble(self) -> pebble.Client:
         """The low-level :class:`ops.pebble.Client` instance for this container."""
         return self._pebble
 
@@ -2444,14 +2444,14 @@ def __repr__(self):
         return repr(self._containers)
 
 
-class ServiceInfoMapping(Mapping[str, 'pebble.ServiceInfo']):
+class ServiceInfoMapping(Mapping[str, pebble.ServiceInfo]):
     """Map of service names to :class:`ops.pebble.ServiceInfo` objects.
 
     This is done as a mapping object rather than a plain dictionary so that we
     can extend it later, and so it's not mutable.
     """
 
-    def __init__(self, services: Iterable['pebble.ServiceInfo']):
+    def __init__(self, services: Iterable[pebble.ServiceInfo]):
         self._services = {s.name: s for s in services}
 
     def __getitem__(self, key: str):
@@ -2467,14 +2467,14 @@ def __repr__(self):
         return repr(self._services)
 
 
-class CheckInfoMapping(Mapping[str, 'pebble.CheckInfo']):
+class CheckInfoMapping(Mapping[str, pebble.CheckInfo]):
     """Map of check names to :class:`ops.pebble.CheckInfo` objects.
 
     This is done as a mapping object rather than a plain dictionary so that we
     can extend it later, and so it's not mutable.
     """
 
-    def __init__(self, checks: Iterable['pebble.CheckInfo']):
+    def __init__(self, checks: Iterable[pebble.CheckInfo]):
         self._checks = {c.name: c for c in checks}
 
     def __getitem__(self, key: str):
@@ -2960,7 +2960,7 @@ def add_metrics(self, metrics: Mapping[str, 'Numerical'],
         cmd.extend(metric_args)
         self._run(*cmd)
 
-    def get_pebble(self, socket_path: str) -> 'pebble.Client':
+    def get_pebble(self, socket_path: str) -> pebble.Client:
         """Create a pebble.Client instance from given socket path."""
         return pebble.Client(socket_path=socket_path)
 

From 64e16fc87d9578ac9caeafbb6a2e0fd952a40681 Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Mon, 12 Jun 2023 15:35:17 +1200
Subject: [PATCH 7/8] Fix typo in begin_with_initial_hooks docstring

---
 ops/testing.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ops/testing.py b/ops/testing.py
index f2d4d5ddf..f75d0ecdf 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -303,7 +303,7 @@ def begin_with_initial_hooks(self) -> None:
             harness.begin_with_initial_hooks()
             # This will cause
             # install, db-relation-created('postgresql'), leader-elected, config-changed, start
-            # db-relation-joined('postrgesql/0'), db-relation-changed('postgresql/0')
+            # db-relation-joined('postgresql/0'), db-relation-changed('postgresql/0')
             # To be fired.
         """
         self.begin()

From 143fb24a26fbf0bd83bede7280c7d2362eceb097 Mon Sep 17 00:00:00 2001
From: Ben Hoyt <ben.hoyt@canonical.com>
Date: Mon, 12 Jun 2023 16:30:59 +1200
Subject: [PATCH 8/8] Comment out alertmanager-k8s-operator CI for now

---
 .github/workflows/observability-charm-tests.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/observability-charm-tests.yaml b/.github/workflows/observability-charm-tests.yaml
index 7a66f6b6f..5199421ce 100644
--- a/.github/workflows/observability-charm-tests.yaml
+++ b/.github/workflows/observability-charm-tests.yaml
@@ -10,7 +10,8 @@ jobs:
       fail-fast: false
       matrix:
         charm-repo:
-          - "canonical/alertmanager-k8s-operator"
+# TODO(benhoyt): reinstate after https://github.com/canonical/alertmanager-k8s-operator-1/pull/1 is merged
+#          - "canonical/alertmanager-k8s-operator"
           - "canonical/prometheus-k8s-operator"
           - "canonical/grafana-k8s-operator"