cfpb / django-flags

Feature flags for Django projects
https://cfpb.github.io/django-flags/
Creative Commons Zero v1.0 Universal
260 stars 32 forks source link

Required conditions #24

Closed willbarton closed 5 years ago

willbarton commented 5 years ago

This PR makes two significant changes:

Currently Django-Flags supports defining multiple conditions for a single feature flag, and any of those conditions can be met to enable the flag. A required condition must always be met along with any non-required conditions.

This makes it possible to define a flag like this:

FLAGS = {
    'MY_FLAG': [
        {'condition': 'path matches', 'value': r'^/apocalypse', 'required': True},
        {'condition': 'user', 'value': 'horseman.one'},
        {'condition': 'user', 'value': 'horseman.two'},
        {'condition': 'user', 'value': 'horseman.three'},
        {'condition': 'user', 'value': 'horseman.four'}
    ],
}

This enables the flag for any of the above users at the given path. It is equivalent to the following pseudo-code:

if re.search(r'^/apocalypse', path) and (
    user == 'horseman.one' or 
    user == horseman.two' or 
    user == 'horseman.three' or 
    user == 'horseman.four'
):

This will require some care and reflection when using required conditions; required conditions are required for all evaluations of all conditions. So if you have multiple path matches conditions that all start with ^/, the flag will never evaluate to True:

FLAGS = {
    'MY_FLAG': [
        {'condition': 'path matches', 'value': r'^/apocalypse', 'required': True},
        {'condition': 'path matches', 'value': r'^/rapture', 'required': True},
    ],
}

In addition, the docs have been updated to reflect the deprecation of single-dict flags, along with all the tests that used such flags. A deprecation warning has been added.

This PR will necessitate a minor version bump, so it'll be in 4.1.0.

willbarton commented 5 years ago

Possibly related to not converting the flag definition to a list, adding a required condition that evaluates to false via the UI is not resulting in the flag evaluating to false as a whole:

That seems like a problem I'll investigate.

As a general comment, I dislike having to make the syntax more complex to accommodate this, but I trust you considered other approaches and that this was the one that made the most sense.

Yeah, I agree, but I think having a list of dictionaries allows us to expand the possible options more in the future with less pain and less ambiguity. We could keep adding members to a tuple, but that becomes exponentially less self-documenting with each new member.

willbarton commented 5 years ago

It looks like DeprecationWarnings are ignored by default.

However, it actually might be better to use FutureWarning:

Base category for warnings about constructs that will change semantically in the future.

Because this is a semantic change to the FLAGS setting. That works and the test to check for it passes.

Scotchester commented 5 years ago

OK, I now see the FutureWarning when runserver starts up, but I still have the aforementioned problem where the actual required flag functionality doesn't seem to be working.