feat(snap): add snap conf module

[james-garner-canonical]

This PR adds the snap library functionality using the /v2/snaps/{snap}/conf endpoint, mirroring the snap CLI commands snap get, snap set and snap unset.

[tonyandrewmeyer]

Code and tests seem fine. There are some API choices I'm unsure about.

[tonyandrewmeyer]

What's the motivation for the positional only? It does seem like kw would be unnecessary here, but forbidding it seems excessive unless there's a good reason.

[james-garner-canonical]

With *keys, you can only use snap as a keyword argument if you don't pass any keys. / makes snap positional-only uniformly in all cases.

This also ties into the unset safety: we require at least one key by using key: str, /, *keys: str, so snap is always required to be positional for unset.

I'm wondering if we should make snap a positional-only argument for all functions that accept a snap as their first argument. We can always reverse this decision and allow them to be specified as keyword arguments, but we can't go to form keyword permitted to positional-only in a backwards compatible way.

[tonyandrewmeyer]

It seems like this arbitrarily nested JSON, is that right? So presumably we can't narrow the rule past Any.

Do you think it's likely we would want to grow any sort of "load into this class" functionality in the future and want to think about that? Or more likely this is just simple types people would cast in reality? Same for set.

[tonyandrewmeyer]

"requested key is not set" does that mean no such key exists, or the key has a default value, or both? Can we make this clearer?

[tonyandrewmeyer]

Every top-level configuration and the things underneath it? Or just the top within going deeper somehow? (Like the way snap get mysnap shows {...}).

[tonyandrewmeyer]

The comment makes me think we had this discussion already. Did we decide to not have something for this until someone provided a use-case? x = s.get(y)[y] does seem a little annoying, but presumably we don't want the return type to change based on the number of args.

[james-garner-canonical]

We didn't talk about this previously. Agreed on not having the return type change based on number/type of args. I have no real objection to including this in the library itself, just trying to scope the initial 2.0 to just the essentials.

[tonyandrewmeyer]

I feel like we maybe already had this conversation, but I don't remember the conclusion or know how to go find it, so will possibly repeat.

I'm unsure about snap.get('mysnap', 'config.engine', 'config.user', 'cache'). I think if it was a class or partial, like mysnap.get or mysnap_get with an arbitrary number of config names I'd like it more. Maybe this will never gain more arguments, but if it does, then knowing at which point the regular args stop and config names start gets even worse. There's also a mismatch with set, which takes a dict rather than kwargs (to handle dots, I assume).

I lean towards a dict here too.

[james-garner-canonical]

I don't think we've talked about this before. I definitely see the ugliness you're highlighting with snap.get('mysnap', 'config.engine', 'config.user', 'cache'). For very long lists, I think it would be most common for them to be stored in a variable somewhere, so I think there's a natural limit where this becomes snap.get('mysnap', *mysnap_keys) based on each user's preferences.

Maybe this will never gain more arguments, but if it does, then knowing at which point the regular args stop and config names start gets even worse.

The function will never be able to grow any more positional arguments without breaking backwards compatibility, so any additional arguments would be keyword only. It would be snap.get('mysnap', 'config.engine', 'config.user', 'cache', foo=bar), which I think is easy to tell where the keys stop.

I lean towards a dict here too.

I don't think a dict makes sense since the leaf values don't mean anything, so there's inevitably some asymmetry with snap.set. It's also nice to avoid having to manually flatten the dict before passing the keys to the API to handle nested (dotted) keys expressed as {'config': {'engine': None, 'user': None}}.

It could be a Sequence[str] or Iterable[str], called like snap.get('mysnap', ['config.engine', 'config.user', 'cache']). I think this looks quite nice, but it introduces three warts:

1. A str is an Iterable[str] and a Sequence[str] too, so the type checker won't reject snap.get('mysnap', 'config.engine'). So we need to do something with that at runtime, even if it's just raising a TypeError or ValueError.
2. Calling with a single argument is a tiny bit uglier with this nested rather than flat design: snap.get('mysnap', ['config.engine']).
3. snap.get('mysnap', []) means "get me all the keys", which feels kind of weird.

