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.

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

feat: add atlas pull support - FC-0012 #950

Conversation

shadinaif
Copy link

@shadinaif shadinaif commented Nov 26, 2023

  • Adding the pull behind a feature flag until it's fully implemented
  • Don't use openedx-i18n translation files when atlas feature is activated

Background

This contribution is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.

@shadinaif shadinaif force-pushed the shadinaif/FC-0012-OEP-58.atlas-pull branch from 27a7755 to 241a757 Compare November 26, 2023 13:47
@shadinaif shadinaif marked this pull request as ready for review November 26, 2023 13:48
@shadinaif
Copy link
Author

Please review @OmarIthawi , @regisb

@shadinaif shadinaif force-pushed the shadinaif/FC-0012-OEP-58.atlas-pull branch 2 times, most recently from 3f45451 to 8bb6a47 Compare November 26, 2023 14:01
@@ -187,6 +187,13 @@ ENV REVISION_CFG /openedx/config/revisions.yml
COPY --chown=app:app settings/lms/*.py ./lms/envs/tutor/
COPY --chown=app:app settings/cms/*.py ./cms/envs/tutor/

{% if OPENEDX_ATLAS_PULL %}
# Support the OEP-58 proposal behind a feature flag until it's fully implemented
RUN pip install openedx-atlas
Copy link
Author

@shadinaif shadinaif Nov 26, 2023

Choose a reason for hiding this comment

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

openedx-atlas is not being installed by default yet, so we need to install it here when the flag is set

Copy link
Contributor

Choose a reason for hiding this comment

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

openedx-atlas==0.5.0 is already installed on master: https://github.com/openedx/edx-platform/blob/master/requirements/edx/base.txt#L781

Do we still need this pip install?

Copy link
Contributor

Choose a reason for hiding this comment

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

@shadinaif now nightly should be tracking quince and atlas is installed on open-release/quince.master:

Copy link
Author

Choose a reason for hiding this comment

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

nice, thank you. Removed

@regisb
Copy link
Contributor

regisb commented Nov 27, 2023

Blocked, pending an answer to my comment: overhangio/tutor-credentials#22 (review)

Copy link
Contributor

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks Shadi. I've added two comments.

Please let me know what do you think and it this is ready for testing again.

@@ -187,6 +187,13 @@ ENV REVISION_CFG /openedx/config/revisions.yml
COPY --chown=app:app settings/lms/*.py ./lms/envs/tutor/
COPY --chown=app:app settings/cms/*.py ./cms/envs/tutor/

{% if OPENEDX_ATLAS_PULL %}
# Support the OEP-58 proposal behind a feature flag until it's fully implemented
RUN pip install openedx-atlas
Copy link
Contributor

Choose a reason for hiding this comment

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

openedx-atlas==0.5.0 is already installed on master: https://github.com/openedx/edx-platform/blob/master/requirements/edx/base.txt#L781

Do we still need this pip install?

@@ -159,7 +159,7 @@ USER ${APP_USER_ID}
# https://hub.docker.com/r/powerman/dockerize/tags
COPY --link --from=docker.io/powerman/dockerize:0.19.0 /usr/local/bin/dockerize /usr/local/bin/dockerize
COPY --chown=app:app --from=edx-platform / /openedx/edx-platform
COPY --chown=app:app --from=locales /openedx/locale /openedx/locale
{% if not OPENEDX_ATLAS_PULL %}COPY --chown=app:app --from=locales /openedx/locale /openedx/locale{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to disable /openedx/locale? It seems that in the edxapp both atlas pull and /openedx/locale can work together without an issue because it's depending on the LOCALE_PATHS variable which accepts multiple directories.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shadinaif please revert those lines if the PR can work okay with them.

Copy link
Author

Choose a reason for hiding this comment

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

Technically they can work together. Done!

@shadinaif shadinaif force-pushed the shadinaif/FC-0012-OEP-58.atlas-pull branch 3 times, most recently from c57df64 to bc27220 Compare December 14, 2023 10:32
Add atlas pull behind a feature flag until it's fully implemented

Refs: FC-0012 OEP-58
@shadinaif shadinaif force-pushed the shadinaif/FC-0012-OEP-58.atlas-pull branch from bc27220 to 26ea636 Compare December 16, 2023 07:48
@OmarIthawi
Copy link
Contributor

@shadinaif please hold off until #964 is merged. We're refactoring Atlas integration with Tutor a bit.

@shadinaif
Copy link
Author

closing as decided here

@shadinaif shadinaif closed this Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Won't fix
Development

Successfully merging this pull request may close these issues.

3 participants