From 1cc4ae2080271bb055dd8dbf26856c667bbab80b Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Fri, 25 Sep 2020 15:02:46 -0400 Subject: [PATCH] ARCHBOM-1339: add ADRs for extracting waffle_utils (#24798) - Add ADR for the extraction of waffle_utils to edx-toggles to reuse across IDAs. - Add ADR for temporarily leaving CourseWaffleFlag in edx-platform in order to simplify extraction. - Add ADR for simplifying the interface for WaffleFlag and WaffleSwitch by removing the Namespace classes. ARCHBOM-1339 Co-authored-by: Feanil Patel --- .../0002-waffle-utils-extraction.rst | 35 +++++++++++++++++ .../0003-leave-course-waffle-flag.rst | 31 +++++++++++++++ .../0004-waffle-util-namespacing.rst | 38 +++++++++++++++++++ 3 files changed, 104 insertions(+) create mode 100644 openedx/core/djangoapps/waffle_utils/docs/decisions/0002-waffle-utils-extraction.rst create mode 100644 openedx/core/djangoapps/waffle_utils/docs/decisions/0003-leave-course-waffle-flag.rst create mode 100644 openedx/core/djangoapps/waffle_utils/docs/decisions/0004-waffle-util-namespacing.rst diff --git a/openedx/core/djangoapps/waffle_utils/docs/decisions/0002-waffle-utils-extraction.rst b/openedx/core/djangoapps/waffle_utils/docs/decisions/0002-waffle-utils-extraction.rst new file mode 100644 index 0000000000..9674ef94f5 --- /dev/null +++ b/openedx/core/djangoapps/waffle_utils/docs/decisions/0002-waffle-utils-extraction.rst @@ -0,0 +1,35 @@ +Waffle Utils Extraction +*********************** + +Status +====== + +Accepted + +Context +======= + +The waffle utilities in this app were created in edx-platform, but are generally useful across IDAs. + +Decision +======== + +These utilities will be be moved to `edx/edx-toggles`_ so that they can be used by other IDAs. Additionally, the shared library will use the module name ``toggles``, rather than ``waffle_utils``, so it can more generally include non-waffle based toggle utilities as well. + +.. _edx/edx-toggles: https://github.com/edx/edx-toggles + +Consequences +============ + +* Rollout plan required to deprecate and update class references. +* See ADR 0003-leave-course-waffle-flag for the decision to leave the CourseWaffleFlag behind. +* See ADR 0004-waffle-util-namespacing for decision to change namespacing implementation before extraction. +* The toggle state endpoint, which is meant to be a Django Plugin, could be extracted as a separate step. This requires some additional work: + + * Finishing out work around Django Plugin capabilities in edx-django-utils. + * Adding ability to document CourseWaffleFlag from edx-platform. Note: we may lose the ability to find course override data for toggles no longer in use, by looping through the entire model, unless we add a hook for this from the edx-toggles version. + +* The helper `get_instance_module_name`_ should probably move to `edx_django_utils/monitoring/code_owner`_. It could be considered hacky, but is quite useful. It needs to work whether the class definition is in a library or an IDA, and whether the instance declaration is in a library or an IDA. + +.. _get_instance_module_name: https://github.com/edx/edx-platform/blob/a8c3413a32510dc45301d0c462bf706a5f7ba487/openedx/core/djangoapps/waffle_utils/__init__.py#L521 +.. _edx_django_utils/monitoring/code_owner: https://github.com/edx/edx-django-utils/tree/master/edx_django_utils/monitoring/code_owner diff --git a/openedx/core/djangoapps/waffle_utils/docs/decisions/0003-leave-course-waffle-flag.rst b/openedx/core/djangoapps/waffle_utils/docs/decisions/0003-leave-course-waffle-flag.rst new file mode 100644 index 0000000000..9b5e336c1d --- /dev/null +++ b/openedx/core/djangoapps/waffle_utils/docs/decisions/0003-leave-course-waffle-flag.rst @@ -0,0 +1,31 @@ +Leave CourseWaffleFlag +********************** + +Status +====== + +Accepted + +Context +======= + +It was decided in 0002-waffle-utils-extraction to remove waffle_utils to the shared library edx-toggles. However, moving the class CourseWaffleFlag would be complicated due to its model data, and it is unclear how much usage it would get in other IDAs. + +Decision +======== + +It has been decided to leave CourseWaffleFlag inside edx-platform for the time being, and to delay its extraction until the work seems warranted (i.e. another IDA actually wants to make use of it). + +Consequences +============ + +* The toggle state endpoint will need to be updated to allow WaffleFlag subclasses to add state data. + +Rejected Alternative +==================== + +The alternative would be to extract CourseWaffleFlag to the shared library. As noted, this can be decided in the future. If extraction were to be pursued, some things to consider would be: + +* Ensuring course override data is properly migrated from the old to new method of defining course overrides. It is possible that this migration would need to be documented and maintained for the Open edX release as well. +* Given the data migration needed, it might be practical to migrate course override data from a ConfigurationModel to a Django Setting (using remote config) before extraction. +* The toggle state report uses the term `course_id` for the course overrides, but this is actually a `course_run_id` in the context of other IDAs. Will its name be configurable per IDA? diff --git a/openedx/core/djangoapps/waffle_utils/docs/decisions/0004-waffle-util-namespacing.rst b/openedx/core/djangoapps/waffle_utils/docs/decisions/0004-waffle-util-namespacing.rst new file mode 100644 index 0000000000..91568e8a45 --- /dev/null +++ b/openedx/core/djangoapps/waffle_utils/docs/decisions/0004-waffle-util-namespacing.rst @@ -0,0 +1,38 @@ +Waffle Util Namespacing +*********************** + +Status +====== + +Accepted + +Context +======= + +The toggle classes WaffleFlag and WaffleSwitch rely on several namespace classes (WaffleNamespace, WaffleSwitchNamespace, and WaffleFlagNamespace). In order to create a WaffleFlag (or WaffleSwitch), you must first create a Namespace object. + +Other IDAs have active waffle flags and switches that don't use any namespacing, like this `example switch in ecommerce`_. Once WaffleFlag and WaffleSwitch are extracted to be used in other IDAs (see 0002-waffle-utils-extraction), the required namespace class will make this transition more difficult. + +Additionally, the fully qualified waffle name, including the namespace, is required in code annotations and the django admin. Since it needs to be manually reconstructed by the developer, it has lead to copy/paste issues that are also difficult to lint. + +Lastly, the namespace classes contain a lot of logic, but in effect, they only are used to ensure the flag name has a prefix like '.'. + +.. _example switch in ecommerce: https://github.com/edx/ecommerce/blob/e899c78325ac492d0a2b1ea0aab4d5e230262b8f/ecommerce/extensions/dashboard/users/views.py#L21 + +Decision +======== + +Change the interface to WaffleFlag and WaffleSwitch to simply take the complete flag name, rather than a Namespace object. + +The constructor can assert that the name includes a `.` to help remind people to use some form of prefixed namespace. However, an optional argument with a name like `skip_namespace_assertion=True` could be used to skip this assertion, enabling a simpler transition for existing flags and switches that don't meet this requirement. + +Consequences +============ + +This change will enable WaffleFlag, WaffleSwitch, and all subclasses to have a simpler interface. In addition to a simpler constructor, we will no longer need to differentiate between an instance's namespaced and non-namespaced name. + +A possible rollout plan would be to introduce WaffleFlag and WaffleSwitch classes with the new interface when they are added into edx-toggles, and deprecate the old versions in edx-platform. This would enable us to reuse the same class names with a new import, for an iterative rollout. + +Although it would be nice to update CourseWaffleFlag to have a similar interface for consistency, it is a lower priority if it is not moving to edx-toggles. See 0003-leave-course-waffle-flag.rst. + +This change needs be documented for the next Open edX release.