feat: remove pebble enum | str unions #1400

james-garner-canonical · 2024-09-27T07:11:37Z

Resolving #1287, this PR removes the use of str and enum member unions connected to the pebble module.

Generally this just involves typing dicts generated by pebble as containing str values, and using the enum types exclusively in ops. The major change is that unknown values from pebble result in an UNKNOWN member of the enum, rather than being passed around as strs.

Reviewers will notice some block comments calling attention to potential issues with the current code, questions about my design choices, and suggestions for further changes. Feedback on these areas would be especially appreciated.

james-garner-canonical · 2024-09-27T07:12:20Z

ops/pebble.py

+            raise ValueError(
+                'self.current must be a ServiceStatus member (e.g. ServiceStatus.ACTIVE)'
+                f', not {self.current}'
+            )
        return self.current == ServiceStatus.ACTIVE


This comparison would be silently False if self.current was accidentally a str.

Would a better solution be to switch to using enum.StrEnum instead of enum.Enum? This would seem to match the intent of the original code better. With StrEnum, ServiceStatus.ACTIVE == 'active', so passing around strings instead of enum members would silently succeed instead of silently failing in the absence of a type check.

StrEnum is a py 3.11 feature, isn't it?

We've got to support py 3.8 because Juju supports specific Ubuntu releases as bases and that's where the Python interpreter comes from.

Yeah, agreed. Unfortunately we can't use StrEnum yet.

Good catch, @dimaqq, thanks.

james-garner-canonical · 2024-09-27T07:14:42Z

ops/pebble.py

+            self.level = CheckLevel.UNKNOWN
+        #
+        # these are all Optional in CheckDict and here, why '' instead of None?
+        # note that all falsey values are filtered out in to_dict
        self.period: Optional[str] = dct.get('period', '')
        self.timeout: Optional[str] = dct.get('timeout', '')
        self.threshold: Optional[int] = dct.get('threshold')


In general a lot of values are marked as Optional when they could perhaps be simply left out of the TypedDict. I haven't fully checked what the values received from Pebble for these dicts can be -- can these items be the go version of None (null? nil?). It seems like if so, we should use None instead of '', and if not we shouldn't use Optional.

Hmm, I'd have to look a bit closer, but I think these will be missing or str in the YAML. So that probably means NotRequired in the TypedDict? In any case, if you want to address this, let's do it in that separate PR.

james-garner-canonical · 2024-09-27T07:15:07Z

ops/pebble.py

