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

I18n Workflow Fixes, Improvements & Github App for CI Integration #644

Merged
merged 6 commits into from
Nov 2, 2024

Conversation

zyf722
Copy link
Contributor

@zyf722 zyf722 commented Nov 2, 2024

What type of PR is this? (check all applicable)

  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • ♻️ Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert
  • 🌐 Internationalization / Translation

Description

This PR is intended to fix issues with the i18n workflow introduced in the previous i18n PR.

It also solves the CI issue that PRs created by Github Actions can not trigger other action workflows, by utilizing a dedicated Github App as CI Bot instead.

Documentation has been updated to reflect the changes.

Breaking changes

  • All i18n workflows and former PAT-based workflows (Release / Deploy) have been modified to use the new Github App.
  • Prevent i18n-update-scheduled from pushing changes from Github to Lokalise, as these source changes should have already been maintained on their own i18n branch.

Related Tickets & Documents

Are there any post-deployment tasks we need to perform?

Some of my suggestions on next moves after merging:

  • Set CI_APP_ID and CI_APP_PRIVATE_KEY as repo secrets.
  • Install the Github App into the repo.
  • Do some testing with the release / deploy workflows. If everything works as expected, GH_ACTIONS_REPO_PAT could theoretically be removed.
  • Allow GitHub Actions to create and approve pull requests could be revoked in repo settings, as related actions will be handled by the Github App.
  • Transfer the Github App's ownership to the organization @live-codes.
  • Set the Github App to private.

Copy link

netlify bot commented Nov 2, 2024

βœ… Deploy Preview for livecodes ready!

Name Link
πŸ”¨ Latest commit 68d8997
πŸ” Latest deploy log https://app.netlify.com/sites/livecodes/deploys/6725e4d44e86b40008c30eb7
😎 Deploy Preview https://deploy-preview-644--livecodes.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

sonarcloud bot commented Nov 2, 2024

@hatemhosny hatemhosny merged commit 2531fd4 into live-codes:develop Nov 2, 2024
11 checks passed
@zyf722
Copy link
Contributor Author

zyf722 commented Nov 2, 2024

Hi @hatemhosny,

I believe we can re-test #640 with this fix first. Please let me know once you have completed the first two steps in the checklist mentioned above. Also, please make sure to remove the i18n/live-codes/console-message branch both on GitHub and Lokalise before re-testing.

Then, we can proceed with testing the release/deploy workflows, which correspond to the 3rd and 4th items in the checklist.

After that, I'll set the GitHub App to private and transfer its ownership to the organization, and then you can re-generate the app private key to ensure security.

P.S. I will send you the current private key of the app for testing via email shortly. Please check your inbox. Thanks!

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 2, 2024

I noticed that the i18n badge in README is rendered wrongly due to its outdated url.

Please consider replacing it with the latest links.

@hatemhosny
Copy link
Collaborator

This worked πŸ‘

I noticed that the i18n badge in README is rendered wrongly due to its outdated url.
Please consider replacing it with the latest links.

Done.

Thank you @zyf722

@hatemhosny
Copy link
Collaborator

i18n-update-scheduled failed

https://github.com/live-codes/livecodes/actions/runs/11645452277

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 3, 2024

i18n-update-scheduled failed

live-codes/livecodes/actions/runs/11645452277

I checked the issue and found that the error was due to merge conflicts between develop and i18n/develop branch. This is mainly because other i18n PRs were directly merged into develop branch, then caused some inconsistency between develop and i18n/develop.

To solve this, I made a small patch at #647 that moves the pull step (from develop to i18n/develop) after importing from Lokalise. This way, as soon as the Lokalise master branch is always up-to-date with other merged i18n PRs (by performing merging on Lokalise), this action will work as expected.

Sorry for the inconvenience. Honestly, I have to admit that building such a complex CI pipeline is not as easy as I thought in the first place, and it really requires many trials and errors to make everything work πŸ€”

Edited:
Already merged the live-codes/console-message into master branch on Lokalise.

@hatemhosny
Copy link
Collaborator

Thank you, @zyf722
Merged.

Sorry for the inconvenience. Honestly, I have to admit that building such a complex CI pipeline is not as easy as I thought in the first place, and it really requires many trials and errors to make everything work πŸ€”

Not at all.
I also struggled when making any significant workflows on github actions mainly because I have to debug by pushing new code every time, specially if having to go through PRs.

I thought about using act, but I solved my problems before using it, so I never tried it :)

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 3, 2024

I also struggled when making any significant workflows on github actions mainly because I have to debug by pushing new code every time, specially if having to go through PRs.

