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

Waffles Part 1: Import config waffles to django waffles 🧇 #15195

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

robhudson
Copy link
Member

@robhudson robhudson commented Sep 20, 2024

Multi-line summary

Part 1 (this PR):

  • Adds django-waffle library
  • Adds management command to load current ConfigValues into waffle tables
  • Removes funnelcake code

Part 2 will:

  • Remove old www_config import
  • Update the template tag helper to use django-waffle

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.

  • Deployment 1: Add waffle table and import waffle switches
  • Deployment 2: Switch template tags to use django-waffle

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:

make install-local-python-deps

Check that you currently have no waffle switches:

./manage.py waffle_switch -l

Import the switches into waffle:

./manage.py import_waffle_switches

Verify that you have the expected waffle switches:

./manage.py waffle_switch -l

Copy link
Member

@alexgibson alexgibson left a 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'

@robhudson robhudson force-pushed the 15157-mv-to-django-waffle-part-1 branch from a654e0a to db68aa9 Compare September 20, 2024 14:18
@robhudson
Copy link
Member Author

I tried following the instructions here but I get an error saying ModuleNotFoundError: No module named 'waffle'

Ah sorry, I forgot to run make compile-requirements. A pip install django-waffle will do, but I also updated the PR to add it to the requirements files.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 16.66667% with 15 lines in your changes missing coverage. Please review.

Project coverage is 77.69%. Comparing base (bc4773b) to head (db68aa9).
Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
...base/management/commands/import_waffle_switches.py 0.00% 15 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@alexgibson alexgibson left a 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?

Comment on lines +19 to +21
# Ignore funnelcakes and other yummy things.
if not config.name.startswith(prefix):
continue
Copy link
Member

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?

@robhudson
Copy link
Member Author

One question: how does this effect switches that are not set in www-config, but in deployments / secrets?

Do you have an example of a switch being configured in this way?

@janbrasna
Copy link
Contributor

how does this effect switches that are not set in www-config, but in deployments / secrets?

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…(?)

@alexgibson
Copy link
Member

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.

@stevejalim
Copy link
Collaborator

stevejalim commented Sep 30, 2024

You can check the env vars set in the various *values.yaml files here https://github.com/mozilla-it/webservices-infra/tree/main/bedrock/k8s/bedrock @alexgibson - the only reference to any switch I can see is switch_newsletter_maintenance_mode: 'False' which would become SWITCH_NEWSLETTER_MAINTENANCE_MODE as an env var

@alexgibson
Copy link
Member

the only reference to any switch I can see is switch_newsletter_maintenance_mode: 'False' which would become SWITCH_NEWSLETTER_MAINTENANCE_MODE as an env var

OK, so I guess we'd need to move this into the DB?

@stevejalim
Copy link
Collaborator

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 basket-client

Copy link
Collaborator

@stevejalim stevejalim left a 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

@janbrasna
Copy link
Contributor

the only reference to any switch I can see is switch_newsletter_maintenance_mode

OK, so I guess we'd need to move this into the DB?

It might well be redundant, as I can't see it being used in Bedrock or basket-client

It's used in the FE forms:

{% set maintenance_mode = switch('newsletter-maintenance-mode') %}

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?)

@robhudson
Copy link
Member Author

(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 settings.DEV is True, all switches will be "on" like we have now.

Question for @stevejalim and others: Should we also pull down the switches via ./bin/run-db-download.py. I would say yes and I've already added it to part-2 of this PR but wanted to ask.

@stevejalim
Copy link
Collaborator

@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.

@robhudson robhudson changed the title Import config waffles to django waffles 🧇 Waffles Part 1: Import config waffles to django waffles 🧇 Oct 1, 2024
@robhudson robhudson merged commit e3c41e7 into main Oct 2, 2024
5 checks passed
@robhudson robhudson deleted the 15157-mv-to-django-waffle-part-1 branch October 2, 2024 22:25
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

Successfully merging this pull request may close these issues.

4 participants