-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Open edX Redwood release #44
Conversation
I think my comment from the other XBlock applies here as well. :) |
c0f01ff
to
141f9a4
Compare
@fghaas with the upgrade to XBlock 2 and the linked dependencies, I don't see a way to make this XBlock installable between the Redwood and older releases without something breaking. Let me know what you think, maybe I am missing something. |
setup.py
Outdated
@@ -41,7 +41,7 @@ def package_data(pkg, roots): | |||
'markdown_xblock', | |||
], | |||
install_requires=[ | |||
'XBlock<=1.9', | |||
'XBlock>=2.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still wrapping my head around this, so apologies if these questions are misguided or irrelevant:
- Why is the PR description talking about XBlock 2 in the context of Redwood, specifically? https://github.com/openedx/edx-platform/blob/open-release/redwood.master/requirements/edx/base.txt#L1222 seems to indicate that Redwood requires XBlock 4.0.1.
- Given that the current XBlock release is 5.0.0, shouldn't this
install_requires
list say something likeXBlock<5
in order to be accurate for Redwood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. Yes the install_requires
should be XBlock<5
, and the commit message should say add support for XBlock 2 and higher. The breaking changes (deprecation of id_generator) were introduced in 2.0 that's why that upgrade is more significant to mention. Quince
was running with XBlock<2
https://github.com/openedx/edx-platform/blob/open-release/quince.master/requirements/edx/base.txt#L1212
Support versions before and after 2 of the XBlock API. Distinguish using the following logic: * When using a Python version prior to 3.9, assume we need the pre-2 API. * For later Python versions, assume the latest API. This way, we get the correct dependencies installed for Redwood (which uses Python 3.11) and earlier releases (which use 3.8).
Having pipdeptree in the test environment is very helpful in determining what versions of dependencies actually get installed, particularly in a CI pipeline. Add the pipdeptree and pipdeptree-requirements testenvs, and invoke them from the GitHub Actions workflow.
We need to test this XBlock on Python 3.8 (for Open edX releases prior to Redwood), 3.11 (for Redwood), and 3.12 (which is the current latest Python release). There is no real good reason to test on 3.10, so we might as well drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now.
Support versions before and after 2 of the XBlock API.
Distinguish using the following logic:
API.
This way, we get the correct dependencies installed for Redwood (which
uses Python 3.11) and earlier releases (which use 3.8).