github-action icon indicating copy to clipboard operation
github-action copied to clipboard

Not setting configured labels on PRs

Open torokati44 opened this issue 1 year ago • 14 comments

These lines should probably use a /pulls/ URL, rather than /issues/...

https://github.com/crowdin/github-action/blob/c953b17499daa6be3e5afbf7a63616fb02d8b18d/entrypoint.sh#L161 https://github.com/crowdin/github-action/blob/c953b17499daa6be3e5afbf7a63616fb02d8b18d/entrypoint.sh#L178

See: https://github.com/ruffle-rs/ruffle/actions/runs/11021911424/job/30610062421#step:4:11 https://github.com/ruffle-rs/ruffle/actions/runs/11021911424/job/30610062421#step:4:576

Docs: https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#update-a-pull-request

torokati44 avatar Sep 25 '24 17:09 torokati44

@torokati44 thanks for reporting this! It's strange, I wonder how it worked before.

I just made a change in the fix-pulls branch (812dd3c)

Could you please try it by referencing the branch instead of the version?

uses: crowdin/github-action@fix-pulls

andrii-bodnar avatar Sep 26 '24 06:09 andrii-bodnar

Maybe it never worked...? 😐🫥 Thank you for the quick fix! I think actually testing it on our end would have a bit too much overhead, especially since we're in the process of moving away from this action anyway... 😶 ( https://github.com/ruffle-rs/ruffle/pull/18028 )

torokati44 avatar Sep 26 '24 06:09 torokati44

since we're in the process of moving away from this action anyway

Could you please provide more details on this?

If you need to make some changes to the translation files, you are probably looking for post-export processing on the Crowdin side instead of doing it after the translations download.

andrii-bodnar avatar Sep 26 '24 06:09 andrii-bodnar

Would you mind summarizing, @kjarosh?

torokati44 avatar Sep 26 '24 07:09 torokati44

We want to localize the following files:

https://github.com/ruffle-rs/ruffle/blob/b59ee2f91bf0c98c3fb6e200f8878c925bc5c84e/desktop/packages/linux/rs.ruffle.Ruffle.metainfo.xml

https://github.com/ruffle-rs/ruffle/blob/b59ee2f91bf0c98c3fb6e200f8878c925bc5c84e/desktop/packages/linux/rs.ruffle.Ruffle.desktop

Crowdin does not support them, so the plan is to:

  1. convert them to .pot and feed it to Crowdin on upload,
  2. download .pos and merge them into the final file on download.

Conversion tools include xgettext, msgfmt, and itstool.

kjarosh avatar Sep 26 '24 07:09 kjarosh

@kjarosh thanks for the details!

I just did a quick test and it seems that the provided XML file imports well without any modifications or convertations. You can additionally specify XPath translatable_elements to better configure what is translatable (Configuration File).

The *.desktop file looks very similar to ini. If you change the extension, it imports very well as an ini file format.

So, you can add an extra step to your workflow where you rename all the *.desktop files to *.ini, then do the sync via Crowdin action (with PR creation disabled). After that, rename ini files back and create a pull request. I hope this should work.

andrii-bodnar avatar Sep 26 '24 08:09 andrii-bodnar

I believe the problem is that both the metainfo and desktop files have specific semantics of how they are supposed to store the localized texts (all in a single file) - it's not as simple as with other files, where it's just multiple instances of the same basic file (one per language). Hence these file formats (semantically speaking, not just about how they "look") would need tailor-made support from Crowdin. But correct me if I'm wrong.

torokati44 avatar Sep 26 '24 08:09 torokati44

Do you mean that these files are multilingual?

andrii-bodnar avatar Sep 26 '24 08:09 andrii-bodnar

@torokati44 is right, Crowdin indeed can translate those files, but we'll get an instance for each locale, which is not how metainfo and desktop files support localization.

The desktop file uses [<locale>] suffix for keys (for translatable elements), and metainfo uses inline xml:lang attributes (for translatable elements).

If we have to merge those files anyway, it's just easier to convert them to .pot/.po using standard methods.


Do you mean that these files are multilingual?

That's right, you can consult the documentation here:

  • metainfo: https://freedesktop.org/software/appstream/docs/chap-Metadata.html
  • desktop: https://specifications.freedesktop.org/desktop-entry-spec/latest/localized-keys.html

kjarosh avatar Sep 26 '24 08:09 kjarosh

Thanks for the details! This sounds very reasonable to me now.

Earlier we had a similar discussion to allow running some pre-commit scripts, but there is no obvious solution on how to do it right (https://github.com/crowdin/github-action/discussions/181)

You can still synchronize the source files and translations with this action, but disable the automatic commit and PR creation.

andrii-bodnar avatar Sep 26 '24 08:09 andrii-bodnar

You can still synchronize the source files and translations with this action, but disable the automatic commit and PR creation.

Yes, that's the plan. I think @torokati44 meant that we're moving away from this action regarding committing & creating PRs.

kjarosh avatar Sep 26 '24 09:09 kjarosh

Indeed, pardon my imprecision.

torokati44 avatar Sep 26 '24 09:09 torokati44

Actually, it's entirely possible that the /issues/ URL does get redirected to the correct one, based on the number (if it actually belongs to a PR); and it doesn't work for us because the GITHUB_TOKEN we supply has no permission to manage labels... But in that case an error message about this would have been nice...

torokati44 avatar Sep 27 '24 09:09 torokati44