very easy to shoot yourself in the foot with hookenv.config()

[axinojolais]

As I discovered in https://github.com/juju-solutions/layer-beats-base/issues/18, it's very easy to shoot yourself in the foot with config().

If you call my_config = hookenv.config() and then modify an item of my_config, the mangled config will get written to disk, which will make the basic layer set the config.changed flag on the next hook run.

Unfortunately, I don't have a sensible solution to propose in order to fix this.

[ajkavanagh]

The obvious way to sort this is to do a deepcopy() before returning the config; but that would change the existing behaviour. So it depends on whether the existing behaviour was a bug or an intended feature?

[stub42]

The charm-helpers behavior is intended and I know of charms using it extensively.

The config could be exposed through a less surprising mechanism for charms.reactive charms, somewhere under the charms.* package namespace I guess.

[ajkavanagh]

The charm-helpers behavior is intended and I know of charms using it extensively.

@stub42 ah, interesting. It's because, I guess, it came about with the Services framework. As hookenv.config() is cached, then there's only ever one of them, which also means that you can 'trample' on other 'instances' being used.

So a couple of thoughts:

1. Change the default behaviour to return a new copy (i.e. not cache it).
2. alternatively, add a config_copy() to hookenv which is an explicit copy that doesn't save on exit. Then the user would know that there was a difference.

In both cases, expand the docstring to explain what's going on.

Thoughts on my thoughts?

[stub42]

We can't change what is being returned - it isn't a cached copy, but a dictionary like object that is designed to be modified and persists those modifications. Its no longer recommended to be used that way (instead, use unitdata.kv() or similar), but some older charms are still using it the way it was designed and using it to store their state.

[ajkavanagh]

@stub42 re: cached, that's kinda what I meant; I didn't explain myself properly.

I guess we can close this.

My final thought is hookenv.config_copy() was to provide a deliberately non singleton config dictionary-like object, so that users would know they weren't going to possibly clobber other uses of 'config()'. And then people could slowly migrate to using config_copy() rather than config(). It would also highlight (perhaps) that there was actually a difference. Even better; add a deprecation notice to config(), rename config() to config_singleton() and add config_copy() to get people to choose proactively.