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

Fetch nightly translations during build #13724

Merged
merged 12 commits into from
Nov 29, 2022
Merged

Conversation

steverep
Copy link
Member

Proposed change

From the discussion in #13381, integrates the nightly translations into the build process:

  • Remove translations from git tracking
  • Add a Gulp task to fetch the translations from the nightly workflow artifacts using the REST API
    • When developer runs any task that builds translations, they will be prompted on the terminal to authenticate on GitHub using the code provided
    • Token will be saved so developer will only have to do this once per local clone (or copy the token around to other locals)
    • Avoids unnecessary API calls and downloads by comparing the creation time of the artifact to a fixed age (currently set to 24 hours but we could probably increase if Lokalise activity is much less)
    • Task runs in about 1-2 sec on my machine when downloading, negligible time otherwise
  • Attempts to integrate this into all workflows
  • Cleans up ESLint rules for build scripts and starts linting them during pre-commit (I got tired of looking at errors and warnings 🍒 )

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

lint-staged.config.js Outdated Show resolved Hide resolved
@bramkragten
Copy link
Member

I'm not sure if we should run this automatically each develop build, maybe we should make a separate script command people can run to download the translations

@steverep
Copy link
Member Author

What are your concerns with having it be part of the build? Maybe I can address them.

@bramkragten
Copy link
Member

It is just an extra step to get your dev end working and extra work each time, that most people will not need during development I think?

@steverep
Copy link
Member Author

Yeah I get that. I was thinking the same, but come to the opposite conclusion. If the translations are removed from tracking, then everyone must execute the task at least once in order for builds to succeed. So if it's not part of the build, then we rely on new developers following more instructions, and maybe possible issues if my translations != your != the CI's?

I also agree Obviously most developers won't need anything outside English, but they should still have an up to date en.json for the backend.

If you didn't already see too, most times the task will exit without doing anything if you already have recent translations.

Co-authored-by: Bram Kragten <mail@bramkragten.nl>
@steverep steverep marked this pull request as draft September 14, 2022 15:53
@steverep
Copy link
Member Author

There's a bug causing inconsistent failures (my favorite kind), so making this a draft until I figure that out.

@bramkragten
Copy link
Member

I also agree Obviously most developers won't need anything outside English, but they should still have an up to date en.json for the backend.

Backend translations are not needed right? I think we only have those to support old instances with cast.home-assistant.io? Or do you mean for typing support?

We should just make the translations/en.json from the src/translations/en.json, so english is always up2date. If devs want to work with local translations, they can fetch them with the script.

@steverep steverep marked this pull request as ready for review September 15, 2022 02:15
@steverep
Copy link
Member Author

Okay I fixed the issue with awaiting the extraction so you can test again.

Backend translations are not needed right? I think we only have those to support old instances with cast.home-assistant.io? Or do you mean for typing support?

Now you've exceeded my HA knowledge. I assumed the core translations were necessary to successfully build the frontend. I wasn't even thinking about typing support yet.

We should just make the translations/en.json from the src/translations/en.json, so english is always up2date. If devs want to work with local translations, they can fetch them with the script.

That would be a big departure from the current build script behavior that uses all languages. I thought the goal was just to do the same thing with up to date files, and get rid of tracking them so there's no more accidental edits.

Anyway you could accomplish what you're saying just by setting the SKIP_FETCH_NIGHTLY_TRANSLATIONS environment variable, and forcing the developer to run gulp fetch-nightly-translations beforehand. I don't see the added value there though. If you remove it from the task series altogether, then all the workflows and maybe some other scripts need to be updated.

@steverep
Copy link
Member Author

Just for kicks I set the variable to skip and built with an empty translations directory. The build succeeds and you can fire up the UI, but there's lots of missing text or text that doesn't match up with the current version, so clearly the existing translations.js script would need modification for what you are saying.

Or I'm completely misunderstanding and we're on totally different pages 🍎 🍏

@bramkragten
Copy link
Member

Just for kicks I set the variable to skip and built with an empty translations directory. The build succeeds and you can fire up the UI, but there's lots of missing text or text that doesn't match up with the current version, so clearly the existing translations.js script would need modification for what you are saying.

Or I'm completely misunderstanding and we're on totally different pages 🍎 🍏

I'll dive in a little deeper this weekend, but if I remember correctly, we always take the src/translations/en.json as base and merge that with the specific lang file, so if that lang file is not there, we should endup with just the base english translations, that is always up to date as it is the source...

@steverep
Copy link
Member Author

Yep, looks like that's what it does, but the build-merged-translations task uses the files in translations/frontend to decide which languages to merge:

gulp.task("build-merged-translations", () =>
  gulp
    .src([inFrontendDir + "/*.json", workDir + "/test.json"], {
      allowEmpty: true,
    })

So if that's empty, there's no output even for English.

@bramkragten
Copy link
Member

OK, so we should change that to use the src/translations/translationMetadata.json instead

@steverep
Copy link
Member Author

Looking at the preceding build-master-translation task, it would be easier just to have that one write build/translations/full/en.json, and just skip English in the next task using it as the master. The two tasks are essentially repeating the same work for English.

Alright, so the default clone will be to build English only, and developers can flip an environment flag in devContainer.json or on the terminal to enable multi-language support. Agreed?

Also need to decide:

  1. Builds for cast, demo, and gallery should always fetch the translations since they merge in the backend translations?
  2. Jobs in the CI workflow should fetch the translations as well? Or maybe that's not needed?

@bramkragten
Copy link
Member

Please keep in mind, that fetching the backend translations now only works because they are in this repo, by removing them from the repo, like you did in this PR, will make that they will also be removed from the nightly artefacts... They are not fetched from Lokalise

@steverep
Copy link
Member Author

Okay I didn't catch that. I can update that script to pull the core translations as well.

@steverep
Copy link
Member Author

Okay here's what I did:

  • New clones will build frontend and supervisor English only (except for those inside GitHub actions)
  • Developer can setup multilingual builds by manually running the "setup & fetch" task to get a token and initial translations once, then future builds will always update them so long as the token is present
  • Builds for demo, cast, and gallery always trigger the fetch since they need the backend translations

Assuming you're good with that, do you think updating with each nightly is okay, or increase 24 to something less often? A good gauge would be how often things change in Lokalise, but I have no idea how to check that.

@steverep
Copy link
Member Author

Did you get a chance to examine the latest changes? No rush for beta here - just wanted to know if I should start adding a couple notes to the developer docs.

@steverep steverep mentioned this pull request Nov 15, 2022
9 tasks
@steverep
Copy link
Member Author

👀 please so I don't have to fix any more conflicts

@bramkragten bramkragten merged commit ee6f97b into dev Nov 29, 2022
@bramkragten bramkragten deleted the use-nightly-translations branch November 29, 2022 18:33
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync lokalise translations and disallow contributions
3 participants