Skip to content
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.

Sign up for GitHub

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

refactor: moves the openedx pip requirements into a patch #670

Merged
merged 2 commits into from
Mar 20, 2024
Merged

refactor: moves the openedx pip requirements into a patch #670

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df65fd6
refactor: moves the openedx pip requirements into a patch
pomegranited Mar 18, 2024
e84ff53
refactor: split requirements into separate lines
pomegranited Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions tutoraspects/patches/openedx-dockerfile-post-python-requirements
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Contributor

@Ian2012 Ian2012 Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we tried this before and it didn't work on Nutmeg Tutor, but my memory is hazy.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, since we've effectively slipped a whole release we may wish to move support to Olive and up if necessary.

Copy link
Contributor Author

@pomegranited pomegranited Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmtcril are you ok with this change?

7 changes: 0 additions & 7 deletions tutoraspects/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,6 @@
("DOCKER_IMAGE_RALPH", "fundocker/ralph:4.1.0"),
("DOCKER_IMAGE_SUPERSET", "edunext/aspects-superset:{{ ASPECTS_VERSION }}"),
("DOCKER_IMAGE_VECTOR", "timberio/vector:0.30.0-alpine"),
(
"OPENEDX_EXTRA_PIP_REQUIREMENTS",
[
"platform-plugin-aspects==0.3.1",
"edx-event-routing-backends==v8.1.1",
],
),
(
"EVENT_SINK_MODELS_ENABLED",
["course_overviews"],
Expand Down