[BD-21] Extract waffle_utils from edx-platform by regisb · Pull Request #70 · openedx/edx-toggles

regisb · 2020-10-07T17:17:47Z

Description: Extract the various waffle switch/flag classes from edx-platform such that they can be used in other IDAs. This implements ADR #2 <https://github.com/edx/edx-toggles/tree/master/docs/decisions/0002-application-toggle-state.rst>, #3 <https://github.com/edx/edx-toggles/tree/master/docs/decisions/0003-django-setting-toggles.rst>, #4 <https://github.com/edx/edx-toggles/tree/master/docs/decisions/0004-toggle-api.rst>__. These changes are backward-compatible. Suppression of the namespace classes will happen in a separate PR.

JIRA: https://openedx.atlassian.net/wiki/spaces/COMM/pages/1596358943?src=mail&src.mail.timestamp=1593719976942

Reviewers:

@robrap

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPi after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

This is a work in progress. A couple things still need to be done:

Move waffle flag classes
Move unit tests
Document the new classes
Do not export BaseNamespace, which should not be exposed to edx-platform

openedx-webhooks · 2020-10-07T17:17:55Z

Thanks for the pull request, @regisb! I've created BLENDED-617 to keep track of it in Jira. More details are on the BD-21 project page.

When this pull request is ready, tag your edX technical lead.

robrap

I'm going to wait on further review.

robrap

I added a couple of additional high-level comments. Thanks!

regisb · 2020-10-12T15:15:12Z

@robrap I expect that the suppression of the namespace classes is going to be tedious, but not otherwise complex. So I propose that we resolve the extraction of the waffle and switch classes first. I will then open a new PR to get rid of the many namespace instances. Otherwise we will end up with a PR that touches upon dozens of files, which is inconvenient to review.

regisb · 2020-10-13T10:17:59Z

@robrap This should now be ready to review.

robrap · 2020-10-16T21:15:06Z

