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

snap.set should correctly handle nested dicts (or raise an exception) #46

Open
PietroPasotti opened this issue Sep 8, 2022 · 5 comments

Comments

@PietroPasotti
Copy link
Contributor

Currently, snap.set({"foo": {"bar": "baz"}}) will result in the snap being configured with
foo = '{"bar": "baz"}' (a repr-dump of the dict object, as a string).

The snap lib should provide means to format this correctly, i.e. something like test.charms.test_main.lib.ops.model._format_action_result_dict: a flattened dict using dots as key separator.

Ideally, snap.set would iterate through the values, attempting to flatten any nested dicts it find, and raise an exception if this fails or if any value is not a string or a dict.

@rwcarlsen
Copy link
Contributor

This seems reasonable to me and consistent with other things we do in charming - (e.g. setting action results in ops you mentioned).

@pengale
Copy link
Contributor

pengale commented Sep 9, 2022

Agree that this is eminently reasonable. It would be nice to expose the helpers we have in ops, so that the library can re-use the existing code, and thus behave consistently.

@PietroPasotti
Copy link
Contributor Author

PietroPasotti commented Sep 9, 2022

Update: I've followed up with the snapcraft folks since snap set appears to have a -f flag that should be able to parse json, but in practice fails with an unhelpful error message.

@PietroPasotti
Copy link
Contributor Author

PietroPasotti commented Sep 9, 2022

We can't re-use the ops utils as they are because snap config is not just nested dicts but YAML, so e.g. lists and stuff, which we don't need for the action-results-set that that internal util was written for. So we need some 'new code'. Hopefully we can just dump to json and be done with it (if the snap cli behaves, that is)

@pengale
Copy link
Contributor

pengale commented Sep 9, 2022

Dumping to json sounds like the right solution. I'm a little bit disappointed that we didn't do that in the first place. (And I'm not sure if I want to run git blame on the repo -- I might discover that I was the one who made that decision.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants