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

Consolidate config settings for handling missing entities #481

Open
tim-mccurrach opened this issue Dec 27, 2022 · 0 comments
Open

Consolidate config settings for handling missing entities #481

tim-mccurrach opened this issue Dec 27, 2022 · 0 comments

Comments

@tim-mccurrach
Copy link

tim-mccurrach commented Dec 27, 2022

The problem

I ran into an error that occured due to a typo in the name of a waffle flag in my code. It would have been much better (IMO), and the bug would have been picked up much earlier, had flag_is_active failed hard for non-existent flags. Failing silently can be dangerous, and whilst I appreciate I can set WAFFLE_LOG_MISSING_FLAGS to logging.ERROR to increase visibility, this will still not cause a request to fail hard. In development environments, I tend not to pay particular attention to logs unless I'm specifically looking for something. However, a hard failure would pick up the typo before it makes its way to production.

Is this configuration bloat??

Obviously, not everyone would want the behaviour I suggest above (quite apart from the fact that it would be a breaking change). As such, an extra configuration setting such as WAFFLE_RAISE_MISSING_FLAGS would be needed. There are currently quite a lot of configuration settings for waffle. For each of FLAG, SWITCH and SAMPLE we have:

  • WAFFLE_*_DEFAULT
  • WAFFLE_CREATE_MISSING_*S
  • WAFFLE_LOG_MISSING_*S

With this in mind, we probably do not want an extra configuration setting. There are no doubt other behaviours that other uses would wish for in the case of a missing flag, and it's untenable to provide a new setting for each of them.

A proposed solution

Instead of introducing a new config setting, I propose that we replace WAFFLE_CREATE_MISSING_*S and WAFFLE_LOG_MISSING_*S with a new setting WAFFLE_HANDLE_MISSING_*S which would be a string that points to a function that would be called when an entity is missing. For example:

# settings.py
WAFFLE_HANDLE_MISSING_FLAGS = "some_file.handle_missing_flags"

# some_file.py
def handle_missing_flags(name):
    # potentially log a message, create a flag and do anything else you want
    # optionally return a flag

(If a flag is returned the flag would be cached, and the value from the flag would be used, otherwise the default value would be used - as is the current behaviour).

This would have several advantages.

  • The default function would use the existing WAFFLE_CREATE_MISSING_*S and WAFFLE_LOG_MISSING_*S settings so that introducing the setting would result in no change in behaviour.
  • We can then utilise the django checks framework to provide warnings that the old settings will eventually be deprecated, and in time update those warnings to errors. Suitable documentation could also be provided to guide migration to the new system (although this should be reasonably straight-forward). This will mean a pleasant deprecation timeline.
  • This allows users greater flexibility to provide the individual functionality they want.
  • This actually reduces the number of configuration settings whilst protecting against future settings bloat.

(Part of me would like to subsume WAFFLE_*_DEFAULT into the new setting too, but seeing as the default values are used in WaffleJS this would be difficult.)

If there is support for this proposition I would be more than happy to submit a PR. I look forward to any feedback on the above.

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

1 participant