-
Notifications
You must be signed in to change notification settings - Fork 50
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 step to the build - FC-0012 #49
feat: add atlas step to the build - FC-0012 #49
Conversation
Please review @OmarIthawi, @regisb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One change and this should look good. Thanks @shadinaif!
5bf64a0
to
eb3625d
Compare
eb3625d
to
2491e47
Compare
Done @OmarIthawi . Also, this PR needs openedx/ecommerce#4051 to be merged first. And that's for one reason: atlas is added to requirements in ecommerce PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shadinaif! Looks good to me.
I will test again after the ecommerce pull request is merged.
2491e47
to
2c54970
Compare
2c54970
to
1543de9
Compare
@OmarIthawi openedx/ecommerce#4051 has been merged. @shadinaif it looks like this one needs a rebase |
Thanks @brian-smith-tcril for taking care of the ecommerce PR! Yup, Quince has been tagged. So this one will land post-quince. We'll rebase, test and let you know when it's ready. |
1543de9
to
09bc517
Compare
rebased, thank you @brian-smith-tcril |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadinaif please refactor in favor of the new integration:
tutorecommerce/plugin.py
Outdated
@@ -31,6 +31,7 @@ | |||
"OAUTH2_KEY_SSO": "ecommerce-sso", | |||
"OAUTH2_KEY_SSO_DEV": "ecommerce-sso-dev", | |||
"WORKER_JWT_ISSUER": "ecommerce-worker", # TODO do we need to keep this? | |||
"ATLAS_PULL": False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this variable. No feature flag is required anymore.
09bc517
to
2f355ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think this is ready to get merged unless we want to use the new --revision
argument.
@@ -79,6 +80,9 @@ RUN cd /openedx/requirements/ \ | |||
{% for extra_requirement in ECOMMERCE_EXTRA_PIP_REQUIREMENTS %}RUN --mount=type=cache,target=/openedx/.cache/pip,sharing=shared,uid=${APP_USER_ID} pip install '{{ extra_requirement }}' | |||
{% endfor %} | |||
|
|||
RUN atlas pull --repository="{{ ATLAS_REPOSITORY }}" --branch="{{ ATLAS_REVISION }}" {{ ATLAS_OPTIONS }} translations/ecommerce/ecommerce/conf/locale:ecommerce/conf/locale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadinaif There's a very new addition to atlas:
Would you like to implement it in this pull request or create a new one after this one is merged?
RUN atlas pull --repository="{{ ATLAS_REPOSITORY }}" --branch="{{ ATLAS_REVISION }}" {{ ATLAS_OPTIONS }} translations/ecommerce/ecommerce/conf/locale:ecommerce/conf/locale | |
RUN atlas pull --repository="{{ ATLAS_REPOSITORY }}" --revision="{{ ATLAS_REVISION }}" {{ ATLAS_OPTIONS }} translations/ecommerce/ecommerce/conf/locale:ecommerce/conf/locale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadinaif could you please accept that suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meanwhile, I've upgraded the ecommerce PR:
We need to merge that one first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated and rebased, waiting for 4108 to be merged
2f355ad
to
6450b8a
Compare
@Faraz32123 This is now ready for merge. Thanks @shadinaif for the work. |
@shadinaif please add changelog entry. |
Adding the pull behind a feature flag until it's fully implemented Refs: FC-0012 OEP-58
6450b8a
to
62ca854
Compare
Change log added @Faraz32123 , thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -0,0 +1 @@ | |||
💥[Feature] Add support to `atlas pull` - FC-0012 project, OEP-58. (by @shadinaif) --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadinaif I found few typos after merging this pull request. Please fix them if needed.
💥[Feature] Add support to `atlas pull` - FC-0012 project, OEP-58. (by @shadinaif) --> | |
- [Feature] Add support to `atlas pull` - FC-0012 project, OEP-58. (by @shadinaif) |
cc: @Faraz32123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank @OmarIthawi , fixed here #66
Adding the pull behind a feature flag until it's fully implemented
IMPORTANT: This PR needs these to be merged first
Background
This contribution is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.