Providing Config.__getitem__() is error prone

[tartley]

In more than one of the services which use talisker, there have been multiple recurring or longstanding bugs caused by callers which should have used:

talisker.get_config()['DEVEL']
# or
talisker.get_config().devel

but instead used:

talisker.get_config()['devel']

This formulation doesn't work, it just returns None.

See definition of getitem() at

talisker/talisker/config.py

Line 148 in 8864e37

def __getitem__(self, name):

The above makes me wonder whether offering __getitem__ on the Config object itself is wise, or does it just potentially confuse callers? Perhaps it would be nice to offer it as a method on Config, like .raw(key), so it's more explicit whether callers should be passing nice property names like 'devel', or raw config keys like 'DEVEL'.

Or maybe, offering indexing right on Config() objects is fine, and the error prone part is actually that we fall back to returning None for unrecognized keys. Perhaps we should raise an index error instead?

Of course these changes would potentially be breaking, so would require care to introduce, but perhaps they would save future bugs like the ones we've already had.

[maxiberta]

I think raising a KeyError on a missing key makes sense and is the expected behaviour from a dict-like object, while keeping full backwards compatibility.

[tartley]

@maxiberta Thanks. Yes, that wouldn't be an incompatible API change, but obviously it would still need deploying with care - there may be code in services which is currently passing an invalid key, getting None back and happily carrying on. After the change it will KeyError.

[maxiberta]

Having services visibly blow up is good actually, and much better than subtly returning unexpected values.

[tartley]

Yes, absolutely agreed, that's why I suggest it! I'm only saying that when we deploy services using this new version of talisker, we should take care that we're not deploying something that would blow up immediately, is all (which, operationally, would be the equivalent of breaking a critical API compatibility)