Skip to content

Free contributors from i18n data sync responsibility #1515

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

Merged
merged 2 commits into from
Oct 19, 2021

Conversation

silvanocerza
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

This PR changes how i18n files are handled for this project.

  • What is the current behavior?

Each a PR is updated the task i18n:check command is launched to verify the i18n/data/en.po file is updated if necessary.
Contributors might forget to update this file with task i18n:update, also it often creates conflicts with the base branch if a new PR is merged.

  • What is the new behavior?

Remove i18n:check from PRs.

i18n-nightly-push workflow now runs i18n:update to update i18n/data/en.po and then i18n:push to push the en.po to Transifex, this removes the burden from contributors to update manually en.po on each PR.

release-go-task.yml workflow now runs i18n:check to verify i18n/data/en.po is up to date, release fails if it's not. We will also edit our release process so the release manager will have to verify the file is updated before starting the process.

check-i18n-task.yml removed, the check is included in the release process now.

Nope.

  • Other information:

None.


See how to contribute

@silvanocerza silvanocerza added type: enhancement Proposed improvement topic: infrastructure Related to project infrastructure labels Oct 18, 2021
@silvanocerza silvanocerza requested a review from a team October 18, 2021 13:22
@silvanocerza silvanocerza self-assigned this Oct 18, 2021
@silvanocerza silvanocerza force-pushed the scerza/i18n-handling-rework branch from 69aa2b6 to a78b6a4 Compare October 18, 2021 14:11
Comment on lines 34 to 38
- name: Check i18n source file is up to date
run:
- task i18n:update
- task i18n:check

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to avoid this approach if possible. The reason is that this workflow is triggered by the creation of tags and it will be better to check the i18n data is synced before creating a tag, as is already done for all the other CI checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I could undo deletion of check-i18n-task.yml and make it run on creation of the [0-9]+.[0-9]+.x branch. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would be the ideal. But it also must run on every push to the release branch. After that, the standard release preparation process where we check the status of all CI workflows with a filter set for the release branch branch will apply just as well to the i18n sync check.

I haven't actually investigated the topic, so perhaps there is some tricky thing that will prevent that approach from working, but I think it's at least worth a try for the sake of consistency in the way the CI system is used for quality assurance in releases.

Copy link
Member

Choose a reason for hiding this comment

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

The current i18n CI checks that the strings in en.po matches the actual contents of the golang source code.

BTW an updated en.po file is not a requirement to correctly run the CLI: we could even ship an empty en.po since the CLI will use the base string (the one wrapped in the Tr(...) function) when a translation is not present, and since the English language is the base language, the base string and the translation are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would be the ideal. But it also must run on every push to the release branch.

Done.

BTW an updated en.po file is not a requirement to correctly run the CLI: we could even ship an empty en.po since the CLI will use the base string (the one wrapped in the Tr(...) function) when a translation is not present, and since the English language is the base language, the base string and the translation are the same.

It's not a requirement for the CLI but it should be for us, it's the only way to know we actually pulled the latest translations from Transifex for now.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a requirement for the CLI but it should be for us, it's the only way to know we actually pulled the latest translations from Transifex for now.

You may never be 100% sure, translators may add or change a translation string on Transifex at any moment... unless you pull the latest version from Transifex just before building the CLI, the resource committed on git may get outdated at any moment.

@silvanocerza silvanocerza force-pushed the scerza/i18n-handling-rework branch from a78b6a4 to dc2807b Compare October 19, 2021 08:30
Co-authored-by: per1234 <accounts@perglass.com>
@silvanocerza silvanocerza merged commit 7b468c0 into master Oct 19, 2021
@silvanocerza silvanocerza deleted the scerza/i18n-handling-rework branch October 19, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Related to project infrastructure type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants