[BD-21] Move waffle_utils from edx-platform to edx-toggles by regisb · Pull Request #25286 · openedx/openedx-platform

regisb · 2020-10-07T17:37:00Z

This PR depends on openedx/edx-toggles#70

TODO:

move unit tests
install edx-toggles from pypi and not from PR

openedx-webhooks · 2020-10-07T17:37:10Z

Thanks for the pull request, @regisb! I've created BLENDED-618 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.

regisb · 2020-10-13T10:18:29Z

@robrap This is ready to review.

robrap

First pass review. Thanks.

robrap

My comments aren't adding much value at this point. I need to have time to review more deeply. I also need to review the edx-toggles code along with this, which I haven't done. :)

robrap · 2020-10-16T00:04:06Z

I still need time to review, but if the old callback was not a required solution, then it was not a great solution. :) I will review more deeply to check that all seems well. Thanks.

robrap

I'll admit that I am scared of this approach. Merging this is going to change all flags at once in a way that we hope is backward compatible. We will then have another large refactor when removing namespacing that will again affect all flags at once.

My original proposed approach would have introduced the new classes as we wanted them into edx-toggles, without namespacing. We could have just updated each flag once, and started with just one flag, or a few at a time, to ensure everything went ok before doing a larger batch or all the rest. My approach seems much safer. I need help understanding the downside to what I am proposing.

For CourseWaffleFlag (and ExperimentWaffleFlag), we would need a new CourseWaffleFlag2 (or some such name) that subclasses the new classes, and we could switch and test a few. When we are happy that all is well, we could replace CourseWaffleFlag with CourseWaffleFlag2 code and the rest would be on the new code.

UPDATE: I have low confidence in my hesitation. It may be I should just do some extra manual testing in a sandbox once we think this is ready.

robrap · 2020-10-16T21:55:41Z

FYI: The whole point of this temporary class is because we used to have the capability to set a default for WaffleFlag and WaffleSwitch, but we removed it because it was too confusing having the value for a flag or switch possibly coming from the database and possibly coming from code. With the new toggle state endpoint, we can now see the values combining what is in the db and what is in code, so it may make sense to simply restore this functionality. If we decide to restore this, it would be in follow-up PRs.

robrap · 2020-10-22T23:06:54Z

Thanks again @regisb.

This is ready to rebase off master.
Maybe add a requirements constraint for edx-toggles so It won’t auto-upgrade. Just add a <2.0.0 constraint to the constraints file.
I will do manual testing once this is green. If my manual testing passes, I will approve/merge. Let me know if you do NOT want me to merge for some reason.

regisb · 2020-10-23T07:00:22Z

@robrap I have rebased and added a edx-toggles<2.0.0 requirement constraint. I will now check wait to see if this is green.

robrap · 2020-10-29T00:27:24Z

Thanks @regisb. I will merge in the morning. Feel free to remind me if I forget.

robrap · 2020-10-29T02:57:29Z

Oops. I left a nit comment in a later PR: https://github.com/edx/edx-platform/pull/25417#discussion_r513890482

robrap

@regisb: I found a 2 layered bug in the toggle state endpoint. Would you be able to address these?

edx-django-utils method get_code_owner_from_module should probably not error if sent None, but should just return None.
The toggle state endpoint probably shouldn't be calling the above method with module of None. This may not be an issue if the first one is addressed?

Traceback (most recent call last):
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/exception.py", line 34, in inner
...
File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/waffle_utils/views.py", line 28, in get
File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/waffle_utils/views.py", line 86, in _get_all_waffle_flags
File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/waffle_utils/views.py", line 102, in _add_waffle_flag_instances
File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/waffle_utils/views.py", line 111, in _add_toggle_instance_details
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/edx_django_utils/monitoring/code_owner/utils.py", line 29, in get_code_owner_from_module

regisb · 2020-10-29T18:26:29Z

I found a 2 layered bug in the toggle state endpoint. Would you be able to address these?

@robrap The fix looks easy enough. Should I do it in this PR, or in another one?

robrap · 2020-10-29T18:53:47Z

@regisb: I think you can fix that in this PR. I think it was caused by this PR. Forget about the edx-django-utils side of the fix for now. I (or you) can do that later. Let's just not call get_code_owner_from_module if we are going to send it None for the module.

robrap

Also, I though I saw a test patching get_code_owner_from_module, but I don't see it now. Could it also use the public API, rather than utils, since it is going to move to an /internal directory.

robrap

Thanks. One more issue.

I grew tired of modifying the Makefile to compile requirements without upgrading them. Also, installing pip-tools should not be part of the compile-requirements target, so a separate target was created.

This makes this class reusable by other IDAs.

This simplifies the internals of the waffle flag classes in order to better move them to edx-toggles later.

The same API is preserved, internally, to avoid many changes across the edx-platform codebase (for now).

Note that those features are destined to be deprecated, eventually.

edx-toggles==2.0.0 is likely to suppress namespace objects for the management of toggle objefcts. We explicitely prevent this by adding a requirement constraint.

When module_name is None, the call to edx-django-utils' get_code_owner_from_module crashes. So we avoid making that call when the module_name is None, which sometimes happens (for good reasons or not, but it's valid behaviour).

regisb · 2020-10-29T22:03:49Z

Ok I rebased my commits, this should be ready to merge! 🚀

edx-status-bot · 2020-10-29T22:29:45Z

Your PR has finished running tests. There were no failures.

robrap · 2020-10-30T00:00:20Z

@regisb: I retested this and will merge in the morning. Thank you!

For a follow-up PR at some point, the difference I saw in the toggle state endpoint on a sandbox was:
Before:

        {
            "class": "CourseWaffleFlag",
            "computed_status": "off",
            "module": "",
            "name": "instructor.enable_data_download_v2"
        },
        {
            "class": "WaffleFlag",
            "computed_status": "off",
            "module": "",
            "name": "instructor.optimised_is_small_course"
        },

After:

        {
            "class": "CourseWaffleFlag",
            "computed_status": "off",
            "module": null,
            "name": "instructor.enable_data_download_v2"
        },
        {
            "class": "WaffleFlag",
            "computed_status": "off",
            "module": null,
            "name": "instructor.optimised_is_small_course"
        },

As I noted earlier, my preference would be that we drop module if it has no value, rather than either of these. Also, I imagine this means that these two flags, wherever they are, aren't passing in __name__.

edx-pipeline-bot · 2020-10-30T13:50:11Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2020-10-30T15:11:29Z

EdX Release Notice: This PR has been deployed to the production environment.

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 force-pushed the regisb/extract-waffle-utils branch 11 times, most recently from e429964 to 8e05827 Compare October 13, 2020 10:14

regisb changed the title ~~[BD-21] Move waffle_utils from edx-platform to edx-toggles (WIP)~~ [BD-21] Move waffle_utils from edx-platform to edx-toggles 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

robrap reviewed Oct 13, 2020

View reviewed changes

robrap reviewed Oct 16, 2020

View reviewed changes

regisb force-pushed the regisb/extract-waffle-utils branch 2 times, most recently from 2f2a2a5 to 48e3e29 Compare October 16, 2020 17:17

robrap reviewed Oct 16, 2020

View reviewed changes

regisb force-pushed the regisb/extract-waffle-utils branch 2 times, most recently from d9cd96e to 865f992 Compare October 22, 2020 19:21

regisb force-pushed the regisb/extract-waffle-utils branch from 865f992 to 2fca046 Compare October 23, 2020 06:52

regisb force-pushed the regisb/extract-waffle-utils branch from cccacf3 to 6be58c1 Compare October 23, 2020 07:44

regisb mentioned this pull request Oct 23, 2020

[BD-21] Switch to edx-toggles classes and deprecate waffle override methods #25417

Merged

robrap reviewed Oct 29, 2020

View reviewed changes

robrap requested changes Oct 29, 2020

View reviewed changes

robrap reviewed Oct 29, 2020

View reviewed changes

Comment thread openedx/core/djangoapps/waffle_utils/views.py Outdated

robrap reviewed Oct 29, 2020

View reviewed changes

Comment thread openedx/core/djangoapps/waffle_utils/views.py Outdated

regisb force-pushed the regisb/extract-waffle-utils branch 2 times, most recently from ede1f5b to 36db9ef Compare October 29, 2020 20:43

robrap reviewed Oct 29, 2020

View reviewed changes

Comment thread openedx/core/djangoapps/waffle_utils/views.py Outdated

regisb added 10 commits October 29, 2020 22:59

Add a make target to compile requirements (without upgrade)

14cb7b6

I grew tired of modifying the Makefile to compile requirements without upgrading them. Also, installing pip-tools should not be part of the compile-requirements target, so a separate target was created.

[BD-21] Migrate WaffleSwitch to edx-toggles

24cf054

This makes this class reusable by other IDAs.

Refactor WaffleFlag and WaffleFlagNamespace internal API

474da0c

This simplifies the internals of the waffle flag classes in order to better move them to edx-toggles later.

[BD-21] Migrate waffle flag classes to edx-toggles

5804372

The same API is preserved, internally, to avoid many changes across the edx-platform codebase (for now).

Move waffle_utils/testutils.py to edx-toggles

db5feec

Backport override features from edx-toggles to waffle_utils

e5500b3

Note that those features are destined to be deprecated, eventually.

Fix waffle_utils unit tests

98ffa34

Add deprecation warning comments to waffle_utils code

a274998

Ensure compatibility with edx-toggles despite upcoming breaking changes

373ee5f

edx-toggles==2.0.0 is likely to suppress namespace objects for the management of toggle objefcts. We explicitely prevent this by adding a requirement constraint.

regisb force-pushed the regisb/extract-waffle-utils branch from 36db9ef to 312f0cd Compare October 29, 2020 22:00

robrap approved these changes Oct 30, 2020

View reviewed changes

robrap merged commit 759cbe2 into openedx:master Oct 30, 2020

openedx-webhooks added merged and removed needs triage labels Oct 30, 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!

regisb commented Oct 13, 2020

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

robrap Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment • 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!

Uh oh!

robrap Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robrap commented Oct 22, 2020

Uh oh!

regisb commented Oct 23, 2020

Uh oh!

robrap commented Oct 29, 2020

Uh oh!

robrap Oct 29, 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!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

regisb commented Oct 29, 2020

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

regisb commented Oct 29, 2020

Uh oh!

edx-status-bot commented Oct 29, 2020

regisb commented Oct 7, 2020 •

edited

Loading

openedx-webhooks commented Oct 7, 2020 •

edited

Loading

robrap left a comment •

edited

Loading

robrap Oct 29, 2020 •

edited

Loading

robrap commented Oct 29, 2020 •

edited

Loading