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

Better naming for Crowdin workflows #407

Merged
merged 3 commits into from
May 28, 2024
Merged

Conversation

lkiesow
Copy link
Member

@lkiesow lkiesow commented May 21, 2024

Let all GitHub Actions workflow definitions for Crowdin…

  • have a filename starting with crowdin-…
  • Have a name starting with Crowdin »

@ziegenberg ziegenberg added the type:code-enhancement Internal improvements to the codebase label May 21, 2024
Copy link
Member

@ziegenberg ziegenberg left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

@ziegenberg ziegenberg left a comment

Choose a reason for hiding this comment

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

The line

crowdin download --config .crowdin.yaml -b admin-ui-picard

probably also needs changing, and the branch name updated to main.

That's what the last GHA error suggest: https://github.com/opencast/opencast-admin-interface/actions/runs/9153874134/job/25163529960

Copy link
Contributor

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Let all GitHub Actions workflow definitions for Crowdin…

- have a filename starting with `crowdin-…`
- Have a name starting with `Crowdin » `
@lkiesow
Copy link
Member Author

lkiesow commented May 23, 2024

Fixed conflicts and branch name

Comment on lines 35 to 40
- name: update language list
working-directory: /src/i18n/org/opencastproject/adminui/languages/
run: |
echo -n '[ "' > locales.json
echo -n ??-??.json | sed 's/ */", "/g' >> locales.json
echo '" ]' >> locales.json
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this step? Isn't the list of languages configured here src/i18n/languages.ts?

- name: download translations
env:
CROWDIN_TOKEN: ${{ secrets.CROWDIN_TOKEN }}
run: |
crowdin download --config .crowdin.yaml -b admin-ui-picard
crowdin download --config .crowdin.yaml -b main

- name: add new translations
run: |
git add /src/i18n/org/opencastproject/adminui/languages/
Copy link
Member

@ziegenberg ziegenberg May 24, 2024

Choose a reason for hiding this comment

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

Should we have those absolute file path here?

Copy link
Member

Choose a reason for hiding this comment

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

The upstream project uses git commit -a here. Unsure which is less safe :D

Copy link
Member

Choose a reason for hiding this comment

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

I'm just not sure if an absolute file path would work or if this should be relative to the current working directory...

Copy link
Member

Choose a reason for hiding this comment

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

@lkiesow, could you quickly look at this? Then, we could merge it right after.

@ziegenberg
Copy link
Member

Looking good now!

@ziegenberg ziegenberg merged commit e89356c into opencast:main May 28, 2024
2 checks passed
@ziegenberg ziegenberg added type:infrastructure Build process, deployment, workflows and removed type:code-enhancement Internal improvements to the codebase labels May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:infrastructure Build process, deployment, workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants