From dfa60a5e92e4d30d0c6eb2476c4495c106630609 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Wed, 6 Jan 2021 09:54:12 -0500 Subject: [PATCH] docs: update sys.path hacks ADR & add upgrade guide The timeline of the import_shims removal has evolved over time; notably, we are keeping the import_shims in Lilac in order to make any lingering old-style import paths obvious to operators. Update the ADR to reflect this. Also, add a table mapping old import paths to new ones. --- .../0007-sys-path-modification-removal.rst | 185 +++++++++++++++++- 1 file changed, 183 insertions(+), 2 deletions(-) diff --git a/docs/decisions/0007-sys-path-modification-removal.rst b/docs/decisions/0007-sys-path-modification-removal.rst index 8bd8d1fa5d..7c62ad9956 100644 --- a/docs/decisions/0007-sys-path-modification-removal.rst +++ b/docs/decisions/0007-sys-path-modification-removal.rst @@ -5,10 +5,12 @@ It used to be acceptable to import code from "lms.djangoapps.something" with `im Soon the old import style will stop working. + Status ====== -In Progress +Complete (as of the Lilac Open edX release) + Context ======= @@ -32,7 +34,7 @@ This deprecation will take place in the following steps: 4. Fix all instances where the ``sys.path``-based modules were ``patch``-ed in unit tests, as those patches no longer work. -5. Once all instances of the ``sys.path``-based imports have been migrated in the ``edx-platform`` codebase, officially deprecate that import pattern for the next open-source release (as per the standard deprecation process). +5. Once all instances of the ``sys.path``-based imports have been migrated in the ``edx-platform`` codebase, officially deprecate that import pattern for the next open-source release. 6. Monitor logs to address continued use of the ``sys.path`` based import patterns. @@ -40,3 +42,182 @@ Goals ===== Eliminate modifications to ``sys.path`` to enable easier tool use in edx-platform. + + +Timeline +======== + +In the Koa release, usages of the old import style raised instances of +``DeprecatedEdxPlatformImportWarning``, but still worked. + +In the Lilac release, usages of old import paths will raise instances of +``DeprecatedEdxPlatformImportError``, breaking any code that was not updated. +This error class is intentionally *not* a subclass of ``ImportError``; +to understand why, consider the following common pattern used in packages outside +of the edx-platform repo itself:: + try: + from student.models import CourseEnrollment + except ImportError: + CourseEnrollment = None +This pattern is (unfortunately) widely used to so packages can import +objects from edx-platform modules in developer or production environments, +whilst falling back to ``None`` when unit tests are run. +Now, if the the above old-style import statement raised a subclass of +``ImportError``, then it would be silently caught by the ``except`` clause! +By instead raising from a disjoint error class in the Lilac release, +we make it more likely that operators can quickly notice and fix +any lingering old-style import paths as they migrate from Koa. + +In Maple and beyond, old import paths will simply raise ``ImportError``, +allowing us to remove the ``import_shims`` directory tree. + + +Upgrade Guide +============= + +Where to look for old import paths: + +* Forks of ``edx-platform`` itself. +* Packages that import edx-platform code (``edx-completion``, ``edx-enterprise``, et al). +* Repositories/files that override Ansible variables or ``edx-platform`` Django settings. + +What forms old import paths may take: + +* Direct imports (e.g. ``import courseware.views``) +* Direct "from" imports (e.g. ``from contentstore.models import VideoUploadConfig``) +* String references to modules (e.g. ``@patch('edxmako.LOOKUP', {})``) +* YAML references to modules (e.g. ``KEY_FUNCTION: util.memcache.safe_key``) + +What the old imports are, and their replacements: + ++-------------------------------+----------------------------------------------+ ++ **Old prefix** | **New prefix** | ++-------------------------------+----------------------------------------------+ +| ``badges`` | ``lms.djangoapps.badges`` | ++-------------------------------+----------------------------------------------+ +| ``branding`` | ``lms.djangoapps.branding`` | ++-------------------------------+----------------------------------------------+ +| ``bulk_email`` | ``lms.djangoapps.bulk_email`` | ++-------------------------------+----------------------------------------------+ +| ``bulk_enroll`` | ``lms.djangoapps.bulk_enroll`` | ++-------------------------------+----------------------------------------------+ +| ``ccx`` | ``lms.djangoapps.ccx`` | ++-------------------------------+----------------------------------------------+ +| ``certificates`` | ``lms.djangoapps.certificates`` | ++-------------------------------+----------------------------------------------+ +| ``commerce`` | ``lms.djangoapps.commerce`` | ++-------------------------------+----------------------------------------------+ +| ``course_api`` | ``lms.djangoapps.course_api`` | ++-------------------------------+----------------------------------------------+ +| ``course_blocks`` | ``lms.djangoapps.course_blocks`` | ++-------------------------------+----------------------------------------------+ +| ``course_goals`` | ``lms.djangoapps.course_goals`` | ++-------------------------------+----------------------------------------------+ +| ``course_home_api`` | ``lms.djangoapps.course_home_api`` | ++-------------------------------+----------------------------------------------+ +| ``courseware`` | ``lms.djangoapps.courseware`` | ++-------------------------------+----------------------------------------------+ +| ``coursewarehistoryextended`` | ``lms.djangoapps.coursewarehistoryextended`` | ++-------------------------------+----------------------------------------------+ +| ``course_wiki`` | ``lms.djangoapps.course_wiki`` | ++-------------------------------+----------------------------------------------+ +| ``dashboard`` | ``lms.djangoapps.dashboard`` | ++-------------------------------+----------------------------------------------+ +| ``debug`` | ``lms.djangoapps.debug`` | ++-------------------------------+----------------------------------------------+ +| ``discussion`` | ``lms.djangoapps.discussion`` | ++-------------------------------+----------------------------------------------+ +| ``edxnotes`` | ``lms.djangoapps.edxnotes`` | ++-------------------------------+----------------------------------------------+ +| ``email_marketing`` | ``lms.djangoapps.email_marketing`` | ++-------------------------------+----------------------------------------------+ +| ``experiments`` | ``lms.djangoapps.experiments`` | ++-------------------------------+----------------------------------------------+ +| ``gating`` | ``lms.djangoapps.gating`` | ++-------------------------------+----------------------------------------------+ +| ``grades`` | ``lms.djangoapps.grades`` | ++-------------------------------+----------------------------------------------+ +| ``instructor`` | ``lms.djangoapps.instructor`` | ++-------------------------------+----------------------------------------------+ +| ``instructor_analytics`` | ``lms.djangoapps.instructor_analytics`` | ++-------------------------------+----------------------------------------------+ +| ``instructor_task`` | ``lms.djangoapps.instructor_task`` | ++-------------------------------+----------------------------------------------+ +| ``learner_dashboard`` | ``lms.djangoapps.learner_dashboard`` | ++-------------------------------+----------------------------------------------+ +| ``lms_initialization`` | ``lms.djangoapps.lms_initialization`` | ++-------------------------------+----------------------------------------------+ +| ``lms_xblock`` | ``lms.djangoapps.lms_xblock`` | ++-------------------------------+----------------------------------------------+ +| ``lti_provider`` | ``lms.djangoapps.lti_provider`` | ++-------------------------------+----------------------------------------------+ +| ``mailing`` | ``lms.djangoapps.mailing`` | ++-------------------------------+----------------------------------------------+ +| ``mobile_api`` | ``lms.djangoapps.mobile_api`` | ++-------------------------------+----------------------------------------------+ +| ``monitoring`` | ``lms.djangoapps.monitoring`` | ++-------------------------------+----------------------------------------------+ +| ``program_enrollments`` | ``lms.djangoapps.program_enrollments`` | ++-------------------------------+----------------------------------------------+ +| ``rss_proxy`` | ``lms.djangoapps.rss_proxy`` | ++-------------------------------+----------------------------------------------+ +| ``shoppingcart`` | ``lms.djangoapps.shoppingcart`` | ++-------------------------------+----------------------------------------------+ +| ``staticbook`` | ``lms.djangoapps.staticbook`` | ++-------------------------------+----------------------------------------------+ +| ``static_template_view`` | ``lms.djangoapps.static_template_view`` | ++-------------------------------+----------------------------------------------+ +| ``support`` | ``lms.djangoapps.support`` | ++-------------------------------+----------------------------------------------+ +| ``survey`` | ``lms.djangoapps.survey`` | ++-------------------------------+----------------------------------------------+ +| ``teams`` | ``lms.djangoapps.teams`` | ++-------------------------------+----------------------------------------------+ +| ``tests`` | ``lms.djangoapps.tests`` | ++-------------------------------+----------------------------------------------+ +| ``verify_student`` | ``lms.djangoapps.verify_student`` | ++-------------------------------+----------------------------------------------+ +| ``course_action_state`` | ``common.djangoapps.course_action_state`` | ++-------------------------------+----------------------------------------------+ +| ``course_modes`` | ``common.djangoapps.course_modes`` | ++-------------------------------+----------------------------------------------+ +| ``database_fixups`` | ``common.djangoapps.database_fixups`` | ++-------------------------------+----------------------------------------------+ +| ``edxmako`` | ``common.djangoapps.edxmako`` | ++-------------------------------+----------------------------------------------+ +| ``entitlements`` | ``common.djangoapps.entitlements`` | ++-------------------------------+----------------------------------------------+ +| ``pipeline_mako`` | ``common.djangoapps.pipeline_mako`` | ++-------------------------------+----------------------------------------------+ +| ``static_replace`` | ``common.djangoapps.static_replace`` | ++-------------------------------+----------------------------------------------+ +| ``status`` | ``common.djangoapps.status`` | ++-------------------------------+----------------------------------------------+ +| ``student`` | ``common.djangoapps.student`` | ++-------------------------------+----------------------------------------------+ +| ``terrain`` | ``common.djangoapps.terrain`` | ++-------------------------------+----------------------------------------------+ +| ``third_party_auth`` | ``common.djangoapps.third_party_auth`` | ++-------------------------------+----------------------------------------------+ +| ``track`` | ``common.djangoapps.track`` | ++-------------------------------+----------------------------------------------+ +| ``util`` | ``common.djangoapps.util`` | ++-------------------------------+----------------------------------------------+ +| ``xblock_django`` | ``common.djangoapps.xblock_django`` | ++-------------------------------+----------------------------------------------+ +| ``api`` | ``cms.djangoapps.api`` | ++-------------------------------+----------------------------------------------+ +| ``cms_user_tasks`` | ``cms.djangoapps.cms_user_tasks`` | ++-------------------------------+----------------------------------------------+ +| ``contentstore`` | ``cms.djangoapps.contentstore`` | ++-------------------------------+----------------------------------------------+ +| ``course_creators`` | ``cms.djangoapps.course_creators`` | ++-------------------------------+----------------------------------------------+ +| ``maintenance`` | ``cms.djangoapps.maintenance`` | ++-------------------------------+----------------------------------------------+ +| ``models`` | ``cms.djangoapps.models`` | ++-------------------------------+----------------------------------------------+ +| ``pipeline_js`` | ``cms.djangoapps.pipeline_js`` | ++-------------------------------+----------------------------------------------+ +| ``xblock_config`` | ``cms.djangoapps.xblock_config`` | ++-------------------------------+----------------------------------------------+