+    def _get_flag_active_default(self, namespaced_flag_name):
+        """
+        Return default value in the absence of any other, more specific flag value.


This name and docstring don't really match the implementation, which includes lots of warnings, because this is an exceptional case. What about _get_flag_active_for_missing_request or _get_flag_active_default_for_missing_request?

I'll go with _get_flag_active_no_request to be consistent with the naming of the _get_flag_active_request method.

Otherwise this contradicts edx-platform's requirements

Otherwise the edx-platform root url is overridden, too... This took me a long while to figure it out...

This is necessary to get the flag value in the current request context.

This will be necessary for caching and monitoring.

For now, we keep the namespace classes, but they are meant to go soon.

The constructor was missing the `module_name` argument.

`tox -e pii_check` was failing with the following error: Coverage found 3 uncovered models: waffle.Flag waffle.Sample waffle.Switch We mark these models as safe to pass the pii_check test.

The class attribute `_cached_value` was never being used. Instead, `self._cached_value` was creating an instance-level attribute. So we just get rid of the class attribute.

As these features are destined to be removed, we should not make them available in the edx-toggles shared package.

This simplifies the API, notably for tests which no longer have to manually call the `_get_waffle_flag_custom_attributes_set.clear_cache` function before each test. Instead, the WAFFLE_FLAG_CUSTOM_ATTRIBUTES setting should be a set. Lists are also supported, although they might be slightly less performant, dependending on the number of custom attributes.

* Fix ``SettingDictToggle`` constructor. * Extract waffle classes from edx-platform and move them here. * Fix pinned requirements and incorrect root url that overrode edx-platform's.

robrap

We will follow-up with cleaning out namespacing in a following PR.

openedx-webhooks added blended PR is managed through 2U's blended developmnt program waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 7, 2020

regisb mentioned this pull request Oct 7, 2020

[BD-21] Move waffle_utils from edx-platform to edx-toggles openedx/openedx-platform#25286

Merged

2 tasks

regisb force-pushed the regisb/extract-waffle-utils branch from 235927f to a41e337 Compare October 7, 2020 17:38

robrap reviewed Oct 7, 2020

View reviewed changes

Comment thread edx_toggles/toggles/internal/__init__.py

Comment thread edx_toggles/toggles/internal/base.py Outdated

Comment thread edx_toggles/toggles/internal/base.py

robrap reviewed Oct 7, 2020

View reviewed changes

Comment thread edx_toggles/toggles/internal/waffles.py Outdated

Comment thread edx_toggles/toggles/internal/waffles.py Outdated

Comment thread edx_toggles/toggles/internal/base.py

regisb force-pushed the regisb/extract-waffle-utils branch 2 times, most recently from d211fa6 to e5796a6 Compare October 12, 2020 14:53

regisb force-pushed the regisb/extract-waffle-utils branch 4 times, most recently from 2390052 to acb0bba Compare October 12, 2020 16:59

This was referenced Oct 13, 2020

[BD-21] Use edx-toggle's SettingDictToggle to document setting-based feature toggles openedx/openedx-platform#25332

Merged

[BD-21] Fix SettingDictToggle constructor #71

Closed

regisb force-pushed the regisb/extract-waffle-utils branch from ec83e96 to 190c219 Compare October 13, 2020 10:14

regisb changed the title ~~[BD-21] Extract waffle_utils from edx-platform (WIP)~~ [BD-21] Extract waffle_utils from edx-platform Oct 13, 2020

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 13, 2020

regisb force-pushed the regisb/extract-waffle-utils branch 4 times, most recently from f54692c to 6a5d3c3 Compare October 14, 2020 14:36

robrap reviewed Oct 16, 2020

View reviewed changes

nedbat reviewed Oct 19, 2020

View reviewed changes

Comment thread CHANGELOG.rst Outdated

regisb force-pushed the regisb/extract-waffle-utils branch from 6a5d3c3 to 999ead8 Compare October 22, 2020 10:26

regisb added 2 commits October 22, 2020 12:28

Upgrade django-waffle to match edx-platform version

34ed7c6

Don't pin zipp too precisely

88f63d8

Otherwise this contradicts edx-platform's requirements

regisb added 13 commits October 22, 2020 12:28

Do not override root url in django app!

0dbf80a

Otherwise the edx-platform root url is overridden, too... This took me a long while to figure it out...

[BD-21] Extract WaffleSwitch from edx-platform

8770eeb

Add django-crum as a base requirement

1d7e7a7

This is necessary to get the flag value in the current request context.

Add dependency on edx-django-utils

824c268

This will be necessary for caching and monitoring.

[BD-21] Extract waffle flag classes from edx-platform

414d01b

For now, we keep the namespace classes, but they are meant to go soon.

Fix SettingDictToggle constructor

7543f93

The constructor was missing the `module_name` argument.

Import waffle_utils/testutils.py module from edx-platform

7a6290d

Fix pii_check test

4c8bb69

`tox -e pii_check` was failing with the following error: Coverage found 3 uncovered models: waffle.Flag waffle.Sample waffle.Switch We mark these models as safe to pass the pii_check test.

Simplify testutils' override

e2e4c65

The class attribute `_cached_value` was never being used. Instead, `self._cached_value` was creating an instance-level attribute. So we just get rid of the class attribute.

Extract caching and monitoring features from edx-platform's waffle_utils

cada11b

Move back override features from Waffle classes to edx-platform

d7f9c01

As these features are destined to be removed, we should not make them available in the edx-toggles shared package.

Release v1.0.0

4f690e6

* Fix ``SettingDictToggle`` constructor. * Extract waffle classes from edx-platform and move them here. * Fix pinned requirements and incorrect root url that overrode edx-platform's.

regisb force-pushed the regisb/extract-waffle-utils branch from 999ead8 to 4f690e6 Compare October 22, 2020 10:32

robrap approved these changes Oct 22, 2020

View reviewed changes

regisb merged commit 8de5f3e into openedx:master Oct 22, 2020

regisb deleted the regisb/extract-waffle-utils branch October 22, 2020 17:42

openedx-webhooks added merged and removed needs triage labels Oct 22, 2020

Uh oh!

Conversation

regisb commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

regisb commented Oct 12, 2020

Uh oh!

regisb commented Oct 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

regisb Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

regisb commented Oct 7, 2020 •

edited

Loading

openedx-webhooks commented Oct 7, 2020 •

edited

Loading

regisb Oct 22, 2020 •

edited

Loading