Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Pebble enum|str types to just enum #1287

Open
benhoyt opened this issue Jul 18, 2024 · 1 comment
Open

Simplify Pebble enum|str types to just enum #1287

benhoyt opened this issue Jul 18, 2024 · 1 comment
Assignees
Labels
24.10 low priority Non-urgent issue we may (or may not) consider later small item

Comments

@benhoyt
Copy link
Collaborator

benhoyt commented Jul 18, 2024

Per our discussion at Charm Tech daily sync today, we'd like to simplify the various pebble enum types from Union[EnumType | str] to just EnumType, for example, ServiceInfo.current now has type Union[ServiceStatus, str], which can be awkward for us (and for charms) to deal with.

The original reason I made them |str was so that if a charm is deployed to a newer version of Juju, which has a newer version of Pebble that add a new value to the enum, code won't fail when the enum is being created. To create the value we have code like this:

        try:
            current = ServiceStatus(d['current'])
        except ValueError:
            current = d['current']

But that means code that uses these fields have to have awkward types and sometimes have to do awkward things like use(info.current) if isinstance(info.current, str) else use(info.current.value).

So we'd like to simplify the type of these to just the enum type, for example just ServiceStatus in this example.

To avoid the charm code raising an exception, we want to add a new value UNKNOWN = 'unknown' to all the enum types that are used in this way. If we got an invalid or unknown string value from Pebble, we'd set it to that instead, and give a warning. The code to create the value from Pebble JSON would become something like:

        try:
            current = ServiceStatus(d['current'])
        except ValueError:
            warnings.warn(f'Unknown ServiceStatus value {d["current"]!r}')
            current = ServiceStatus.UNKNOWN

(Note that FileType already has an UNKNOWN value, which is handy.)

@benhoyt benhoyt added small item low priority Non-urgent issue we may (or may not) consider later 24.10 labels Jul 18, 2024
@benhoyt
Copy link
Collaborator Author

benhoyt commented Sep 24, 2024

@james-garner-canonical Assigning this to you to see if you can squeeze it in before The Hague.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
24.10 low priority Non-urgent issue we may (or may not) consider later small item
Projects
None yet
Development

No branches or pull requests

2 participants