search

overhangio / tutor-discovery

Course Discovery plugin for Tutor
GNU Affero General Public License v3.0
11 stars 40 forks source link

feat: add minimal `atlas` step to the build FC-0012 #41

Closed OmarIthawi closed 8 months ago

OmarIthawi commented 11 months ago

Add https://github.com/openedx/openedx-atlas to the Dockerfile and run the atlas pull command on every build.

I'm starting here because this is a simple plugin and helps us to design it properly. The end goal is to add atlas on all other components including edX Platform, MFEs and possibly even ecommerce.

This is a very minimal build that needs the following to be viable:

This PR requires the following branch of discovery: https://github.com/openedx/course-discovery/pull/4037

TODO

Building and testing logs ``` # Set the following variables: # DISCOVERY_ATLAS_ARGS: --filter=ar,de # DISCOVERY_ATLAS_PULL: true $ tutor config save $ tutor images build discovery Building image docker.io/overhangio/openedx-discovery:16.0.0 .... $ docker run -it docker.io/overhangio/openedx-discovery:16.0.0 atlas --version v0.4.4 ``` Check the file tree ``` $ docker run -it docker.io/overhangio/openedx-discovery:16.0.0 ls -R course_discovery/conf/locale/ course_discovery/conf/locale/: ar config.yaml de course_discovery/conf/locale/ar: LC_MESSAGES course_discovery/conf/locale/ar/LC_MESSAGES: djangojs.po django.po course_discovery/conf/locale/de: LC_MESSAGES course_discovery/conf/locale/de/LC_MESSAGES: djangojs.po django.po ``` Verify translations within the image: ``` $ docker run -it docker.io/overhangio/openedx-discovery:16.0.0 cat course_discovery/conf/locale/de/LC_MESSAGES/django.po | head -n 100 # #-#-#-#-# django.po (course-discovery) #-#-#-#-# # edX translation file # Copyright (C) 2018 edX # This file is distributed under the GNU AFFERO GENERAL PUBLIC LICENSE. # # Translators: # Translators: # Omar Al-Ithawi , 2023 # msgid "" msgstr "" "Project-Id-Version: edx-platform\n" "Report-Msgid-Bugs-To: \n" "POT-Creation-Date: 2023-08-03 00:22+0000\n" "PO-Revision-Date: 2023-01-26 15:39+0000\n" "Last-Translator: Omar Al-Ithawi , 2023\n" "Language-Team: German (https://app.transifex.com/open-edx/teams/147691/de/)\n" "MIME-Version: 1.0\n" "Content-Type: text/plain; charset=UTF-8\n" "Content-Transfer-Encoding: 8bit\n" "Language: de\n" "Plural-Forms: nplurals=2; plural=(n != 1);\n" #: apps/api/filters.py:47 #, python-brace-format msgid "No user with the username [{username}] exists." msgstr "Ein Nutzer mit Namen [{username}] existiert nicht." #: apps/api/filters.py:50 msgid "" "Only staff users are permitted to filter by username. Remove the username " "parameter." msgstr "" "Nur Mitarbeiter sind berechtigt nach Benutzernamen zu filtern. Entfernen Sie" " den Benutzername Parameter" #: apps/api/serializers.py:813 msgid "Number of courses contained in this catalog" msgstr "Anzahl der Kurse in diesem Katalog" #: apps/api/serializers.py:816 msgid "Usernames of users with explicit access to view this catalog" msgstr "" "Benutzernamen der Nutzer mit ausdrücklichem Zugriff um diesen Katalog zu " "sehen" #: apps/api/serializers.py:976 msgid "Start date cannot be after the End date" msgstr "Startdatum kann nicht nach dem Enddatum gesetzt werden" #: apps/api/serializers.py:981 msgid "Term cannot be changed" msgstr "Begriff kann nicht geändert werden" #: apps/api/serializers.py:993 msgid "Language in which the course is administered" msgstr "Sprache in der dieser Kurs verwaltet wird" ..... ```

Background

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

regisb commented 11 months ago

I converted this PR to a draft, let me know when it's ready for another round of review.

OmarIthawi commented 11 months ago

@regisb I've addressed some of your notes.

I think we still need some way to pass options for example:

Another feature is that atlas pull should be disabled by default, because at the moment it pulls an incomplete translation and it's worse than what's checkout on GitHub.

Atlas and Open edX Translations

Perhaps, it's good to share a recap on what happened on Atlas. Part of OEP-58, we've created two new additions to the Open edX Platform https://github.com/openedx/openedx-translations and https://github.com/openedx/openedx-atlas .

atlas pull replaces tx pull in which that it doesn't pull from Transifex, but pulls from the https://github.com/openedx/openedx-translations GitHub repo which makes it possible to pull without needing authentication or .transifexrc file.

Testing

$ tutor images build discovery
$ tutor dev run discovery -- atlas --help  # Prints the help message

File tree

The file tree seems about right. It removed the old files and add fresh ones from the repository (as opposed to Transifex).

$ tutor dev run discovery -- ls -R course_discovery/conf/locale
course_discovery/conf/locale:
ar  config.yaml  de  en  fr_CA

tutor dev run discovery -- ls -R course_discovery/conf/locale
course_discovery/conf/locale/ar:
LC_MESSAGES

course_discovery/conf/locale/ar/LC_MESSAGES:
djangojs.po  django.po

course_discovery/conf/locale/de:
LC_MESSAGES

course_discovery/conf/locale/de/LC_MESSAGES:
djangojs.po  django.po

course_discovery/conf/locale/en:
LC_MESSAGES

course_discovery/conf/locale/en/LC_MESSAGES:
djangojs.po  django.po

course_discovery/conf/locale/fr_CA:
LC_MESSAGES

course_discovery/conf/locale/fr_CA/LC_MESSAGES:
djangojs.po  django.po
brian-smith-tcril commented 10 months ago

I think we should wait until we implement https://github.com/openedx/openedx-atlas/issues/34 and use pip

OmarIthawi commented 10 months ago

@regisb and @brian-smith-tcril would you mind taking a look at this PR? It should be ready again for review.

OmarIthawi commented 10 months ago

@regisb I've updated the PR except for the args part which I tend to think it's valuable but I'd like to hear from you.

Anyway, it's not a blocker for this PR so we can remove it until we figure out another alternative.

axim $ tutor config save
Configuration saved to /home/omar/work/axim/config.yml
Environment generated in /home/omar/work/axim/env

axim $ tutor images build discovery
Building image docker.io/overhangio/openedx-discovery:16.0.0
....
 => => naming to docker.io/overhangio/openedx-discovery:16.0.0

axim $ tutor dev run discovery bash

app@c91d22d98665:~/discovery$ ls -R course_discovery/conf/locale/
course_discovery/conf/locale/:
ar  config.yaml  de  en  fr_CA

course_discovery/conf/locale/ar:
LC_MESSAGES

course_discovery/conf/locale/ar/LC_MESSAGES:
djangojs.po  django.po

course_discovery/conf/locale/de:
LC_MESSAGES

course_discovery/conf/locale/de/LC_MESSAGES:
djangojs.po  django.po

course_discovery/conf/locale/en:
LC_MESSAGES

course_discovery/conf/locale/en/LC_MESSAGES:
djangojs.po  django.po

course_discovery/conf/locale/fr_CA:
LC_MESSAGES

course_discovery/conf/locale/fr_CA/LC_MESSAGES:
djangojs.po  django.po
OmarIthawi commented 10 months ago

@regisb what do you think should happen if a po file fails to compile due to Translator writing invalid strings Hi {user} --> Merhaba {kullanci}?

Currently the docker build fails, which I think isn't appropriate.

I'm thinking we should add validation to the https://github.com/openedx/openedx-translations so this won't happen inside Tutor, correct?

cc: @brian-smith-tcril

brian-smith-tcril commented 10 months ago

@OmarIthawi I made an issue on the openedx-translations repo for validation https://github.com/openedx/openedx-translations/issues/549

OmarIthawi commented 10 months ago

Thanks Brian and Regis. I'll fix the openedx-translations repo first then I'll continue with this PR.

OmarIthawi commented 10 months ago

@regisb @brian-smith-tcril would mind taking another look?

This took a lot of experimentation and review effort, but I hope it's worth it. I've also made sure that atlas pull gets valid pofiles by validating pofiles at the source repo:

OmarIthawi commented 10 months ago

@regisb the pull request is ready again for review.

OmarIthawi commented 9 months ago

@regisb a gentile reminder about this pull request :)

regisb commented 9 months ago

pulling in @Faraz32123 who is the new maintainer for this plugin.

OmarIthawi commented 9 months ago

@Faraz32123 would you mind checking this pull request?

regisb commented 9 months ago

@OmarIthawi did you see my last comment above?

OmarIthawi commented 9 months ago

@regisb Let's please continue with the feature flag the three of us thinks it's the way to go.

I don't fully understand Tutor branching strategy, so should I change the base branch from master --> nightly?

regisb commented 9 months ago

I don't fully understand Tutor branching strategy, so should I change the base branch from master --> nightly?

Do you want the feature in the Palm release? If yes, then keep the base branch as master, and it will also be included in nightly. If the feature should only target the upstream master branch of the course-discovery repo, aim for the nightly branch here. For more information: https://docs.tutor.overhang.io/tutorials/nightly.html

OmarIthawi commented 9 months ago

@regisb for the discovery, I think it should go to nightly so it's included in Quince.

All other atlas pull requests should go to nightly except for the MFE because the communications MFE needs atlas in Palm.

regisb commented 9 months ago

@regisb for the discovery, I think it should go to nightly so it's included in Quince.

To be clear: if atlas goes to the plugin master branch, the change will also be included in Quince. We should push to nightly only if the change does not work in Palm.

Does that make sense?

OmarIthawi commented 9 months ago

@regisb for the discovery, I think it should go to nightly so it's included in Quince.

To be clear: if atlas goes to the plugin master branch, the change will also be included in Quince. We should push to nightly only if the change does not work in Palm.

Does that make sense?

Yes, makes sense :)

I don't want tutor-discovey to support atlas in Palm. Therefore it'll go to nightly so it's only Quince and beyond.

OmarIthawi commented 8 months ago

@regisb @Faraz32123 I've rebased over nightly and tested the build and it pulled the translations correctly:

Sample output from the build logs ``` $ pip install -e tutor-discovery $ tutor config save && tutor images build discovery ... => [minimal 16/19] RUN atlas pull translations/course-discovery/course_discovery/conf/locale:course_discovery/conf/locale 2.0s ... app@e4c97e0af60c:~/discovery/course_discovery/conf/locale$ git status On branch master Your branch is up to date with 'origin/master'. Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." to discard changes in working directory) modified: ar/LC_MESSAGES/django.mo modified: ar/LC_MESSAGES/django.po modified: ar/LC_MESSAGES/djangojs.mo modified: ar/LC_MESSAGES/djangojs.po modified: da/LC_MESSAGES/djangojs.mo modified: da/LC_MESSAGES/djangojs.po modified: de/LC_MESSAGES/django.mo modified: de/LC_MESSAGES/django.po modified: de/LC_MESSAGES/djangojs.mo modified: de/LC_MESSAGES/djangojs.po modified: de_DE/LC_MESSAGES/djangojs.mo modified: de_DE/LC_MESSAGES/djangojs.po modified: el/LC_MESSAGES/djangojs.mo modified: el/LC_MESSAGES/djangojs.po modified: en/LC_MESSAGES/django.po modified: en/LC_MESSAGES/djangojs.po modified: fr_CA/LC_MESSAGES/django.mo modified: fr_CA/LC_MESSAGES/django.po modified: fr_CA/LC_MESSAGES/djangojs.mo modified: fr_CA/LC_MESSAGES/djangojs.po $ git diff +++ b/course_discovery/conf/locale/ar/LC_MESSAGES/django.po @@ -5,34 +5,17 @@ # # Translators: # Translators: -# 7a89e761fc15f91cc2a9e7bcff1a9c10, 2019 -# Ahmed Jazzar , 2016 -# Amira 111111111111111111 , 2016 -# 6e68c7971a89e50e680ae9444d303c8f, 2016-2017 -# AR R1 , 2016 -# Dillon Dumesnil , 2021 -# e2f_ar r3 , 2016-2017 -# e2f_ar t3 , 2016 -# Hamza Assada <7amza.it@gmail.com>, 2020 -# Hamza Madni, 2022 -# Natalia Berdnikov , 2016 -# NELC Open edX Translation , 2020 -# Omar Al-Ithawi , 2016 -# qualityalltext , 2016 -# Rama Alshebel, 2021 -# Renzo Lucioni , 2017 -# Roaa Nader , 2021 -# Sahbi BG , 2017 -# SAM.AM , 2019 -# shefaa abu jabel , 2016-2017 +# Omar Al-Ithawi , 2023 +# Brian Smith, 2023 +# ```

I think this pull request is ready to be merged.

BTW, we've added translation validation, so if the the feature flag was removed, it would still be production ready, at least up to our knowledge.

OmarIthawi commented 8 months ago

Hey!! That's a great way to end the day. Thanks @regisb!

Thanks for bearing with me Omar!

On the contrary! It's been a great review. I'm new to Tutor anyway, so I'm expecting a lot of comments anyway.