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.

Sign up for GitHub

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

Jump to bottom

feat: enable atlas pull for all MFEs | FC-0012 #184

Merged
merged 1 commit into from
Feb 27, 2024
Merged

feat: enable atlas pull for all MFEs | FC-0012 #184

merged 1 commit into from
Feb 27, 2024

Conversation

OmarIthawi
Copy link
Contributor

@OmarIthawi OmarIthawi commented Jan 29, 2024 •
edited
Loading

Changes

  • Enable atlas pull for all MFEs

TODOs

  • Add instructions in README
  • Add changelog entry

Blocking PRs

This pull request will fail unless the following PRs are merged:

Refs

This pull request is part of the FC-0012 project which implements the Translation Infrastructure update OEP-58.

regisb
regisb reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

Once MFEs are ready, don't forget to add a changelog entry and some instructions in the README ;)

@OmarIthawi OmarIthawi requested a review from regisb February 2, 2024 14:27
@OmarIthawi
Copy link
Contributor Author

@regisb @arbrandes the change log and docs are ready.

We're still pending the rest of MFEs pulll requests to get merged but please let me know if there's anything to change in the docs.

@DawoudSheraz DawoudSheraz self-assigned this Feb 6, 2024
@OmarIthawi OmarIthawi mentioned this pull request Feb 10, 2024
Closed
regisb
regisb reviewed Feb 13, 2024
View reviewed changes
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@OmarIthawi OmarIthawi requested a review from regisb February 19, 2024 07:57
@OmarIthawi
Copy link
Contributor Author

@regisb @DawoudSheraz This is finally ready for review. Please let me know if you have any questions or concerns.

@OmarIthawi OmarIthawi marked this pull request as ready for review February 19, 2024 07:58
regisb
regisb approved these changes Feb 20, 2024
View reviewed changes
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

looks good! @DawoudSheraz @arbrandes what do you think?

DawoudSheraz
DawoudSheraz reviewed Feb 21, 2024
View reviewed changes
changelog.d/20240202_172501_i_atlas_for_all.md Outdated
@@ -0,0 +1 @@
- [Improvement] Enable `atlas pull` on all Micro-frontends. (by @omarithawi)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this be an improvement or feature? I understand we are changing the translation process but it is an entirely new process overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. I'll make it Feature :)

DawoudSheraz
DawoudSheraz reviewed Feb 21, 2024
View reviewed changes
README.rst Outdated
Comment on lines 169 to 175
The ``make pull_translations`` command passes the ``ATLAS_OPTIONS`` environment variable to the ``atlas pull`` command. This allows specifying a custom repository or branch to pull translations from:

To use your own translations repository, you can override the
``ATLAS_REPOSITORY: your-organization/translations`` configuration variable.

If a custom branch is needed, you can override the
``ATLAS_REVISON: your-branch`` configuration variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, the values for ATLAS_REPOSITORY, ATLAS_REVISION, and ATLAS_OPTIONS are not provided by default. They are only needed for overriding the default pipeline, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DawoudSheraz I think the docs weren't clear. I've just pushed an update. Does that answer your question?

@DawoudSheraz DawoudSheraz mentioned this pull request Feb 21, 2024
Merged
2 tasks
@OmarIthawi OmarIthawi force-pushed the atlas-for-all branch 4 times, most recently from ddcfdbb to 165c0eb Compare February 21, 2024 19:52
DawoudSheraz
DawoudSheraz reviewed Feb 22, 2024
View reviewed changes
README.rst Outdated Show resolved Hide resolved
arbrandes
arbrandes requested changes Feb 27, 2024
View reviewed changes
Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Just a couple of README nits, otherwise, looks great!

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@arbrandes arbrandes self-requested a review February 27, 2024 15:26
@arbrandes arbrandes merged commit 829ba70 into overhangio:nightly Feb 27, 2024
@OmarIthawi
Copy link
Contributor Author

OmarIthawi commented Feb 27, 2024 •
edited
Loading

@arbrandes I was going to ping you that notes has been addressed, but you merged it already!

Thanks! If you're using a great GitHub AI bot please let me know, otherwise it's an amazing turnaround 😄

@arbrandes
Copy link
Collaborator

@OmarIthawi, lol, no, I'm just doing a Github notification run now, ordered by newest first. ;)

@OmarIthawi
Copy link
Contributor Author

It's still very fast, but it's a relief that I'm not missing out 😅

@regisb
Copy link
Contributor

regisb commented Feb 27, 2024

@OmarIthawi turns out that this is breaking the build in CI:

linux/amd64 profile-common 5/5] RUN make OPENEDX_ATLAS_PULL=true ATLAS_OPTIONS="--repository=openedx/openedx-translations --revision=main " pull_translations:
0.187 mkdir src/i18n/messages
0.194 cd src/i18n/messages \
0.194       && atlas pull --repository=openedx/openedx-translations --revision=main  \
0.194                translations/frontend-platform/src/i18n/messages:frontend-platform \
0.194                translations/paragon/src/i18n/messages:paragon \
0.194                translations/frontend-component-header/src/i18n/messages:frontend-component-header \
0.194                translations/frontend-component-footer/src/i18n/messages:frontend-component-footer \
0.194                translations/frontend-app-profile/src/i18n/messages:frontend-app-profile
0.195 /bin/sh: 2: atlas: not found
0.196 make: *** [Makefile:62: pull_translations] Error 127
------
Dockerfile:444
--------------------
 442 |     COPY --from=profile-src / /openedx/app
 443 |     
 444 | >>> RUN make OPENEDX_ATLAS_PULL=true ATLAS_OPTIONS="--repository=openedx/openedx-translations --revision=main " pull_translations
 445 |     
 446 |     EXPOSE 1995
--------------------
ERROR: failed to solve: process "/bin/sh -c make OPENEDX_ATLAS_PULL=true ATLAS_OPTIONS=\"--repository=openedx/openedx-translations --revision=main \" pull_translations" did not complete successfully: exit code: 2
Error: Command failed with status 1: docker buildx build --tag=docker.io/overhangio/openedx-mfe:17.0.0-nightly --output=type=image --platform=linux/amd64,linux/arm64 --cache-from=type=registry,ref=docker.io/overhangio/openedx-mfe:17.0.0-nightly-cache /ci/.local/share/tutor-nightly/env/plugins/mfe/build/mfe

Can you please have a look?

@OmarIthawi
Copy link
Contributor Author

Taking a look now @regisb. Could it be that the CI has stale cache?

@OmarIthawi
Copy link
Contributor Author

Some process have removed the openedx-atlas from the frontend app profile:

I think it's a matter of a bad conflict resolution. This should never happen, but that's what CI is for.

The fault isn't in the plugin, so I'll fix the other MFE.

This was referenced Feb 27, 2024
Merged
Merged
@OmarIthawi
Copy link
Contributor Author

Fix is on the way: openedx/frontend-app-profile#976

@arbrandes
Copy link
Collaborator

I just merged the fix. Thanks, @OmarIthawi!

@OmarIthawi
Copy link
Contributor Author

Thanks @arbrandes!! Please let me know if all is good now.

cc: @regisb

@regisb
Copy link
Contributor

regisb commented Feb 28, 2024

Thanks for the super quick turnaround guys :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DawoudSheraz DawoudSheraz DawoudSheraz approved these changes

@arbrandes arbrandes arbrandes approved these changes

@regisb regisb regisb approved these changes

Assignees

@OmarIthawi OmarIthawi

@DawoudSheraz DawoudSheraz

Labels
None yet
Projects
Tutor project management
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@OmarIthawi @arbrandes @regisb @DawoudSheraz