-
Notifications
You must be signed in to change notification settings - Fork 28
feat(snap): add snap conf module #572
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
base: main
Are you sure you want to change the base?
Changes from all commits
6f21289
18d31d5
6733ac6
1a15928
1da87f9
d3dc3c5
3127e26
83d76ba
3767de2
db8a4c6
9246122
213385e
88f4548
f2787ab
e33c012
583597f
0343fda
0163d10
425e549
12e44ff
6ad5731
2062476
ce0733e
1945c70
8742196
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| # Copyright 2026 Canonical Ltd. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Snap operations implemented as direct calls to the snapd REST API.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import typing | ||
|
|
||
| from . import _client, _errors | ||
|
|
||
| if typing.TYPE_CHECKING: | ||
| from collections.abc import Iterable | ||
| from typing import Any | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # /v2/snaps/{snap}/conf | ||
|
|
||
|
|
||
| # Getting one config value looks like get(s, [k])[k]. In future we could add a get_one(s) helper. | ||
| # Get with keys=None returns the entire config, following the CLI (get_all is unnecessary). | ||
| def get(snap: str, keys: Iterable[str] | None = None) -> dict[str, Any]: | ||
| """Get snap configuration. | ||
|
|
||
| Args: | ||
| snap: The name of the snap to read configuration from. | ||
| keys: Configuration keys to read. Nested options may be accessed with dotted notation, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I lean towards a dict here too.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we've talked about this before. I definitely see the ugliness you're highlighting with
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
I don't think a It could be a
If we disregard 2, then we could avoid 1 by requiring some concrete types, like We could resolve both at once by explicitly allowing
Let me know what you think.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh also, this ties into the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant list (or sequence, really) not dict, sorry.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Fair, although this is super common and we already deal with this exact problem in other places.
I'll give you a bit uglier, but I would argue a tiny bit clearer.
This could easily be an error. I agree if it means "all the keys" that's bad.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In that case, the non-*args version needs a different way of expressing "all the keys". If we want to retain Alternatively, perhaps we should align with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Argh, sorry I was reading this as
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 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
For symmetry, def unset(snap: str, keys: Iterable[str]) -> dict[str, Any]: ...
# unset(mysnap, 'some-string'): TypeError
# unset(mysnap): TypeErrorHowever, we still have to think about the case of the empty iterable. Note that naively
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fair. Did you discuss it being an error, like your proposed
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.
Convert meaning discard everything and return an empty dict?
I think we could hold off on this and do it later as an optimisation if needed, without changing the API shape, right?
Interesting. For the Juju case I assume charms would almost always be doing an ensure in
This looks good to me.
Agreed, particularly if
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your comments here. I stuck with |
||
| for example ``['server.port']``. If ``None``, the full config is returned as a | ||
| nested dict. If an empty iterable, an empty dict is returned if the snap is installed. | ||
| Must not be a bare string. | ||
|
|
||
| Returns: | ||
| A dict mapping each requested key to its configured value. If all keys are requested | ||
| (keys=None), the entire config is returned as a nested dict (empty if the snap has no | ||
| configuration). If no keys are requested (keys=[]), an empty dict is returned if the snap | ||
| is installed. Each dotted key queried is returned as a top-level entry. | ||
|
|
||
| Raises: | ||
| TypeError: if ``keys`` is a string (must be a non-string iterable of strings, or ``None``). | ||
| NotFoundError: if the snap is not installed. Never raised for ``system`` or ``core``, | ||
| whose configuration is served whether or not the core snap is installed. | ||
| OptionNotFoundError: if a requested key has no value stored in the snap's configuration. | ||
| Snap configuration is schemaless, so snapd does not distinguish between a key the | ||
| snap doesn't recognise, a key that was never set, and a key that was unset. Any | ||
| defaults a snap applies internally are invisible here unless its configure hook has | ||
| stored them with ``snapctl set``. | ||
|
|
||
| :: | ||
|
|
||
| # Full config. | ||
| get('foo') # {'server': {'port': 8080}, 'client': {'timeout': 30}} | ||
| get('foo', keys=None) # {'server': {'port': 8080}, 'client': {'timeout': 30}} | ||
| # Querying specific keys. | ||
| get('foo', ['client']) # {'client': {'timeout': 30}} | ||
| get('foo', ['client.timeout']) # {'client.timeout': 30} | ||
| get('foo', ['server', 'server.port']) # {'server': {'port': 8080}, 'server.port': 8080} | ||
| # Querying no keys. | ||
| get('foo', keys=[]) # {} | ||
| # Invalid keys argument. | ||
| get('foo', keys='client') # TypeError | ||
| """ | ||
| if isinstance(keys, str): | ||
| raise TypeError('keys must be an iterable of strings, or None (not a string)') | ||
| if keys is not None: | ||
| keys = list(keys) | ||
| if keys == []: | ||
| # NOTE: snapd returns the full configuration if no keys are specified. | ||
| # We pass this behaviour through for keys=None, but for keys=[] we return | ||
| # an empty dict, since the caller explicitly requested no keys. | ||
| _raise_if_snap_not_installed_or_system(snap) | ||
| return {} | ||
| params = {'keys': ','.join(keys)} if keys is not None else None | ||
| try: | ||
| config = _client.get(f'/v2/snaps/{snap}/conf', query=params) | ||
| except _errors.OptionNotFoundError: | ||
| # NOTE: snapd reports option-not-found both for a missing key and for a missing snap. | ||
| # The CLI returns 'error: snap "foo" has no "bar" configuration' in both cases. | ||
| # For symmetry with PUT (set/unset), we raise NotFoundError here for a missing snap. | ||
| # Following PUT, NotFoundError isn't raised for system/core (installed or not). | ||
| _raise_if_snap_not_installed_or_system(snap) | ||
| raise | ||
| if keys is None and not config: | ||
| # NOTE: snapd returns {} for an installed snap with no config and for a missing snap. | ||
| # The CLI returns 'error: snap "foo" has no configuration' for a missing snap. | ||
| # For symmetry with PUT (set/unset), we raise NotFoundError here for a missing snap. | ||
| # Following PUT, NotFoundError isn't raised for system/core (installed or not). | ||
| _raise_if_snap_not_installed_or_system(snap) | ||
| assert isinstance(config, dict) | ||
| return typing.cast('dict[str, Any]', config) | ||
|
|
||
|
|
||
| def _raise_if_snap_not_installed_or_system(snap: str) -> None: | ||
| """Raise NotFoundError if the snap is not installed, unless it's system/core.""" | ||
| # NOTE: snapd's conf endpoints treat 'system' as an alias for 'core'. | ||
| # System configuration is served whether or not the core snap is installed. | ||
| # /v2/snaps/system always 404s (it's a hardcoded alias, not a real snap). | ||
| # /v2/snaps/core 404s when the core snap is absent (typical when no other snaps depend on it). | ||
| # For the purposes of this module, we can skip snap installed checks for both names. | ||
| if snap not in ('system', 'core'): | ||
| _client.get(f'/v2/snaps/{snap}') | ||
|
|
||
|
|
||
| def unset(snap: str, keys: Iterable[str]) -> None: | ||
| """Unset snap configuration keys. | ||
|
|
||
| Unsetting a key that is not currently set is a no-op and does not raise. | ||
|
|
||
| Args: | ||
| snap: The name of the snap to unset configuration on. | ||
| keys: Configuration keys to unset. Nested options may be addressed with dotted | ||
| notation, for example ``['server.port']``. Must not be a bare string. | ||
| An empty iterable is passed to snapd, where it is treated as a no-op. | ||
|
|
||
| Raises: | ||
| TypeError: if ``keys`` is a string (must be a non-string iterable of strings). | ||
| NotFoundError: if the snap is not installed. | ||
| ChangeError: if the snap's configure hook fails. This includes unsetting any | ||
| configuration on a snap that does not define a configure hook. A failed change | ||
| is rolled back: no key from the request is unset. | ||
| """ | ||
| if isinstance(keys, str): | ||
| raise TypeError('keys must be an iterable of strings (not a string)') | ||
| # NOTE: snap-not-found is returned for a missing snap, but not for system or core, | ||
| # even if the core snap isn't installed -- configuration changes are still applied. | ||
| # NOTE: Unset with no keys is a no-op (like set with an empty dict). We let snapd handle it. | ||
| _client.put(f'/v2/snaps/{snap}/conf', body=dict.fromkeys(keys)) | ||
|
|
||
|
|
||
| # Defined last to minimise the chance of meaningfully shadowing the built-in set type. | ||
| def set(snap: str, config: dict[str, Any]) -> None: # noqa: A001 (shadowing a Python builtin) | ||
| """Set snap configuration. | ||
|
|
||
| Args: | ||
| snap: The name of the snap to configure. | ||
| config: A mapping of configuration keys to values. Values may be any JSON-serialisable | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| type, including nested dicts and lists. Setting a key to ``None`` unsets it. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we've talked deeply about this. I think 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 |
||
| Nested options may be addressed with dotted keys, for example ``server.port``. | ||
| An empty mapping is accepted as a no-op. | ||
|
|
||
| Raises: | ||
| NotFoundError: if the snap is not installed. | ||
| ChangeError: if the snap's configure hook fails. This includes setting any | ||
| configuration on a snap that does not define a configure hook, and configuration | ||
| rejected by a validating configure hook. A failed change is rolled back: no key | ||
| from the request is applied. | ||
| """ | ||
| # NOTE: snap-not-found is returned for a missing snap, but not for system or core, | ||
| # even if the core snap isn't installed -- configuration changes are still applied. | ||
| # NOTE: Set with an empty dict is a no-op. We let snapd handle it. | ||
| _client.put(f'/v2/snaps/{snap}/conf', body=config) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| #!/bin/sh -e | ||
| # Validating configure hook: rejects any value for 'bad-key', accepts everything else. | ||
| # Used by functional tests to exercise how snaps reject configuration at set time. | ||
| bad="$(snapctl get bad-key)" | ||
| if [ -n "$bad" ]; then | ||
| echo "bad-key is not allowed" >&2 | ||
| exit 1 | ||
| fi |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| name: test-configure-snap | ||
| version: "1.0" | ||
| summary: Snap with a validating configure hook for charmlibs.snap functional tests | ||
| confinement: strict | ||
| base: core24 |
Uh oh!
There was an error while loading. Please reload this page.