feat: add atlas pull with global settings | FC-0012 #964
Conversation
|
cc: @regisb |
|
Can we merge this PR and #950 in a single one? |
| @@ -2,6 +2,8 @@ | |||
| # This file includes all Tutor setting defaults. Settings that do not have a | |||
| # default value, such as passwords, should be stored in base.yml. | |||
| # This must be defined early | |||
| ATLAS_BRANCH: "main" | |||
| ATLAS_REPOSITORY: "openedx/openedx-translations" | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
That's how atlas works at the moment. There's an open issue to support non-GitHub repositories but that's not planned yet:
There was a problem hiding this comment.
OK, that's a restriction that must be documented in the Tutor docs.
There was a problem hiding this comment.
@regisb where should we add the section for Atlas docs?
There was a problem hiding this comment.
It should go somewhere in this page: https://docs.tutor.edly.io/configuration.html
tutor/templates/config/defaults.yml
Outdated
| @@ -2,6 +2,8 @@ | |||
| # This file includes all Tutor setting defaults. Settings that do not have a | |||
| # default value, such as passwords, should be stored in base.yml. | |||
| # This must be defined early | |||
| ATLAS_BRANCH: "main" | |||
There was a problem hiding this comment.
Should be "ATLAS_REVISION" and it should be "main" only in nightly, based on the value of "OPENEDX_COMMON_VERSION".
OmarIthawi
changed the title
|
@shadinaif @regisb would you mind reviewing this pull request based on the latest refactoring? Please note that this pull request replaces #950 I'll continue improving the pull request and testing it on my machine and post screenshot. Meanwhile, I'd love a quick validation of the approach and the breaking changes. |
docs/troubleshooting.rst
Outdated
| @@ -158,7 +158,9 @@ The error produced should help you better understand what is happening. | |||
| The chosen default language does not display properly | |||
| ----------------------------------------------------- | |||
|
|
|||
| By default, Open edX comes with a `limited set <https://github.com/openedx/edx-platform/blob/master/conf/locale/config.yaml>` of translation/localization files. To complement these languages, we add locales from the `openedx-i18n project <https://github.com/openedx/openedx-i18n/blob/master/edx-platform/locale/config-extra.yaml>`_. But not all supported locales are downloaded. In some cases, the chosen default language will not display properly because it was not packaged in either edx-platform or openedx-i18n. If you feel like your language should be packaged, please `open an issue on the openedx-i18n project <https://github.com/openedx/openedx-i18n/issues>`_. | |||
| By default, Open edX comes with a `limited set <https://github.com/openedx/openedx-translations/tree/main/translations/edx-platform/conf/locale>` of translation/localization files. To complement these languages, we add locales from the `openedx-i18n project <https://github.com/openedx/openedx-i18n/blob/master/edx-platform/locale/config-extra.yaml>`_. | |||
There was a problem hiding this comment.
This paragraph should not contain references to openedx-i18n anymore.
| # and we need to do a pass ourselves. Also, we need to compile the djangojs.js files for | ||
| # the downloaded locales. | ||
| # Pull latest translations via atlas | ||
| RUN atlas pull {{ ATLAS_OPTIONS }} translations/edx-platform/conf/locale:conf/locale |
There was a problem hiding this comment.
I think that ATLAS_OPTIONS should be empty by default, and we should explicitely pass --repository='{{ ATLAS_REPOSITORY }}' --branch '{{ ATLAS_REVISION }}' to all atlas pull commands. Like so:
RUN atlas pull --repository='{{ ATLAS_REPOSITORY }}' --branch '{{ ATLAS_REVISION }}' {{ ATLAS_OPTIONS }} translations/edx-platform/conf/locale:conf/locale
Otherwise, users need to be smart about properly setting the upstream repository whenever they want to customise atlas options.
There was a problem hiding this comment.
seems appropriate. done.
| @@ -2,6 +2,9 @@ | |||
| # This file includes all Tutor setting defaults. Settings that do not have a | |||
| # default value, such as passwords, should be stored in base.yml. | |||
| # This must be defined early | |||
| ATLAS_REVISION: "{% if OPENEDX_COMMON_VERSION == 'master' %}main{% else %}{{ OPENEDX_COMMON_VERSION }}{% endif %}" | |||
There was a problem hiding this comment.
Do you expect to maintain "open-release/xxx.master" branches and "open-release/xxx.y" tags? I see that the openedx-translations repo does not have an openedx.yaml file.
There was a problem hiding this comment.
Do you expect to maintain "open-release/xxx.master" branches and "open-release/xxx.y" tags?
Not really.
We don't have a plan for exactly that, but the intention is to keep a single open-release/redwood.master (or release/redwood, depending on the BTR group naming convention) branch for example but not open-release/xxx.y tags because we need operators to stay up to date with the release translations.
How do you recommend we proceed with this variable?
I see that the openedx-translations repo does not have an openedx.yaml file.
I'm a bit behind on that front. I've created an issue and asked Brian Smith to follow up:
OmarIthawi
changed the title
|
@regisb this pull request has been tested pretty well and I think it's ready to be merged. For reproducible builds and release branches, I've created a decision to discuss that: |
There was a problem hiding this comment.
Awesome! Thanks for this important contribution Omar. Let's just keep in mind that we'll need release branches and tags in time for Redwood openedx/openedx-translations#3183
We will, I'll need to conclude that decision and handover the work to the BTR Working Group. |
Add
atlasdefault variables for other plugins to use.I chose to make it simple.
Another option I've had is to enable a third variable:
This is useful to allow the languages and add other atlas arguments. I'm open to suggestions.
TODO
git status after Docker build
Variables test
This pull request is part of the FC-0012 project which implements the Translation Infrastructure update OEP-58.