Repository: canonical/operator (python)

Issue:
Align the exception raised from Harness when a service does not exist
Basically, we want to [fix this TODO](https://github.com/canonical/operator/blob/449793f9867eba11dc2e7e1c28ea3dca1e1da231/ops/testing.py#L3123) (and the related following ones).

When starting, stopping, restarting a service, if the service does not exist, then Harness will raise a `RuntimeError`, but if this happens outside of tests, then `ops.pebble.APIError` will be raised.

It's better if the Harness exceptions match the ones you normally get, so that the charm doesn't have to catch exceptions just to make test behaviour match the normal behaviour.

Note that Scenario also uses the ops.testing Pebble client, so this will also adjust Scenario as well.

Pull request:
fix: in tests, raise the same error as Pebble when an invalid service is provided
This PR addresses the TODO and notes in the start/stop/restart service methods of Harness (and via inheritance, Scenario).

* Raise an APIError rather than RuntimeError, with the same message as Pebble, when a service doesn't exist.
* Return a ChangeID, now that we're keeping track of these.

Also: check for an empty list of services and raise the same APIError as Pebble in that case.

Fixes #1310

Patch:
diff --git a/.github/workflows/publish-ops-scenario.yaml b/.github/workflows/publish-ops-scenario.yaml
index 1efe190..9ed2dd0 100644
--- a/.github/workflows/publish-ops-scenario.yaml
+++ b/.github/workflows/publish-ops-scenario.yaml
@@ -29,7 +29,7 @@ jobs:
         run: python -m build
         working-directory: ./testing
       - name: Attest build provenance
-        uses: actions/attest-build-provenance@v2.2.0
+        uses: actions/attest-build-provenance@v2.2.2
         with:
           subject-path: 'testing/dist/*'
       - name: Publish
diff --git a/.github/workflows/publish-ops.yaml b/.github/workflows/publish-ops.yaml
index a10037e..9c8c8a0 100644
--- a/.github/workflows/publish-ops.yaml
+++ b/.github/workflows/publish-ops.yaml
@@ -32,7 +32,7 @@ jobs:
       - name: Build
         run: python -m build
       - name: Attest build provenance
-        uses: actions/attest-build-provenance@v2.2.0
+        uses: actions/attest-build-provenance@v2.2.2
         with:
           subject-path: 'dist/*'
       - name: Publish
diff --git a/ops/_private/harness.py b/ops/_private/harness.py
index 6e18bac..7814deb 100644
--- a/ops/_private/harness.py
+++ b/ops/_private/harness.py
@@ -3213,75 +3213,102 @@ class _TestingPebbleClient:
                 self._new_perform_check(info)
         return self.autostart_services(timeout, delay)
 
+    def _new_service_change(self, kind: pebble.ChangeKind, services: List[str]) -> pebble.Change:
+        now = datetime.datetime.now()
+        if kind == pebble.ChangeKind.START:
+            action = 'Start'
+        elif kind == pebble.ChangeKind.STOP:
+            action = 'Stop'
+        elif kind == pebble.ChangeKind.RESTART:
+            action = 'Restart'
+        else:
+            raise ValueError(f'unknown kind {kind}')
+        summary = f'{action} service {services[0]}'
+        if len(services) > 1:
+            summary += f' and {len(services) - 1} more'
+        change = pebble.Change(
+            pebble.ChangeID(str(uuid.uuid4())),
+            kind.value,
+            summary=summary,
+            status=pebble.ChangeStatus.DONE.value,
+            tasks=[],
+            ready=True,
+            err=None,
+            spawn_time=now,
+            ready_time=now,
+        )
+        self._changes[change.id] = change
+        return change
+
     def start_services(
         self,
         services: List[str],
         timeout: float = 30.0,
         delay: float = 0.1,
-    ):
+    ) -> pebble.ChangeID:
         # A common mistake is to pass just the name of a service, rather than a list of services,
         # so trap that so it is caught quickly.
         if isinstance(services, str):
             raise TypeError(f'start_services should take a list of names, not just "{services}"')
-
+        if not services:
+            raise self._api_error(400, 'must specify services for start action')
         self._check_connection()
 
-        # Note: jam 2021-04-20 We don't implement ChangeID, but the default caller of this is
-        # Container.start() which currently ignores the return value
         known_services = self._render_services()
         # Names appear to be validated before any are activated, so do two passes
         for name in services:
             if name not in known_services:
-                # TODO: jam 2021-04-20 This needs a better error type
-                raise RuntimeError(f'400 Bad Request: service "{name}" does not exist')
+                raise self._api_error(400, f'cannot start services: service {name} does not exist')
         for name in services:
             self._service_status[name] = pebble.ServiceStatus.ACTIVE
+        return self._new_service_change(pebble.ChangeKind.START, services).id
 
     def stop_services(
         self,
         services: List[str],
         timeout: float = 30.0,
         delay: float = 0.1,
-    ):
-        # handle a common mistake of passing just a name rather than a list of names
+    ) -> pebble.ChangeID:
+        # Handle a common mistake of passing just a name rather than a list of names.
         if isinstance(services, str):
             raise TypeError(f'stop_services should take a list of names, not just "{services}"')
+        if not services:
+            raise self._api_error(400, 'must specify services for start action')
 
         self._check_connection()
 
-        # Note: jam 2021-04-20 We don't implement ChangeID, but the default caller of this is
-        # Container.stop() which currently ignores the return value
         known_services = self._render_services()
         for name in services:
             if name not in known_services:
-                # TODO: jam 2021-04-20 This needs a better error type
-                #  400 Bad Request: service "bal" does not exist
-                raise RuntimeError(f'400 Bad Request: service "{name}" does not exist')
+                raise self._api_error(400, f'cannot stop services: service {name} does not exist')
         for name in services:
             self._service_status[name] = pebble.ServiceStatus.INACTIVE
+        return self._new_service_change(pebble.ChangeKind.STOP, services).id
 
     def restart_services(
         self,
         services: List[str],
         timeout: float = 30.0,
         delay: float = 0.1,
-    ):
-        # handle a common mistake of passing just a name rather than a list of names
+    ) -> pebble.ChangeID:
+        # Handle a common mistake of passing just a name rather than a list of names.
         if isinstance(services, str):
             raise TypeError(f'restart_services should take a list of names, not just "{services}"')
+        if not services:
+            raise self._api_error(400, 'must specify services for start action')
 
         self._check_connection()
 
-        # Note: jam 2021-04-20 We don't implement ChangeID, but the default caller of this is
-        # Container.restart() which currently ignores the return value
         known_services = self._render_services()
         for name in services:
             if name not in known_services:
-                # TODO: jam 2021-04-20 This needs a better error type
-                #  400 Bad Request: service "bal" does not exist
-                raise RuntimeError(f'400 Bad Request: service "{name}" does not exist')
+                raise self._api_error(
+                    400,
+                    f'cannot restart services: service {name} does not exist',
+                )
         for name in services:
             self._service_status[name] = pebble.ServiceStatus.ACTIVE
+        return self._new_service_change(pebble.ChangeKind.RESTART, services).id
 
     def wait_change(
         self,
diff --git a/ops/charm.py b/ops/charm.py
index 1318435..77fb0a8 100644
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -1451,6 +1451,17 @@ class CharmMeta:
     assumes: 'JujuAssumes'
     """Juju features this charm requires."""
 
+    charm_user: Literal['root', 'sudoer', 'non-root']
+    """Type of user used to run the charm hook code.
+
+    The value of ``root`` ensures the charm runs as root. The value of
+    ``sudoer`` runs the charm as a user other than root with access to sudo to
+    elevate its privileges. ``non-root`` ensures the charm does not run as root
+    and also does not have ``sudo`` privileges.
+
+    .. jujuadded 3.6.0
+    """
+
     containers: Dict[str, 'ContainerMeta']
     """Container metadata for each defined container."""
 
@@ -1524,6 +1535,7 @@ class CharmMeta:
         # Note that metadata v2 does not define min-juju-version ('assumes'
         # should be used instead).
         self.min_juju_version = raw_.get('min-juju-version')
+        self.charm_user = raw_.get('charm-user', 'root')
         self.requires = {
             name: RelationMeta(RelationRole.requires, name, rel)
             for name, rel in raw_.get('requires', {}).items()


Test patch:
diff --git a/.github/workflows/test-publish-ops-scenario.yaml b/.github/workflows/test-publish-ops-scenario.yaml
index 39833cc..28c6045 100644
--- a/.github/workflows/test-publish-ops-scenario.yaml
+++ b/.github/workflows/test-publish-ops-scenario.yaml
@@ -26,7 +26,7 @@ jobs:
         run: python -m build
         working-directory: ./testing
       - name: Attest build provenance
-        uses: actions/attest-build-provenance@v2.2.0
+        uses: actions/attest-build-provenance@v2.2.2
         with:
           subject-path: 'testing/dist/*'
       - name: Publish to test.pypi.org
diff --git a/.github/workflows/test-publish-ops.yaml b/.github/workflows/test-publish-ops.yaml
index cd08316..43c6247 100644
--- a/.github/workflows/test-publish-ops.yaml
+++ b/.github/workflows/test-publish-ops.yaml
@@ -25,7 +25,7 @@ jobs:
       - name: Build
         run: python -m build
       - name: Attest build provenance
-        uses: actions/attest-build-provenance@v2.2.0
+        uses: actions/attest-build-provenance@v2.2.2
         with:
           subject-path: 'dist/*'
       - name: Publish to test.pypi.org
diff --git a/test/test_charm.py b/test/test_charm.py
index 934f5c3..b2c9461 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -1129,3 +1129,19 @@ assumes:
             ops.JujuAssumesCondition.ANY,
         ),
     ]