If we disregard 2, then we could avoid 1 by requiring some concrete types, like list[str] or tuple[str], but that doesn't feel super Pythonic.

We could resolve both at once by explicitly allowing str, and special casing like keys = [keys] if isinstance(keys, str) else keys. There are a few points that pushed me towards *keys instead:

• simplicity of implementation (no special case)
• honesty of argument name: keys being a slight lie as an argument name since it can be a single flat key (not even a sequence of one key)
• clarity at call site when getting all keys: snap.get('mysnap', *[]) is also valid "get me all the keys" with the *keys approach, but it's more obviously "no arguments" than "one empty list argument".

Let me know what you think.

[james-garner-canonical]

Oh also, this ties into the unset safety: we require at least one key by using key: str, /, *keys: str. If we switched to a sequence here, we'd want to do that for consistency in unset. So the 'at least one key' safety would move from being statically and runtime enforced by the signature itself, to a runtime only check.

[tonyandrewmeyer]

I meant list (or sequence, really) not dict, sorry.

[tonyandrewmeyer]

The function will never be able to grow any more positional arguments without breaking backwards compatibility,

I assume this is because snap config names can be any valid Python argument name (or even others). Honestly, this makes it feel even ickier that it's the way it is.

1. A str is an Iterable[str] and a Sequence[str] too, so the type checker won't reject snap.get('mysnap', 'config.engine'). So we need to do something with that at runtime, even if it's just raising a TypeError or ValueError.

Fair, although this is super common and we already deal with this exact problem in other places.

2. Calling with a single argument is a tiny bit uglier with this nested rather than flat design: snap.get('mysnap', ['config.engine']).

I'll give you a bit uglier, but I would argue a tiny bit clearer.

3. snap.get('mysnap', []) means "get me all the keys", which feels kind of weird.

This could easily be an error. I agree if it means "all the keys" that's bad.

[james-garner-canonical]

The function will never be able to grow any more positional arguments without breaking backwards compatibility,

I assume this is because snap config names can be any valid Python argument name (or even others). Honestly, this makes it feel even ickier that it's the way it is.

No, it's because of how *args works in Python. Without *args, a function with N positional args can add an N+1th (optional) positional argument and be backwards compatible. Because *args consumes all trailing positional arguments, you can only add new ones before *args, and adding new positional arguments in the middle of existing ones just can't be backwards compatible. So using *args always blocks you from changing the number of positional arguments later.

I think that's fine though, because functions should only have a few different arguments whose meaning is determined by their position, the rest should use keywords. And new optional arguments would typically be best as keyword args.

Returning to the original point, this means it's always easy to know which arguments are the config keys: all positional arguments other than the first, which (as with all the other functions) is the snap name.

[james-garner-canonical]

3. snap.get('mysnap', []) means "get me all the keys", which feels kind of weird.

This could easily be an error. I agree if it means "all the keys" that's bad.

In that case, the non-*args version needs a different way of expressing "all the keys".

If we want to retain snap.get('mysnap') having that meaning, then it would have to be the default value. I'd lean towards allowing users to express "all the keys" in a way compatible with snap.get(my_snap, my_keys), so the default shouldn't be a private sentinel but instead something like None. So snap.get('mysnap', None). This at least has the virtue of being a bit more explicit (and hopefully less error-prone) than [] but I still don't love it.

Alternatively, perhaps we should align with unset and have the library shield the user from this change in semantics based on argument count altogether, and have a separate get_all, requiring get to have at least one config name specified. I believe that's best done with the arg, *args pattern.

[tonyandrewmeyer]

No, it's because of how *args works in Python.

Argh, sorry I was reading this as **keys. I do know how *args works 😄.

Returning to the original point, this means it's always easy to know which arguments are the config keys: all positional arguments other than the first, which (as with all the other functions) is the snap name.

I still feel the explicit iterable is the clearer choice here. It's still having one of the arguments not match the rest that most puts me off. If it was snap_get(*keys) or similar then I think it would be ok (although I'm still not sold on that).

I'm not sure "all but the first" is something that is easy to know without seeing the docs or function signature. Just like knowing the order of arguments with positional in many cases is not knowable without that, which is generally an indicator of bad shape.

[james-garner-canonical]

I chatted with Ben and we settled on the following design and semantics:

def get(snap: str, keys: Iterable[str] | None = None) -> dict[str, Any]: ...
# get(mysnap, 'some-string'): TypeError
# get(mysnap) -> full config
# get(mysnap, keys=None) -> full config
# get(mysnap, []) -> {}

I'm a little on the fence about get(mysnap, []) -> {}. The most straightforward implementation, a naive early return, would bypass absent snap errors, which is undesirable, so we have a few options I'd like your opinion on:

1. Add snap installed check to the early return. Straightforward enough, but complicated a bit by the system snap case (see changes since your last review).
2. Fetch the full config, passing through the usual error handling paths, and then convert it immediately before return if the original keys argument was falsey-but-not-None.
3. Condense the installed checks into a single check that we always run, rather than trying to keep the happy path as a single API request.
4. Reconsider whether we want to second guess the API by making get for a non-installed snap an explicit error, and just follow the API's behaviour of returning {}.

---

For symmetry, unset should therefore probably have keys: Iterable[str] as well. Not passing any keys doesn't really make any sense, which suggests it should be required and not accept None:

def unset(snap: str, keys: Iterable[str]) -> dict[str, Any]: ...
# unset(mysnap, 'some-string'): TypeError
# unset(mysnap): TypeError

However, we still have to think about the case of the empty iterable. Note that naively unset(snap, []) performs the exact same snapd API operation that set(snap, {}) does -- a PUT with a {} body, so worth figuring out what that means too. The snapd API treats it as a harmless no-op (it's still a request, so it fails on an absent snap, but it doesn't change any config on the happy path, and it also doesn't error if the snap doesn't have a config hook). My instinct is that we should just let the API handle this. WDYT?

[tonyandrewmeyer]

I 'm a little on the fence about get(mysnap, []) -> {}.

Fair. Did you discuss it being an error, like your proposed unset?

1. Add snap installed check to the early return. Straightforward enough, but complicated a bit by the system snap case (see changes since your last review).

I feel like this would potentially end up messy if there end up being other snap checks needed. But perhaps an issue for another day.

2. Fetch the full config, passing through the usual error handling paths, and then convert it immediately before return if the original keys argument was falsey-but-not-None.

Convert meaning discard everything and return an empty dict?

3. Condense the installed checks into a single check that we always run, rather than trying to keep the happy path as a single API request.

I think we could hold off on this and do it later as an optimisation if needed, without changing the API shape, right?

4. Reconsider whether we want to second guess the API by making get for a non-installed snap an explicit error, and just follow the API's behaviour of returning {}.

Interesting. For the Juju case I assume charms would almost always be doing an ensure in install, unless it was some third-party snap the charm knew would be installed? But it does seem more logical for it to be an error to avoid a caller needing to know.

For symmetry, unset should therefore probably have keys: Iterable[str] as well. Not passing any keys doesn't really make any sense, which suggests it should be required and not accept None:

def unset(snap: str, keys: Iterable[str]) -> dict[str, Any]: ...
# unset(mysnap, 'some-string'): TypeError
# unset(mysnap): TypeError

This looks good to me.

However, we still have to think about the case of the empty iterable. Note that naively unset(snap, []) performs the exact same snapd API operation that set(snap, {}) does -- a PUT with a {} body, so worth figuring out what that means too. The snapd API treats it as a harmless no-op (it's still a request, so it fails on an absent snap, but it doesn't change any config on the happy path, and it also doesn't error if the snap doesn't have a config hook). My instinct is that we should just let the API handle this. WDYT?

Agreed, particularly if get with an empty iterable gives an empty dict.

[tonyandrewmeyer]

I wonder if people would want to be able to pass in either pre-JSON'd content or a serialiser. For things like datetimes (maybe those are not often config values?). But I guess that would be easy enough to add in the future.

[james-garner-canonical]

Exposing a serializer argument would be easy enough (though we'd have to wire it through several layers of client code). Passing a pre-JSON'd string is something I hadn't considered. Currently users can just serialize specific values as strings if needed (e.g. datetimes). I'm hopeful that regular Python values are sufficient for most cases, but this might warrant some investigation.

[tonyandrewmeyer]

I wonder if we have this conversation before too. We have this elsewhere in charming, too. I'm not a big fan, even though it's just exposing the snapd behaviour. I would rather people explicitly unset. I guess it's better to leak the underlying behaviour than to prevent people using it by searching for Nones and raising.

[james-garner-canonical]

I don't think we've talked deeply about this. I think None removing is fairly well-known by our users, and the symmetry with Python's get on the result side seems reasonable. It's probably also convenient if you want to build up a config and set it all in one go (without a separate unset call), though maybe that's a pattern you'd argue for discouraging?

I lean towards letting the behaviour leak through like this for simplicity of implementation and stick with the philosophy of staying close to the API. As you say, searching for None and raising doesn't seem like a good approach.

[tonyandrewmeyer]

❤️

[james-garner-canonical]

Pulling the API discussion into a single thread.

Currently we have:

def get(snap: str, /, *keys: str) -> dict[str, Any]: ...
    # get(snap) -> return full config

# Could add: def get_one(snap: str, key: str, /) -> Any: ...

def unset(snap: str, key: str, / *keys: str) -> None: ...
    # unset(snap) is a runtime TypeError, detected statically
    # unset(snap) would be a no-op

def set(snap: str, conf: Mapping[str, Any], /) -> None: ...
    # set(snap, {}) is a silent no-op

---

We could forbid get(snap) for symmetry with unset (getting all config doesn't seem dangerous though, so not that useful; and we'd need to add get_all since getting all config seems perfectly reasonable (while unsetting all config sounds a bit more like we add it if someone asks for it)):

def get(snap: str, key: str, /, *keys: str) -> dict[str, Any]: ...
def get_all(snap: str) -> dict[str, Any]: ...

---

Alternatively, we could strive for more symmetry with set, accepting a single Iterable[str] argument:

def get(snap: str, keys: Iterable[str]) -> dict[str, Any]:
    # get(snap, ()) should probably be an error (so we'd need get_all)
    # get(snap, "key") passes type checking, need to handle it explicitly
    #    could make this an error
    #    could overload and have this return the unwrapped value, but pyright complains about overlapping overloads, so this is probably too fancy (point in favour of adding get_one and making get(snap, "key") an error)
    #    could just make get(snap, "key") equivalent to get(snap, ["key"])
    keys = [keys] if isinstance(keys, str) else list(keys)
    if not keys:
        raise ValueError(...)  # Or should it be a snap.Error subclass?
    ...

def get_all(snap: str) -> dict[str, Any]: ...

# Could add: def get_one(snap: str, key: str, /) -> Any: ...

def unset(snap: str, keys: Iterable[str]) -> None: ...
    # unset(snap, ()) should probably be an error
    # same question about how to handle "key"
    keys = [keys] if isinstance(keys, str) else list(keys)
    if not keys:
        raise ValueError(...)  # Or should it be a snap.Error subclass?

# Could add: def unset_all(snap: str) -> None: ...

def set(snap: str, conf: Mapping[str, Any], /) -> None: ...
    # conf={} is a no-op currently -- could make this an error, symmetrically with get/unset

@tonyandrewmeyer do you have a particular flavour of error or non-error behaviour you'd like to champion for get(snap, ()) and get(snap, "key"), and likewise for unset? And also for set(snap, {}).

---

If symmetry is the goal (and I don't think it necessarily should be), we could adapt the signature of set to use *config or **config. But because config values have such a naturally dict shaped structure, and because config={} is a safe no-op, I'm pretty convinced that the conf: Mapping[str, Any] approach is correct. Maybe that points us inevitably in the direction of keys: Iterable[str].

[tonyandrewmeyer]

@tonyandrewmeyer do you have a particular flavour of error or non-error behaviour you'd like to champion for get(snap, ())

I dislike this being the same as get(snap). I'd be ok if it non-error returned an empty dict, since I think that's what you have actually asked for. I think my preference would be some sort of error (Value?) because it doesn't seem like it would ever make sense to ask for nothing (except maybe when the calling code doesn't know what's being asked for and is just going to loop through the results, but even then I think the caller could just put in an if themselves).

and get(snap, "key"),

Definitely not no-error lookups for k, e, y. I wish I knew how common it was to only have one config item or to only be getting one (can we tell that from the charm use of the old version?). I don't really like forcing people to do get(snap, {"key"}) if that's the common case. But generally, I think it's less confusing to not have (essentially) str|Iterable[str] and so raise TypeError for a plain str.

and likewise for unset?

I think the same for passing in an empty iterable as for get, for the same reasons. And also for passing a plain string.

And also for set(snap, {}).

I think I would still prefer an error here, because again it seems like you're asking to do nothing and that is probably a mistake. If you're asking to un-set, then that's definitely an error and you should be hard pushed over to using unset.

If symmetry is the goal (and I don't think it necessarily should be),

I don't think it has to be symmetric, and there's going to be some asymmetry anyway because of the get/unset/set differences. But I do think something that feels close to symmetry is often a good smell.

we could adapt the signature of set to use *config or **config.

Again, I'm let down here by my lack of snap knowledge. If the vast majority of the time you're setting config at the top level, where all the keys would be valid Python identifiers and you only rarely need to do **{"foo.bar": 123, "def": 456} then **config would seem appealing (I'm not sure how *config would work, where would the values be?). But I suspect that this is not the case, and nesting is actually common.

Maybe that points us inevitably in the direction of keys: Iterable[str].

Well, that's part of my argument, yes 😂.

I'm happy to continue this async, but if you'd rather have a short call to discuss it (and rope in that Ben guy) that's good too.

[tonyandrewmeyer]

Since you love dataclasses so much, what about something wildly different?

# Claude wrote this, I just eyeballed it for this wild idea that I'm 99% sure will get rejected.
import dataclasses
import typing
from typing import Any, TypeVar

_T = TypeVar('_T')

def _key(f: dataclasses.Field[Any]) -> str:
    return f.metadata.get('snap_key', f.name.replace('_', '-'))

def _unmarshal(cls: type[_T], data: dict[str, Any]) -> _T:
    kwargs: dict[str, Any] = {}
    hints = typing.get_type_hints(cls)
    for f in dataclasses.fields(cls):
        wire = _key(f)
        if dataclasses.is_dataclass(hints[f.name]):         # nested namespace
            kwargs[f.name] = _unmarshal(hints[f.name], data.get(wire, {}))
        elif wire in data:
            kwargs[f.name] = data[wire]
    return cls(**kwargs)                                    # unset -> field default

def _marshal(obj: Any) -> dict[str, Any]:
    out: dict[str, Any] = {}
    for f in dataclasses.fields(obj):
        value = getattr(obj, f.name)
        if dataclasses.is_dataclass(value):
            nested = _marshal(value)
            if nested:
                out[_key(f)] = nested                       # snapd accepts nested dicts
        else:
            out[_key(f)] = value                            # includes None => unset
    return out

def get_conf(snap: str, schema: type[_T], /) -> _T:
    config = _client.get(f'/v2/snaps/{snap}/conf')
    assert isinstance(config, dict)
    return _unmarshal(schema, config)

def set_conf(snap: str, conf: Any, /) -> None:
    _client.put(f'/v2/snaps/{snap}/conf', body=_marshal(conf))

Used like:

# I wrote most of this bit.
import dataclasses
from charmlibs import snap

@dataclasses.dataclass
class Core:
    https_address: str | None = None           # <-> core.https-address
    trust_password: str | None = None          # <-> core.trust-password

@dataclasses.dataclass
class LxdConf:
    core: Core = dataclasses.field(default_factory=Core)
    debug: bool | None = None                  # <-> debug
    # escape hatch when snake_case<->dash-case isn't the real key:
    ceph_builtin: bool | None = dataclasses.field(
        default=None, metadata={'snap_key': 'ceph.builtin'}
    )

...

conf = snap.get_conf('lxd', LxdConf)   # one GET /v2/snaps/lxd/conf
# conf is a fully-typed LxdConf; IDE completion + type-checker over conf.core.https_address

conf.core.https_address = '[::]:8443'
conf.debug = True
conf.core.trust_password = None        # None => unset this key (ick, maybe there's something better)

snap.set_conf('lxd', conf)             # one PUT /v2/snaps/lxd/conf