-
Notifications
You must be signed in to change notification settings - Fork 919
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
Waffles Part 1: Import config waffles to django waffles 🧇 #15195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried following the instructions here but I get an error saying ModuleNotFoundError: No module named 'waffle'
a654e0a
to
db68aa9
Compare
Ah sorry, I forgot to run |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15195 +/- ##
==========================================
- Coverage 77.88% 77.69% -0.19%
==========================================
Files 162 164 +2
Lines 8473 8470 -3
==========================================
- Hits 6599 6581 -18
- Misses 1874 1889 +15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked well for me (sorry it took a day or so to get back to this) r+
One question: how does this effect switches that are not set in www-config, but in deployments / secrets?
# Ignore funnelcakes and other yummy things. | ||
if not config.name.startswith(prefix): | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove mention of funnalcakes here?
Do you have an example of a switch being configured in this way? |
I believe e.g. the Cloud Run demos have set it directly in the .env — so the question perhaps is if anything set on the env will continue working, or only the db switches will be honored…(?) |
In addition I was wondering if dev/stage/prod deployments might also have certain switches set directly environment variables, but I don't have any visibility there to check. |
You can check the env vars set in the various |
OK, so I guess we'd need to move this into the DB? |
I'd imagine it can wait till after this rollout. It might well be redundant, as I can't see it being used in Bedrock or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't test-driven because Alex aleady has. Code LGTM.
Park of me is asking "Are we 100% confident funnelcake builds won't come back?" and am sure the answer is "No", but I'd love to know more about that
It's used in the FE forms:
to disable interaction with an error message banner. (Which e.g. has been off in staging for some time recently…) However this switch is already set in www-config so if you're migrating those it will be included in the set. (Will the switches still be read from env, in addition to those set in db, e.g. for testing/dev perhaps just with the exported db snapshots, or the db keys will be the only source?) |
My thought was having the database switches be the only source. That clears up confusion between switches and regular env vars that could exist. The django-waffle library comes with a management command to easily list and add/remove switches locally, so if you needed one, you could: Check if it already exists: ./manage.py waffle_switch -l Toggle a switch on or off: ./manage.py waffle_switch SWITCH_NAME on Add a switch: ./manage.py waffle_switch --create SWITCH_NAME on Remove a switch: ./manage.py waffle_delete --switches SWITCH_NAME On top of that, locally when Question for @stevejalim and others: Should we also pull down the switches via |
@robhudson If you want the waffle flags to be included in the DB export, you'll need to add them to this list https://github.com/mozilla/bedrock/blob/main/bin/export-db-to-sqlite.sh#L132-L176 though it sounds like you've already done this The only reason not to include the Switches in the DB would be if they are secret (eg they leak info via their name, for instance), because the DB is downloadable (deliberately) and contains no sensitive info. |
Multi-line summary
Part 1 (this PR):
ConfigValue
s into waffle tablesPart 2 will:
We need to roll this out in two deployments since we can't ensure the database will be updated when the template tag switch happens.
Significant changes and points to review
Ensure waffle switches still work. We haven't added django-waffle's templatetag or anything here so all the www_config stuff should work as before.
Issue / Bugzilla link
#15157
Testing
After updating Python dependencies with:
Check that you currently have no waffle switches:
Import the switches into waffle:
Verify that you have the expected waffle switches: