openedx / tutor-contrib-aspects

The Open Analytics Reference System - Tutor plugin
Apache License 2.0
10 stars 14 forks source link

feat: use open edx atlas for both superset and aspects for OEP-58 support #905

Closed OmarIthawi closed 1 week ago

OmarIthawi commented 1 month ago

Translations are hardcoded in aspects:

https://github.com/openedx/tutor-contrib-aspects/blob/8d4e5424bd11633c218e7a7ac498959d806e0c55/tutoraspects/templates/aspects/build/aspects-superset/localization/locale.yaml#L1-L26

These translations should be pulled live from the Dockerfile:

https://github.com/openedx/tutor-contrib-aspects/blob/8d4e5424bd11633c218e7a7ac498959d806e0c55/tutoraspects/templates/aspects/build/aspects-superset/Dockerfile#L35-L36

# Dockerfile#L35-L36
 ...
 +pip install openedx-atlas
 ...
+# Pull latest translations directly from superset `master`
+atlas pull --repository apache/superset --revision=master superset/translations:superset/translations
 COPY ./openedx-assets /app/openedx-assets
-COPY ./localization/ /app/localization/
+# Pull latest aspects translations from openedx-tranlsations repository
+atlas pull translations/tutor-contrib-aspects/tutoraspects/templates/aspects/apps/superset/conf/locale:/app/localization/
+# combine the /app/localization/ files into a single `localization/locale.yaml` file in a way Aspects can use

Edit 1: Existing support is partially there inside the Makefile, which should be moved into the Dockerfile instead:

https://github.com/openedx/tutor-contrib-aspects/blob/8d4e5424bd11633c218e7a7ac498959d806e0c55/Makefile#L88-L92

On the other hand, update for the superset translations support is completely missing:

$ atlas pull --repository apache/superset
OmarIthawi commented 1 month ago

cc: @Ian2012 @bmtcril @brian-smith-tcril @ehuthmacher

This repository wasn't included in the original OEP-58 / FC-0012 scope. Some Dockerfile changes are needed.

bmtcril commented 1 month ago

Thanks @OmarIthawi ! Just to clarify a few things:

OmarIthawi commented 1 month ago

Thanks @OmarIthawi ! Just to clarify a few things:

  • Aspects translations are currently pushed to transifex daily using the usual "Extract Translation Source Files" action in openedx-translations and pulled from openedx-translations weekly using the "Pull Translations" action in this repo

Thanks for the clarification. The push part is covered and well done indeed.

  • I 100% agree that it would be better to pull them at image build time to get the freshest possible translations

That's what's missing and it's fairly an achievable task. Understandably, it needs an engineer time.

  • There is some complexity around the localized Superset assets we'd have to confirm but off hand I don't see any reason why this wouldn't work

šŸ‘šŸ¼

  • I'm less sure about Superset translations, we don't usually do custom translations for 3rd party systems. would it not be better to submit them upstream rather than effectively forking their translations?

Completely agree. That's the plan and we probably don't want to diverge from that.

Or is the concern the turnaround time for getting the translations back into Aspects?

Yes, that's the goal. Getting the most up to date translation would be very beneficial imo. OEP-58's main benefits is to get the engineers out of the Translations path.

Ian2012 commented 1 week ago

This has been applied here: https://github.com/openedx/tutor-contrib-aspects/pull/921