-
Notifications
You must be signed in to change notification settings - Fork 7
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 pull --expand-glob
to expand bash glob file patterns
#45
Conversation
Thanks for the pull request, @OmarIthawi! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
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.
Left a tiny comment in the rejected alternatives part of the decision doc, but nothing that I feel should block merging.
Dismissed alternatives | ||
********************** | ||
|
||
1. Support links again: | ||
----------------------- | ||
|
||
Previous tests on unknown git version showed to support links in April 2023, | ||
but it had no automated tests and broke somewhere during the | ||
`git v2.25.1 version support`_ we've added. | ||
|
||
The sparse-checkout part is already complex, so we'll not touch it. | ||
|
||
2. Make complete copies of the translations in `edx-platform-links`_ | ||
--------------------------------------------------------------------- | ||
This is the most viable and simple alternative to glob patterns. | ||
|
||
If it turns out the ``--expand-glob`` option is flaky to work with, we'll | ||
fallback to this. | ||
|
||
This _may_ complicate the GitHub Transifex App sync process which we prefer | ||
to keep simple and avoid adding additional step to ensure the | ||
`edx-platform-links`_ have full copies of the XBlock and plugins translations | ||
instead of symbolic links. | ||
|
||
3. Use the `edx-platform-links`_ to store the original files | ||
------------------------------------------------------------ | ||
|
||
This is a viable option as well but needs to refactor the Transifex | ||
configuration which could reset the translations. | ||
|
||
Therefore, we'll only fallback to this if the glob patterns has problems. |
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.
It's not fully clear to me how these alternatives would accomplish what is being done with --expand-glob
It seems each of the alternatives has a great description of why we decided against it, but not a full explanation of how it would work if we decided to go that route.
I also think the lines about which alternative we'll fall back to if the glob expansion is problematic can be removed. We can reference the alternatives we rejected and decide on a new path forward if that time comes.
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.
Good points. Both are addressed.
I'll merge once tests passes.
Example: 'atlas pull translations/*/done' pulls 'atlas pull translations/DoneXBlock/done' if it exists.
@OmarIthawi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 0.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Expand glob pattern in
directory_from
which is useful for instance to pull XBlocks in which the directory name isn't known but the module name is.Check the attached
0001-support-glob-pattern.rst
decision document for more context.This contribution is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.
Example
atlas pull -g translations/*/done
pullsatlas pull translations/DoneXBlock/done
if it exists.TODO
edx-platform-links
from translations repoatlas pull
for the edx-platform and its plugins FC-0012 edx-platform#33166