+        # so do we want to just use 'unknown' for CheckLevel.UNKNOWN here?
+        # we still get a warning on making `c1` but not on `c2`, but maybe that's ok
+        # but is it useful to have 'unknown' in the dict?
+        # It'll error if that value makes it to pebble, right?
        fields = [


I'm not really sure what behaviour we want here with CheckLevel.UNKNOWN.

james-garner-canonical · 2024-09-27T07:15:58Z

ops/pebble.py

+            # while we might want <some-unknown-value-string> to overwrite,
+            # would we ever want Checklevel.UNKNOWN to overwrite?
+            if value is CheckLevel.UNKNOWN:
+                continue


I think we should just use other.to_dict() here, is there a reason we weren't already?

I see why you want to use other.to_dict() here, but it doesn't seem the right thing here: we're merging two instances of the class, so it seems weird to convert one to a dict first.

Let's just avoid the special case here and the unknown will be merged (and probably eventually fail at the Pebble level). That seems okay here.

james-garner-canonical · 2024-09-27T07:16:51Z

ops/pebble.py

@@ -3046,6 +3105,9 @@ def get_checks(
        """
        query: Dict[str, Any] = {}
        if level is not None:
+            # is this bad now that value could be 'unknown'?
+            # should we explicitly avoid it here?
+            # if level is not None and level.value is not CheckLevel.UNKNOWN:
            query['level'] = level.value


Another case where it's not clear how to handle CheckLevel.UNKNOWN on the outgoing side.

james-garner-canonical · 2024-09-27T07:17:37Z

ops/pebble.py

@@ -3098,7 +3160,7 @@ def get_notices(
        *,
        users: Optional[NoticesUsers] = None,
        user_id: Optional[int] = None,
-        types: Optional[Iterable[Union[NoticeType, str]]] = None,
+        types: Optional[Iterable[NoticeType]] = None,
        keys: Optional[Iterable[str]] = None,
    ) -> List[Notice]:
        """Query for notices that match all of the provided filters.


I wonder if this will break any users.

There's not a single user of .get_notices() yet.
Source: charm-analysis and github search.

Yep, I'm comfortable with this. Use of notice events themselves are rare; I'd be surprised if anyone actually needed get_notices even if they were using notices. The charm normally wants the event, not to query for them.

james-garner-canonical · 2024-09-27T07:18:06Z

test/test_pebble.py

-        assert info.type == 'foobar'
+        with pytest.warns(UserWarning):
+            info = pebble.FileInfo.from_dict(d)
+        assert info.type == pebble.FileType.UNKNOWN


Should we be using a different kind of warning here?

Nah, the default of UserWarning seems fine.

james-garner-canonical · 2024-09-27T07:20:33Z

ops/_private/harness.py

@@ -3863,7 +3863,7 @@ def _notice_matches(
        # For example: if user_id filter is set and it doesn't match, return False.
        if user_id is not None and not (notice.user_id is None or user_id == notice.user_id):
            return False
-        if types is not None and notice.type not in types:
+        if types is not None and notice.type.value not in types:
            return False


types is a collection of strs. I think this is mimicking the logic of pebble, so I think it makes sense for us to use notice.type.value here. Was this always silently broken? Or silently worked because notice.type was often a str -- while now it's supposed to always be an enum member?

I think you can answer that question, as notices are not really used by charmers yet.
Which means that the question can be rephrased as "do we have a test for this harness feature?" and "does that test actually work?".

Yeah, this was almost certainly silently broken. I should have written a test for this. Can you please add a regression test in test_testing.py?

dimaqq · 2024-10-01T01:31:25Z

Only superficial comments from me, I didn't read the change properly yet.

benhoyt · 2024-10-01T01:46:14Z

ops/_private/harness.py

@@ -3863,7 +3863,7 @@ def _notice_matches(
        # For example: if user_id filter is set and it doesn't match, return False.
        if user_id is not None and not (notice.user_id is None or user_id == notice.user_id):
            return False
-        if types is not None and notice.type not in types:
+        if types is not None and notice.type.value not in types:
            return False


Yeah, this was almost certainly silently broken. I should have written a test for this. Can you please add a regression test in test_testing.py?

benhoyt · 2024-10-01T01:52:47Z

ops/pebble.py

+            raise ValueError(
+                'self.current must be a ServiceStatus member (e.g. ServiceStatus.ACTIVE)'
+                f', not {self.current}'
+            )


I think we should drop this runtime check, as 1) recently we've been leaning more heavily on static typing rather than runtime type checks, and 2) it can basically never happen, especially now, given that these objects are parsed via from_dict and not created by the user (though I realise we don't prevent that).

benhoyt · 2024-10-01T01:53:10Z

ops/pebble.py

+            raise ValueError(
+                'self.current must be a ServiceStatus member (e.g. ServiceStatus.ACTIVE)'
+                f', not {self.current}'
+            )
        return self.current == ServiceStatus.ACTIVE


Yeah, agreed. Unfortunately we can't use StrEnum yet.

benhoyt · 2024-10-01T01:55:23Z

ops/pebble.py

@@ -1098,11 +1107,16 @@ def __init__(self, name: str, raw: Optional[CheckDict] = None):
        self.name = name
        dct: CheckDict = raw or {}
        self.override: str = dct.get('override', '')
+        self.level: CheckLevel
+        dct_level = dct.get('level', '')


Nit: dct is confusing here. How about level_str or raw_level?

benhoyt · 2024-10-01T01:57:04Z

ops/pebble.py

@@ -1098,11 +1107,16 @@ def __init__(self, name: str, raw: Optional[CheckDict] = None):
        self.name = name
        dct: CheckDict = raw or {}
        self.override: str = dct.get('override', '')
+        self.level: CheckLevel


Is this line needed?

benhoyt · 2024-10-01T02:20:29Z

ops/pebble.py

@@ -3046,6 +3105,9 @@ def get_checks(
        """
        query: Dict[str, Any] = {}
        if level is not None:
+            # is this bad now that value could be 'unknown'?


I think do the same as earlier: put it through as the string "unknown" and let Pebble fail it (in practice it's extremely unlikely to happen, as we haven't added any new levels since they first came out).

benhoyt · 2024-10-01T02:21:27Z

ops/pebble.py

@@ -3098,7 +3160,7 @@ def get_notices(
        *,
        users: Optional[NoticesUsers] = None,
        user_id: Optional[int] = None,
-        types: Optional[Iterable[Union[NoticeType, str]]] = None,
+        types: Optional[Iterable[NoticeType]] = None,
        keys: Optional[Iterable[str]] = None,
    ) -> List[Notice]:
        """Query for notices that match all of the provided filters.


Yep, I'm comfortable with this. Use of notice events themselves are rare; I'd be surprised if anyone actually needed get_notices even if they were using notices. The charm normally wants the event, not to query for them.

benhoyt · 2024-10-01T02:22:09Z

test/test_pebble.py

-        assert info.type == 'foobar'
+        with pytest.warns(UserWarning):
+            info = pebble.FileInfo.from_dict(d)
+        assert info.type == pebble.FileType.UNKNOWN


Nah, the default of UserWarning seems fine.

benhoyt · 2024-10-01T02:25:10Z

test/test_pebble.py

-        for current in [pebble.ServiceStatus.INACTIVE, pebble.ServiceStatus.ERROR, 'other']:
+        for current in (
+            status for status in pebble.ServiceStatus if status is not pebble.ServiceStatus.ACTIVE
+        ):


The nested looping here is hard to read, especially split on multiple lines. I think let's either keep the explicit list, or do it on two lines, something like:

inactive_statuses = [status for status in ServiceStatus if status != ACTIVE] for current in inactive_statuses: ...

James and I discussed whether we should use "is" or "==" to compare enums, and mildly settled on "let's use ==" as it's what you'd do with regular values (and StrEnum or whatever).

benhoyt · 2024-10-01T02:25:47Z

test/test_pebble.py

+        s = pebble.ServiceInfo(
+            name='s',
+            startup=pebble.ServiceStartup.ENABLED,
+            current='active',  # pyright: ignore[reportArgumentType]


Per earlier comment, I think we should drop this runtime check and just let static type checking handle this case.

james-garner-canonical added 13 commits September 27, 2024 18:17

Remove enum unions from use in ServiceInfo

35a367d

Make Ruff happy about Yoda conditions, this commit will

1903918

Use 'unknown' string in stead of _Unknown sentinel for UNKNOWN in enums

2c38b88

Handle silent bad values in is_running

1c31c54

Add CheckLevel.UNKNOWN

c6e98a1

Remove CheckLevel | str union from CheckDict and Check

0814851

Remove CheckLevel | str union from CheckInfo and _CheckInfoDict

ebd06a4

Remove CheckStatus | str unions

5e00c53

Emit warnings when enums don't recognise values

1dc9e56

Catch expected warnings in pebble tests

5acfc27

Remove FileType | str unions

b70552b

Remove NoticeType | str unions

391ec65

Remove unneeded change and expand comments for review

e19792b

james-garner-canonical commented Sep 27, 2024

View reviewed changes

james-garner-canonical requested review from benhoyt and tonyandrewmeyer September 27, 2024 07:24

benhoyt reviewed Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove pebble enum | str unions #1400

feat: remove pebble enum | str unions #1400

james-garner-canonical commented Sep 27, 2024

james-garner-canonical Sep 27, 2024

james-garner-canonical Sep 29, 2024

dimaqq Oct 1, 2024

benhoyt Oct 1, 2024

james-garner-canonical Oct 1, 2024

james-garner-canonical Sep 27, 2024

benhoyt Oct 1, 2024

james-garner-canonical Sep 27, 2024

james-garner-canonical Sep 27, 2024

benhoyt Oct 1, 2024

james-garner-canonical Sep 27, 2024

james-garner-canonical Sep 27, 2024

dimaqq Oct 1, 2024

benhoyt Oct 1, 2024

james-garner-canonical Sep 27, 2024

benhoyt Oct 1, 2024

james-garner-canonical Sep 27, 2024

dimaqq Oct 1, 2024

benhoyt Oct 1, 2024

dimaqq commented Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

benhoyt Oct 1, 2024

feat: remove pebble enum | str unions #1400

Are you sure you want to change the base?

feat: remove pebble enum | str unions #1400

Conversation

james-garner-canonical commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimaqq commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment