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 step to the build - FC-0012 #49

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

shadinaif
Copy link
Contributor

@shadinaif shadinaif commented Nov 15, 2023

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.

@shadinaif
Copy link
Contributor Author

Please review @OmarIthawi, @regisb

Copy link

@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.

One change and this should look good. Thanks @shadinaif!

@shadinaif
Copy link
Contributor Author

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

Copy link

@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 @shadinaif! Looks good to me.

I will test again after the ecommerce pull request is merged.

@shadinaif shadinaif changed the title feat: Add atlas pull to Dockerfile feat: add minimal atlas step to the build - FC-0012 Nov 26, 2023
@shadinaif shadinaif changed the title feat: add minimal atlas step to the build - FC-0012 feat: add atlas step to the build - FC-0012 Nov 26, 2023
@brian-smith-tcril
Copy link

@OmarIthawi openedx/ecommerce#4051 has been merged.

@shadinaif it looks like this one needs a rebase

@OmarIthawi
Copy link

OmarIthawi commented Dec 13, 2023

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.

@shadinaif
Copy link
Contributor Author

rebased, thank you @brian-smith-tcril

Copy link

@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.

@@ -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,

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.

Copy link

@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.

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

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?

Suggested change
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

cc: @Faraz32123 @brian-smith-tcril

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?

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.

Copy link
Contributor Author

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

@OmarIthawi
Copy link

@Faraz32123 This is now ready for merge.

Thanks @shadinaif for the work.

@Faraz32123
Copy link
Collaborator

@shadinaif please add changelog entry.

Adding the pull behind a feature flag until it's fully implemented

Refs: FC-0012 OEP-58
@shadinaif
Copy link
Contributor Author

Change log added @Faraz32123 , thank you!

Copy link
Collaborator

@Faraz32123 Faraz32123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Faraz32123 Faraz32123 merged commit 0c2e064 into overhangio:nightly Jan 25, 2024
@@ -0,0 +1 @@
💥[Feature] Add support to `atlas pull` - FC-0012 project, OEP-58. (by @shadinaif) -->

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.

Suggested change
💥[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

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants