boolean interpretation for feature flags

[Martin Pool]

Many feature flags have boolean effects. However, the infrastructure just sends them back as strings, and the callers have varying interpretations:

 * 'on' vs 'off'
 * non-empty string vs empty string through Python bool(s)
 * 'enabled' vs 'disabled'

The inconsistency is likely to lead to bugs or misconfigurations.

We should instead choose a particular syntax and add an API that will get the value as a boolean, including an API that can be used from TAL.

[Martin Pool (mbp)]

One way to do this that might fit cleanly with the registry of them, and with wgrant's recent declaration of a value domain, and that might work well from tal, would be for the default getFeatureFlag to just interpret the string from the database into whatever type is expected.

We could also (perhaps as a separate bug) validate that values entered into the admin ui match what is that expected.

In both cases we need to tolerate unknown flags and just treat them as strings.

[Curtis Hovey (sinzui)]

I have looked at the code and think that FeatureController.getFlag() could do late casting of the value using flag_info that is defined in the same module.

As we develop a feature, we might want to change the flag from boolean to int to string, so I do not think we want to force the type. We want to report when the type is not what we expect though. FeatureController.getFlag() already reports undocumented flags. It could do something like this:
    val = self._known_flags.lookup(flag)
    try:
        return val_to_type(flag, val)
    except CasteError:
        wrong_documented_flags.add(flag)
        return val

The val_to_type() function could be a sanity check when we save rules. I think these are warnings, not failures since the documentation may be wrong, or the code is using the value as more than one type.
While this issue is just about boolean, we could support string, int, and float too

On the topic of what value is boolean. I favour documenting that we expect enabled/disabled. We could easily support on/off, true/false, 1/0, yes/no, or any-text/null case-insensitive string evaluation (like lazr.config)

[Robert Collins (lifeless)]

There is a real risk of skew if we support non-empty strings as false
for booleans. We have pretty robust in-appserver code that the moment:
if getFlag('foo'):

if not getFlag('foo'):

- I'm going to mark this wontfix for the moment. if we want types we
probably want a whole nother layer and LEP.

[Robert Collins (lifeless)]

Further to my mail, the problem statement this bug has was a problem but was resolved by wgrants earlier work.