refactor: moves the openedx pip requirements into a patch #670
Conversation
instead of using the OPENEDX_EXTRA_PIP_REQUIREMENTS variable. This allows Tutor operators to use this variable to add their own extra requirements.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
| @@ -0,0 +1,2 @@ | |||
| RUN --mount=type=cache,target=/openedx/.cache/pip,sharing=shared \ | |||
| pip install "platform-plugin-aspects==0.3.1" "edx-event-routing-backends==v8.1.1" | |||
There was a problem hiding this comment.
Thanks for this change. Can you split every requirement into a separate line?
Have you tested this with mounts and verify that the dev installation remains as the one installed instead of the defined version here?
Is this compatible with older versions of tutor?
There was a problem hiding this comment.
I think we tried this before and it didn't work on Nutmeg Tutor, but my memory is hazy.
There was a problem hiding this comment.
That said, since we've effectively slipped a whole release we may wish to move support to Olive and up if necessary.
There was a problem hiding this comment.
Can you split every requirement into a separate line?
Absolutely: e84ff53
Have you tested this with mounts and verify that the dev installation remains as the one installed instead of the defined version here?
I have not tested this with dev mounts, no. But this patch gets applied before OPENEDX_EXTRA_PIP_REQUIREMENTS are installed, so you can always override that variable in your dev env to install different versions.
Is this compatible with older versions of tutor?
Yes. This patch has been around since Tutor v11.2.7, and is definitely in v14.0.0 / Nutmeg.
I think we tried this before and it didn't work on Nutmeg Tutor, but my memory is hazy.
I don't remember trying it.. but I can test it if we need to make sure.
There was a problem hiding this comment.
@bmtcril are you ok with this change?
|
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
itsjeyd
added
the
core contributor
Uses a patch to add extra python requirements to Open edX instead of using the
OPENEDX_EXTRA_PIP_REQUIREMENTSvariable.I've been running into issues with using Aspects with other Tutor plugins that also overwrite
OPENEDX_EXTRA_PIP_REQUIREMENTS, and someone on my team suggested this patch approach.This change allows Tutor operators to use this variable to override or add their own extra requirements without disrupting Aspects.
Testing instructions
Rebuild your
openedx(-dev)image using this branch.Note that the platform-plugin-aspects and edx-event-routing-backends packages get installed as expected.