+
+
+@pytest.mark.parametrize('user', ['root', 'sudoer', 'non-root'])
+def test_meta_charm_user(user: str):
+    meta = ops.CharmMeta.from_yaml(f"""
+name: my-charm
+charm-user: {user}
+""")
+    assert meta.charm_user == user
+
+
+def test_meta_charm_user_default():
+    meta = ops.CharmMeta.from_yaml("""
+name: my-charm
+""")
+    assert meta.charm_user == 'root'
diff --git a/test/test_testing.py b/test/test_testing.py
index 4aa0f0b..9f370a2 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -4688,7 +4688,10 @@ class TestTestingPebbleClient:
                 command: '/bin/echo bar'
             """,
         )
-        client.start_services(['bar'])
+        change_id = client.start_services(['bar'])
+        change = client.get_change(change_id)
+        assert change.kind == pebble.ChangeKind.START.value
+        assert 'start' in change.summary.lower()
         infos = client.get_services()
         assert len(infos) == 2
         bar_info = infos[0]
@@ -4701,7 +4704,10 @@ class TestTestingPebbleClient:
         assert foo_info.name == 'foo'
         assert foo_info.startup == pebble.ServiceStartup.ENABLED
         assert foo_info.current == pebble.ServiceStatus.INACTIVE
-        client.stop_services(['bar'])
+        change_id = client.stop_services(['bar'])
+        change = client.get_change(change_id)
+        assert change.kind == pebble.ChangeKind.STOP.value
+        assert 'stop' in change.summary.lower()
         infos = client.get_services()
         bar_info = infos[0]
         assert bar_info.name == 'bar'
@@ -4772,8 +4778,7 @@ class TestTestingPebbleClient:
         assert infos == []
 
     def test_invalid_start_service(self, client: _TestingPebbleClient):
-        # TODO: jam 2021-04-20 This should become a better error
-        with pytest.raises(RuntimeError):
+        with pytest.raises(pebble.APIError):
             client.start_services(['unknown'])
 
     def test_start_service_str(self, client: _TestingPebbleClient):
@@ -4782,12 +4787,20 @@ class TestTestingPebbleClient:
         with pytest.raises(TypeError):
             client.start_services('unknown')
 
+    def test_start_service_no_services(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.start_services([])
+
     def test_stop_service_str(self, client: _TestingPebbleClient):
         # Start service takes a list of names, but it is really easy to accidentally pass just a
         # name
         with pytest.raises(TypeError):
             client.stop_services('unknown')
 
+    def test_stop_service_no_services(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.stop_services([])
+
     def test_mixed_start_service(self, client: _TestingPebbleClient):
         client.add_layer(
             'foo',
@@ -4800,8 +4813,7 @@ class TestTestingPebbleClient:
                 command: '/bin/echo foo'
             """,
         )
-        # TODO: jam 2021-04-20 better error type
-        with pytest.raises(RuntimeError):
+        with pytest.raises(pebble.APIError):
             client.start_services(['foo', 'unknown'])
         # foo should not be started
         infos = client.get_services()
@@ -4824,8 +4836,7 @@ class TestTestingPebbleClient:
             """,
         )
         client.autostart_services()
-        # TODO: jam 2021-04-20 better error type
-        with pytest.raises(RuntimeError):
+        with pytest.raises(pebble.APIError):
             client.stop_services(['foo', 'unknown'])
         # foo should still be running
         infos = client.get_services()
@@ -4901,6 +4912,20 @@ class TestTestingPebbleClient:
         assert foo_info.startup == pebble.ServiceStartup.ENABLED
         assert foo_info.current == pebble.ServiceStatus.INACTIVE
 
+    def test_invalid_restart_service(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.restart_services(['unknown'])
+
+    def test_restart_service_str(self, client: _TestingPebbleClient):
+        # Restart service takes a list of names, but it is really easy to
+        # accidentally pass just a name.
+        with pytest.raises(TypeError):
+            client.restart_services('unknown')
+
+    def test_restart_service_no_services(self, client: _TestingPebbleClient):
+        with pytest.raises(pebble.APIError):
+            client.restart_services([])
+
     @unittest.skipUnless(is_linux, 'Pebble runs on Linux')
     def test_send_signal(self, client: _TestingPebbleClient):
         client.add_layer(