Skip to content

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

Merged
robrap merged 6 commits into
openedx:masterfrom
regisb:regisb/deprecate-toggle-override
Nov 3, 2020
Merged

[BD-21] Switch to edx-toggles classes and deprecate waffle override methods#25417
robrap merged 6 commits into
openedx:masterfrom
regisb:regisb/deprecate-toggle-override

Conversation

@regisb

@regisb regisb commented Oct 23, 2020

Copy link
Copy Markdown
Contributor

This move is part of a migration from edx-platform's waffle_utils to edx-toggles.

This depends on an edx-toggles PR: openedx/edx-toggles#75 Merged ✔️
This depends This does not depend anymore on edx/completion PR: openedx/completion#118

This is not yet ready for review.

TODO:

@openedx-webhooks

openedx-webhooks commented Oct 23, 2020

Copy link
Copy Markdown

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

@openedx-webhooks 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 23, 2020
@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch from 0bebabc to 200ba98 Compare October 23, 2020 13:01
@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch 8 times, most recently from 7ce2969 to 2f03966 Compare October 26, 2020 08:41
@regisb

regisb commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

The remaining broken unit test looks flaky to me, as I can't reproduce the behaviour locally.

@regisb

regisb commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

This should now be ready to review cc @robrap

@regisb regisb changed the title [BD-21] Deprecate waffle override methods (WIP) [BD-21] Deprecate waffle override methods Oct 26, 2020
@openedx-webhooks openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 26, 2020
@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch 7 times, most recently from 80f1fc9 to 1c19226 Compare October 28, 2020 08:33

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For many of my comments, it may be ok to refactor, to add an appropriate comment, or to explain why you don't think either is appropriate. You'll see. :)
Thanks.

Comment thread openedx/core/djangoapps/waffle_utils/__init__.py Outdated
Comment thread cms/djangoapps/contentstore/views/tests/test_videos.py Outdated
Comment thread lms/djangoapps/certificates/tests/test_signals.py Outdated
Comment thread lms/djangoapps/course_api/blocks/tests/test_api.py Outdated
Comment thread lms/djangoapps/courseware/tests/test_module_render.py Outdated
Comment thread openedx/core/djangoapps/waffle_utils/tests/test_init.py Outdated
Comment thread openedx/core/djangoapps/waffle_utils/testutils.py Outdated
Comment thread openedx/core/djangoapps/waffle_utils/__init__.py Outdated
Comment thread openedx/core/djangoapps/waffle_utils/__init__.py Outdated
@regisb

regisb commented Oct 29, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review of this difficult PR @robrap I appreciate it :) I pushed additional changes to resolve some of your comments. Please mark them as resolved accordingly (or comment).

@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch from 91177fa to 902274b Compare October 29, 2020 12:11
@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch 6 times, most recently from 49d6dba to 3a12cba Compare October 31, 2020 10:02

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Just some minor updates needed, as well as handling conflicts.

Comment thread openedx/core/djangoapps/waffle_utils/__init__.py Outdated
Comment thread openedx/core/djangoapps/waffle_utils/__init__.py Outdated
Comment thread openedx/core/djangoapps/waffle_utils/__init__.py Outdated
@robrap robrap changed the title [BD-21] Deprecate waffle override methods [BD-21] Switch to eedx-toggles classes and deprecate waffle override methods Nov 3, 2020
@robrap robrap changed the title [BD-21] Switch to eedx-toggles classes and deprecate waffle override methods [BD-21] Switch to edx-toggles classes and deprecate waffle override methods Nov 3, 2020
@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch from 3a12cba to 536f57b Compare November 3, 2020 08:30
@regisb

regisb commented Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

@robrap I rebased my PR to resolve conflicts and added just one commit with the proposed changes, to make it easier to review.

@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch from 536f57b to e1551d4 Compare November 3, 2020 09:29

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Let me know when this is squashed and ready to merge.

This is part of a removal of the many override methods of toggle
flag/namespace classes. This allows us to remove imports of test modules
from production code.
This allows us to get rid of waffle_utils' custom WaffleFlag method.
This allows us to get rid of the custom WaffleSwitch and
WaffleSwitchNamespace classes from waffle_utils in favour of
edx_toggles.toggles classes.
Those classes were ported to edx_toggles. The imports remain in
waffle_utils.__init__ for backward compatibility.
These functions should from now on be imported from
edx_toggles.toggles.testutils.
This is for backward compatibility.
@regisb
regisb force-pushed the regisb/deprecate-toggle-override branch from e1551d4 to c9c1136 Compare November 3, 2020 18:25
@regisb

regisb commented Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

This is squashed and ready to merge.

@edx-status-bot

Copy link
Copy Markdown

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

@robrap
robrap merged commit 4a0e4aa into openedx:master Nov 3, 2020
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

@regisb
regisb deleted the regisb/deprecate-toggle-override branch November 3, 2020 21:11
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blended PR is managed through 2U's blended developmnt program merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants