Proposal for addition of some patches

Tutor
1

I’ve been working with Tutor for the use case of eduNEXT and want to propose the addition of some patches that will help us and maybe other people in the community to adopt Tutor as their main tool for edx deployments.

  1. Common env: Similar to the patch common-env-features, this patch will be located in the edxapp config templates (tutor/lms.env.json at master · overhangio/tutor · GitHub, tutor/cms.env.json at master · overhangio/tutor · GitHub) and will help us to add shared settings between the LMS and the CMS.

  2. Edx-platform repo patches: In this case the Tutor openedx Dockerfile currently contains some edx-platform patches that in our use case are not compatible with some of the custom features that we added to the edx-platform repo. So I would like to move this block tutor/Dockerfile at master · overhangio/tutor · GitHub to a plugin patch making its installation optional.

  3. Edx platform extra locales: Similar to the previous one, in our use case we don’t require this block tutor/Dockerfile at master · overhangio/tutor · GitHub and I consider that it may be moved to a plugin patch.

  4. Edx extra requirements patch: Although there is a process described in the docs about how to add extra requirements (Configuration and customisation — Tutor documentation) I found more convenient to add requirements using a patch, so we can use a requirements file that already exists in the repository (as it is in our use case) or we can add some Dockerfile instructions to install the requirements as it works here tutor/Dockerfile at master · overhangio/tutor · GitHub. I was even able to install requirements from private repositories using an extra-requirements patch and the following approach GitHub - bmihelac/docker-images-with-private-python-packages-example: Building docker images with private python packages instead of having to clone them manually and adding them in the private.txt requirements file.

  5. Openedx build partials settings: In a similar way of the patch openedx-lms-production-settings, I wopuld like to have a patch for extra settings in these files: tutor/assets.py at master · overhangio/tutor · GitHub and tutor/i18n.py at master · overhangio/tutor · GitHub. That’s because during our collectsatic and i18ncompile processes we need to set some extra settings to make it work properly.

I just want to start the discussion about those patches and check if they may be useful for the community. Please let me know your comments and thanks in advance for your help.

1 Like
2

Hi @eric.herrera! Your message got me into brainstorming mode, as you are pushing the limits of what Tutor can do. Let’s discuss each item:

There are already multiple patches in the lms.env.json and cms.env.json files, including in the auth.json partial template. Are you worried about code duplication? In that case, you should create two patches, for lms-env and cms-env, and each one of those should contain {% include "yourplugin/partials/common.json" %}. You would then only have to implement the common.json patch.

Did I understand correctly your problem?

This is tricky. First of all, I’d like these patches to remain present by default, because they address security issues and bugs. However, I understand your need to bypass them. If the openedx/Dockerfile file contained a {{ patch("openedx-dockerfile-post-checkout") }} statement, you would then be able to add custom statements, such as git reset --hard origin/open-release/koa.2, and then add your patches there. Would that work for you? If yes, please feel free to open a PR!

I understand that eduNEXT does not need this, but is it problematic for you? Having custom locales baked in the platform is very useful for many users. If I were to do this from scratch again today, I would probably try to implement this with a plugin, but today I am reluctant to remove an existing, useful feature – unless it is preventing other users to customize the platform as they see fit.

By default, requirements are installed from edx-platform/requirements/edx/base.txt. If you already forked edx-platform and added custom requirements there, why not re-compile the base.txt file to make sure that your requirements are properly taken into account? It seems to me that this is the safest way to ensure that you have the right pinned versions across all environments. Remember that requirements from edx-platform/requirements/edx/development.txt are installed in the development image. If requirements from this file conflict with yours, the dev image will not contain the requirements you expect.

We can address this by adding “openedx-common-assets-settings” and “openedx-common-i18n-settings” patches. Feel free to open a PR!

1 Like
3

Hi @regis, thanks for your prompt answer. Let me go pint by point as well

You understood my problem correctly however I tried that with a YAML plugin and Jinja is not able to find the common.json template. Do you think that it is possible to do that with a YAML plugin or this solution will require a Python package plugin ?

Unfortunately it does not work for us since while running the git apply commands the Docker image build fails with errors like:

error: patch failed: lms/djangoapps/support/static/support/js/views/certificates.js:7
error: lms/djangoapps/support/static/support/js/views/certificates.js: patch does not apply
error: patch failed: lms/static/js/certificates/views/certificate_whitelist.js:8
error: lms/static/js/certificates/views/certificate_whitelist.js: patch does not apply
error: patch failed: lms/static/js/courseware/certificates_api.js:13
error: lms/static/js/courseware/certificates_api.js: patch does not apply
error: patch failed: lms/static/js/discovery/views/course_card.js:4
error: lms/static/js/discovery/views/course_card.js: patch does not apply
error: patch failed: lms/static/js/discovery/views/facet.js:3
error: lms/static/js/discovery/views/facet.js: patch does not apply
error: patch failed: lms/static/js/edxnotes/views/note_group.js:1
error: lms/static/js/edxnotes/views/note_group.js: patch does not apply
error: patch failed: lms/static/js/edxnotes/views/note_item.js:2
error: lms/static/js/edxnotes/views/note_item.js: patch does not apply
error: patch failed: lms/static/js/edxnotes/views/tabs/course_structure.js:2
error: lms/static/js/edxnotes/views/tabs/course_structure.js: patch does not apply
error: patch failed: lms/static/js/edxnotes/views/tabs/recent_activity.js:1
error: lms/static/js/edxnotes/views/tabs/recent_activity.js: patch does not apply
error: patch failed: lms/static/js/groups/views/course_cohort_settings_notification.js:1
error: lms/static/js/groups/views/course_cohort_settings_notification.js: patch does not apply
error: patch failed: lms/static/js/student_account/views/account_section_view.js:16
error: lms/static/js/student_account/views/account_section_view.js: patch does not apply
error: patch failed: lms/static/js/verify_student/views/error_view.js:21
error: lms/static/js/verify_student/views/error_view.js: patch does not apply

Another possible solution would be to change the Docker file section to use a conditional using a build -arg, something like this:

ARG EDX_PLATFORM_PACTHES_ENABLED=True
RUN if [ ${EDX_PLATFORM_PACTHES_ENABLED} = "True" ]; then \
        curl https://github.com/overhangio/edx-platform/commit/a4369d300b85d94442844443a3d258a2b516955c.patch | git apply -; \
        .
        .
        .
        curl https://github.com/overhangio/edx-platform/commit/1c733e3ba11249cf16358684169e6137a308e796.patch | git apply -; \
    else \
        echo "Skip application of edx-platform repo patches"; \
    fi

I got the idea from here docker - Dockerfile if else condition with external arguments - Stack Overflow.

Actually we can build the image with that even if we are not using it, so I agree with you in this point.

In our use case we have an extra requirements file which follows the approach that you suggest edunext-platform/requirements/edunext/base.txt at master · eduNEXT/edunext-platform · GitHub. That’s why I would like to have an extra patch (around here tutor/tutor/templates/build/openedx/Dockerfile at master · overhangio/tutor · GitHub) that allow us to do something like:

RUN pip install -r ./requirements/edunext/base.txt

Or as I mentioned before something like this to install private requirements:

ARG SSH_PRIVATE_KEY
RUN mkdir /root/.ssh/
RUN echo "${SSH_PRIVATE_KEY}" > /root/.ssh/id_rsa
RUN chmod 600 /root/.ssh/id_rsa

# make sure your domain is accepted
RUN touch /root/.ssh/known_hosts
RUN ssh-keyscan github.com>> /root/.ssh/known_hosts

# install packages
RUN pip install git+ssh://git@github.com/edunext/example_package_1.git@v1.0.0#egg=example_package_1==1.0.0
RUN pip install git+ssh://git@github.com/edunext/example_package_2.git@v1.0.0#egg=example_package_2==1.0.0
RUN pip install git+ssh://git@github.com/edunext/example_package_3.git@v1.0.0#egg=example_package_3==1.0.0

Do you think this is a good idea ?

Great I am going to open the PR then.

Thanks again for your help and your time .

4

Right. This is because YAML plugins declare patches, not templates. I’m afraid you’ll have to create a Python plugin here. But you are aware there is a convenient plugin cookiecutter, right? GitHub - overhangio/cookiecutter-tutor-plugin: Cookiecutter for tutor plugins

Of course. I think you are right that git patches should only be applied on top of the open-release/koa.2 tag. We could write:

RUN if [ "$EDX_PLATFORM_VERSION" = "{{ OPENEDX_COMMON_VERSION }}" ]; then \
    curl ...
else \
     echo "Skip application of edx-platform repo patches"; \
fi

Would that work for you?

The thing is that if we add a patch below RUN pip install -r ./requirements/edx/base.txt in the openedx/Dockerfile, then we also need to add a patch below RUN pip install -r requirements/edx/development.txt in openedx-dev/Dockerfile. Otherwise, your requirements will be overridden in the dev image.

Instead of including requirements/edx/base.txt from requirements/edunext/base.txt, you could do the opposite and include requirements/edunext/base.txt in requirements/edx/base.txt.

This means there are already two different mechanisms for adding custom requirements. I’d rather avoid adding a third one. On the other hand, I think it’s pretty much the only way to add private requirements. So feel free to open a PR with “openedx-dockerfile-post-python-requirements” and “openedx-dev-dockerfile-post-python-requirements” patches.

5

@eric.herrera could you comment on that?

Do you think this would resolve your issue? Do you have time to open a PR or do you want me (or another @developers) to do it?

6

Hi @regis, sorry for the late response.

I think it should work for us. Do you want me to open the PR ?

Sure, that patch would be useful so I am going to open the PR.

1 Like