Exactly. That's where the problem lies - locally debugging the workflows, reverting changes caused by your mistakes and keeping your environment same with the upstream - have to say, sometimes these things can really be tricky to handle, not to mention when it comes to integration with outer services.

I thought about using act, but I solved my problems before using it, so I never tried it :)

I've heard of act before, but never tried it as well. Maybe I should give it a try someday :)


So I think we've already done some basic testing with the i18n workflow. I think we can now move on to the release / deploy workflows.

Please kindly do some dry runs on the workflows when you have time and let me know if everything works.

@hatemhosny
Copy link
Collaborator

Hi @zyf722

This PR updated the translation key app.logo.title.
After pushing to Lokalise, I expect the live-codes/logo-title branch on Lokalise to have the new value in English locale. However it still holds the old value.
Am I doing something wrong?

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 10, 2024

After pushing to Lokalise, I expect the live-codes/logo-title branch on Lokalise to have the new value in English locale. However it still holds the old value.

Hi @hatemhosny,

Sorry for my delayed response, I have been a bit busy this week. I'll check the issue and get back to you as soon as possible.

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 17, 2024

Hi @hatemhosny,

Just checked the issue, and find out that by default the file upload api of Lokalise won't replace translation that already exists in the project with modified ones in the uploaded file.

I've updated the code with replace_modified set to true explicitly to solve the issue. Please check the PR #650 and try re-running the workflow with clean-ups mentioned before.

Thank you for your patience. Just keep me posted if any other issues arise.

@hatemhosny
Copy link
Collaborator

hatemhosny commented Nov 17, 2024

Thank you @zyf722

The whole workflow now works as expected.
Please see: #649 and #651

After that, I'll set the GitHub App to private and transfer its ownership to the organization, and then you can re-generate the app private key to ensure security.

We may now proceed to moving the GitHub App.

edit: the sync action failed: https://github.com/live-codes/livecodes/actions/runs/11878352243/job/33098927618

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 17, 2024

The whole workflow now works as expected.

Great! I’m glad to hear that :)

We may now proceed to moving the GitHub App.

Just one more question before beginning the transfer process:

Commit a80f535 replace GH_ACTIONS_REPO_PAT with the Github App in deploy, release-pr and release workflow. Do you wish to do some dry runs with them now, or just postpone it until after the transfer?

@hatemhosny
Copy link
Collaborator

Commit a80f535 replace GH_ACTIONS_REPO_PAT with the Github App in deploy, release-pr and release workflow. Do you wish to do some dry runs with them now, or just postpone it until after the transfer?

Honestly, I do not have a dry run setup for these.
However, the GH app did work with repo permission for creating PR and comments. I expect it should work.
So I would like to test the release with the final setup.

also please have a look here:

edit: the sync action failed: live-codes/livecodes/actions/runs/11878352243/job/33098927618

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 17, 2024

I've just sent the transfer request to the organization. Please accept it to continue the process.

also please have a look here:

edit: the sync action failed: live-codes/livecodes/actions/runs/11878352243/job/33098927618

I believe there are some merge conflicts between develop and i18n/develop history, and it's probably caused by our previous failing test runs.

Could you please manually solve the conflicts and merge develop into i18n/develop? Basically it can be done by just keeping all the adding changes from develop. We may need to monitor this workflow for an extra while before deciding whether to do some fixing with it.

@hatemhosny
Copy link
Collaborator

I've just sent the transfer request to the organization. Please accept it to continue the process.

Done. Thank you.

Could you please manually solve the conflicts and merge develop into i18n/develop

Done.
Strangely, the merge conflicts were the last addition by the "logo title" PR.

@hatemhosny
Copy link
Collaborator

The UI update PR is aboout to be ready.

We plan to update language translations once we do that.
I want to try the workflow for modifying the translation files locally.
I can do that for the Arabic translation.
Would you please advise? Shall I edit the json or ts files?

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 17, 2024

I want to try the workflow for modifying the translation files locally.

Would you please advise? Shall I edit the json or ts files?

You could start from the .ts files to do translation. After all authoring is done, you could run npm run i18n-lokalise-json <lang> to generate Lokalise JSON files for the newly-added language and then manually upload them to Lokalise. This is a new script added recently, documented in i18n.md.

@hatemhosny
Copy link
Collaborator

You could start from the .ts files to do translation. After all authoring is done, you could run npm run i18n-lokalise-json <lang> to generate Lokalise JSON files for the newly-added language and then manually upload them to Lokalise. This is a new script added recently, documented in i18n.md.

Great. I will do that πŸ‘

Please check this. The UI PR is almost ready and we will be working on i18n for that. Your guidance there will be very valuable.

Thank you.

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

Successfully merging this pull request may close these issues.

2 participants