-
Notifications
You must be signed in to change notification settings - Fork 19
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]: Create the Github Action for Sync Translations with POEditor #2672
base: master
Are you sure you want to change the base?
Conversation
Deployed to https://pr-2672.aam-digital.net/ |
…te the another PR
… made any changes
branches: | ||
- feat/translation-poeditor-integration |
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.
We need to update the branch name before merging this PR to the master.
paths: | ||
- "src/assets/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.
This workflow will run only if changes are made to files within this specific directory.
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.
This should be automated via CI as well:
when I merge a new change on master ...
... npm run extract-i18n
is automatically executed
... and the resulting latest .xlf files are then sent to POEditor (as is happening here already, I guess)
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.
@sleidig If we want npm run extract-i18n
to execute automatically, we need to set up a separate GitHub Action workflow. This workflow will run whenever there is an update to the master branch. It will:
- Automatically execute the
npm run extract-i18n
command to update the language files. - Commit and push the updated language files to the master branch.
- Trigger the
Sync Translations with POEditor
workflow.
Is my understanding correct based on your suggestions?
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.
Yes, potentially it could run like this. However, this would mean that after almost every commit to master, there will be another (automatic) commit to update the results from "npm run exxtract-i18n" (this is what I pointed out last week during our call). So that would be far from ideal.
Maybe instead the CI could work like this?
- run "npm run extract-i18n within CI without committing it
- check the result of this and trigger the push action to POEditor (if there were changes)
- open a PR with the new xlf files (rather than immediately committing them to master)
- update that branch/PR whenever POEditor users update a translation?
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.
@sleidig Understood. Instead of creating a new flow, I think we can update the existing workflow. Here are the small changes I propose:
- Remove the condition that restricts the workflow to run only when specific files change.
- Add a step after the
npm run extract-i18n
command to check if there are any updates on the branch; the workflow should only run if there are updates, otherwise, it should be skipped. - Continue sending the updated language files to POEditor and follow the remaining steps as they are already implemented in this workflow.
response=$(curl -s -X POST https://api.poeditor.com/v2/projects/upload \ | ||
-F "api_token=${{ secrets.POEDITOR_API_TOKEN }}" \ | ||
-F "id=${{ secrets.POEDITOR_PROJECT_ID }}" \ | ||
-F "language=$lang" \ | ||
-F "updating=terms_translations" \ | ||
-F "overwrite=1" \ | ||
-F "sync_terms=1" \ | ||
-F "file=@${files[$lang]}") | ||
status=$(echo $response | jq -r '.response.status') | ||
if [ "$status" != "success" ]; then | ||
echo "Error uploading file for $lang: $response" | ||
exit 1 | ||
fi |
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.
This is the POEditor upload API, where need to send the language xlf
file. For reference, here is the documentation of the POEditor Upload API.
echo "Waiting for 30 second to respect rate limits..." | ||
sleep 30 |
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.
Added the sleep interval, ensuring compliance with the rate limit of one request every 20 seconds.
response=$(curl -s -X POST https://api.poeditor.com/v2/projects/export \ | ||
-F "api_token=${{ secrets.POEDITOR_API_TOKEN }}" \ | ||
-F "id=${{ secrets.POEDITOR_PROJECT_ID }}" \ | ||
-F "language=$lang" \ | ||
-F "type=xlf") | ||
status=$(echo $response | jq -r '.response.status') | ||
if [ "$status" == "success" ]; then | ||
url=$(echo $response | jq -r '.result.url') | ||
if [ -n "$url" ]; then | ||
echo "Downloading translations for $lang from $url" | ||
if [ "$lang" == "en" ]; then | ||
curl -s -o src/assets/locale/messages.xlf $url | ||
else | ||
curl -s -o src/assets/locale/messages.$lang.xlf $url | ||
fi | ||
else | ||
echo "No URL found for downloading translations for $lang" | ||
exit 1 | ||
fi |
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.
This is the POEditor export API, where we can export the updated language files. For reference, here is the documentation of the POEditor Export API.
This is the Automated PR generated by the |
… made any changes
@sleidig As we discussed over the call, it is currently not creating automated PRs. Last time, it created this Automated PR. But now, it is creating a branch and pushing it to the origin, but it is not creating a Pull Request. I need to check with @tomwwinter once to see if there have been any changes to the workflow permissions. |
…b.com/Aam-Digital/ndb-core into feat/translation-poeditor-integration
|
||
- name: Create or update branch for translations | ||
run: | | ||
branch_name="chore/update-poeditor-translations" |
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.
When is this action supposed to run? On each PR? Then a fixed branch name would overwrite changes from other PRs.
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.
Yes, this action will run after every commit on the master branch. I believe we don’t need to create another PR if the translation automated PR has already been created. Instead, we can update the language files in the same PR. What do you suggest in this?
- name: Check if any files changed | ||
run: | | ||
if git diff --quiet; then | ||
echo "No changes detected." | ||
echo "changed=false" >> $GITHUB_ENV | ||
else | ||
echo "Changes detected." | ||
echo "changed=true" >> $GITHUB_ENV | ||
fi |
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.
This change detection could also be handled at Github Action level:
on:
push:
paths:
- 'src/assets/locale/*.xlf'
branches:
- feat/translation-poeditor-integration
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.
@tomwwinter Yes, this can be managed at the GitHub Action level, and I have implemented it that way before. However, following Sebastian's suggestion, we can handle this through CI. For reference, please check this comments. Please let me know, if I need to changes this.
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, missed that conversation.
…b.com/Aam-Digital/ndb-core into feat/translation-poeditor-integration
@tomwwinter I have updated the changes according to your suggestions. And replied to your questions. Can you please check and let me know if there are any changes required. |
closes: #2642
Description of changes