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

Add update locales workflow to l10n_main branch #3211

Open
wants to merge 9 commits into
base: l10n_main
Choose a base branch
from

Conversation

tararoshan
Copy link
Contributor

Relates to issue #2706. The workflow requires GitHub Actions to have access
to create and approve PRs in order for the workflow to successfully create a PR.


I did this in my fork in the project settings like so:

image

I noticed, however, that changes to files outside of the `locale` folder are added to the PR as well. Let me know if that's an issue and I'll try to find a workaround!

@mouse-reeve
Copy link
Member

I set up the update locales command to only pull in locales that are manually added to a list; this is more work but it means that only locales that are in use are shown, and it means less files. I'm no longer sure that that's worth the downsides of making things more complicated, but that might not be a question for today :)

I think if you remove the git commands that merge in l10n_main, and just do a git add and commit, the right files will be in the commit and the additional ones will remain excluded. The ./bw-dev update_locales command includes a step that pulls files over from that branch, so it shouldn't be necessary to do it again.

@tararoshan
Copy link
Contributor Author

Got it! I removed the merge command and set the workflow up as you recommended. I think it works, based on the new commit to the main branch from GitHub Actions on my fork! Let me know if anything seems fishy. :)

- rename job `build` -> `update_locales`; makes it easier to run locally
  with tools like "act"

- ensure checkout@v3 checks out `main` branch (which it doesn't by default
  since the workflow runs on l10n_main

- remove now unneeded switch to `main` by hand

- rebase commit before publishing, to minimize risk of failed push
dato added a commit to dato/bookwyrm that referenced this pull request Jan 27, 2024
Copy link
Contributor

@dato dato left a comment

Choose a reason for hiding this comment

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

Hello! This is an excellent improvement! I tested it and it works for me.

In addition to the simplifications @mouse-reeve offered, I think we could drop the git fetch commands too by making sure the checkout@v3 action always uses the main branch. See this commit to see what I mean. (That commit also includes two other minor improvements which you may like to include.)

I wish there was a way for the workflow file itself to live in main and not and l10n_main, but it doesn't seem possible (not easily, anyway). @mouse-reeve Does Crowdin fetch l10n_main and then commits on top of it, or does it just force-push, do you know? (Asking because the later would definitely be a problem.)

Improvements to update_locales.yml
@tararoshan
Copy link
Contributor Author

Thank you! I didn't know the with: refs: option was available for actions instead of manually setting the upstream, that's neat!

Also, I was looking into your second question and found this article about Crowdin's version control integrations. Turns out there's already a (GitHub Crowdin Action)[https://github.com/marketplace/actions/crowdin-action] which we could use, if it's more convenient. I'm still not sure if Crowdin fetches updates before making a PR, though.

@mouse-reeve
Copy link
Member

I'm confused about why this has to live on the l10n branch -- what is the block you're hitting when running this from main? When I use the update locales shortcut, I run it from the main branch, so I would have assumed that the script would operate successfully from main as well.

I suspect the reason not to use the github action is the same as the reason I haven't been merging the PR that crowdin creates automatically -- it brings in all the translation files, rather than the just the ones for locales that are actually in use.

@tararoshan
Copy link
Contributor Author

I think the issue is that to have a GitHub action be triggered by a change to a branch, it needs to live on that branch. When I tried testing with the workflow in the main branch (of my local fork), the workflow was never triggered. As I understand from this SO issue and the GitHub documentation about triggering workflows, Actions need to live in the branch that they're triggered by. In this case, that would be l10n_main.

I agree though that this isn't the best for future maintenance. We could also turn this into a scheduled workflow, but it may run unnecessarily (eg. no locales were updated) or run late (eg. hours after a locale was updated).

Thank you for your patience with this, I know it took me a while to reply.

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.

None yet

3 participants