Integrate main i18n tool into build pipeline by LeanidShutau · Pull Request #22254 · elastic/kibana

LeanidShutau · 2018-08-22T15:25:57Z

Closes #20291

elasticmachine · 2018-08-22T17:13:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-24T12:34:19Z

Ugh, old test results disappeared, will retest to get new ones and check the output out.

azasypkin · 2018-08-24T12:34:23Z

retest

azasypkin

Left one nit and one proposal, just to make i18nCheck task similar to other lint-like tasks.

azasypkin · 2018-08-24T13:15:13Z

src/ui/ui_render/bootstrap/template.js.hbs

      err.style['background'] = '#F44336';
      err.style['padding'] = '25px';
-      err.innerText = '{{i18n 'UI-WELCOME_ERROR' '{"defaultMessage": "Kibana did not load properly. Check the server output for more information."}'}}';
+      err.innerText = '{{i18n 'common.ui.welcomeError' '{"defaultMessage": "Kibana did not load properly. Check the server output for more information."}'}}';


nit: please update ids for all existing UI-WELCOME_ messages we have in Kibana (in knb-plugin-helpers tests) and remove src/ui/translations entirely for now.

azasypkin · 2018-08-24T13:23:46Z

tasks/verify_translations.js

-export default function (grunt) {
-  grunt.registerTask('verifyTranslations', async function () {
+module.exports = function (grunt) {
+  grunt.registerTask('verifyTranslations', function () {


issue: hmm, can you please make run:i18nCheck task exactly like run:typeCheck (define in config/run.js, run it right after 'run:typeCheck' in jenkins:unit and test grunt tasks, with !grunt.option('quick') for the latter task)?

And let's rename scripts/extract_default_translations.js to scripts/i18n_check for now and later on we'll figure out whether to keep this name for extraction purpose or not.

yankouskia

just 1 note

yankouskia · 2018-08-24T13:39:56Z

tasks/verify_translations.js

+        grunt.log.writeln(result);
+        resolve();
+      });
+    }).then(done, done);


It is likely more obsolete, I would use:

.then(done) .catch(done)

elasticmachine · 2018-08-24T13:54:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…-i18n-tools

azasypkin · 2018-08-24T14:32:26Z

retest

elasticmachine · 2018-08-24T16:14:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin

Looks good!

I still see one usage of verifyTranslations in tasks/test.js, please remove it and make sure we don't have any other leftovers
While testing I've noticed that we misspelled message here:

kibana/src/dev/i18n/extract_react_messages.js

Line 115 in d787b64

throw new Error(`Default message in <FormattedMessage> is not allowed ("${messageId}").`);

, it should be Empty default message .... is not allowed .... instead of Default message ... is not allowed ...

@LeanidShutau, also let's wait for @spalger feedback before merging this.

Hey @spalger, could you please take a brief look at the PR and let us know if it looks ok to you? We're going to integrate Listr and other bells and whistles in the follow up, just need a bare minimum right now :)

elasticmachine · 2018-08-27T11:33:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

Other than the replacement of the error handling from run() this looks good. Will try running it after that one thing is fixed.

spalger · 2018-08-27T21:19:40Z

src/dev/run_i18n_check.js

+      outputFormat,
+    });
+  } catch (e) {
+    console.error(`${chalk.white.bgRed(' I18N ERROR ')} ${e.message || e}`);


The run() function implements standardized failure handling, please don't override it as removing the stacktrace from unexpected errors can make them really hard to debug. If you have specific errors that don't require a stack trace please throw a FailError instead, like so:

import { createFailError } from '../run'; // ... throw createFailError(`${chalk.white.bgRed(' I18N ERROR ')} some error message`);

run() will only print the error message from FailErrors and will print the stacktrace for other unexpected errors.

elasticmachine · 2018-08-28T13:40:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-28T19:19:37Z

Thanks @LeanidShutau!

* Integrate main i18n tool to build process * Resolve comments * Remove old task * Replace default Error with FailError

LeanidShutau · 2018-08-29T10:44:47Z

6.x/6.5: b705fed

Integrate main i18n tool to build process

0f9eb7a

LeanidShutau added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Project:i18n backport pending v6.5.0 labels Aug 22, 2018

LeanidShutau self-assigned this Aug 22, 2018

LeanidShutau mentioned this pull request Aug 22, 2018

Use extract_default_messages tool for check translations #21639

Closed

LeanidShutau requested review from azasypkin and yankouskia August 22, 2018 18:22

LeanidShutau added the review label Aug 22, 2018

azasypkin requested changes Aug 24, 2018

View reviewed changes

yankouskia approved these changes Aug 24, 2018

View reviewed changes

yankouskia reviewed Aug 24, 2018

View reviewed changes

LeanidShutau added 2 commits August 24, 2018 17:12

Resolve comments

df5cb71

Merge remote-tracking branch 'upstream/master' into feature/integrate…

dcb35f1

…-i18n-tools

azasypkin approved these changes Aug 24, 2018

View reviewed changes

Remove old task

b90f21a

LeanidShutau added feedback_needed and removed review labels Aug 27, 2018

spalger suggested changes Aug 27, 2018

View reviewed changes

LeanidShutau added review and removed feedback_needed labels Aug 28, 2018

Replace default Error with FailError

08f91bc

LeanidShutau force-pushed the feature/integrate-i18n-tools branch from bf7170a to 08f91bc Compare August 28, 2018 11:59

LeanidShutau mentioned this pull request Aug 28, 2018

Add logging to messages validation #22296

Merged

LeanidShutau added the blocker label Aug 28, 2018

LeanidShutau requested a review from spalger August 28, 2018 13:42

spalger approved these changes Aug 28, 2018

View reviewed changes

LeanidShutau removed blocker review labels Aug 29, 2018

LeanidShutau merged commit 1e5d82c into elastic:master Aug 29, 2018

LeanidShutau deleted the feature/integrate-i18n-tools branch August 29, 2018 08:55

LeanidShutau mentioned this pull request Aug 29, 2018

[6.x] Integrate main i18n tool into build pipeline (#22254) #22482

Merged

LeanidShutau added a commit to LeanidShutau/kibana that referenced this pull request Aug 29, 2018

Integrate main i18n tool into build pipeline (elastic#22254)

72d5793

* Integrate main i18n tool to build process * Resolve comments * Remove old task * Replace default Error with FailError

LeanidShutau added a commit that referenced this pull request Aug 29, 2018

Integrate main i18n tool into build pipeline (#22254) (#22482)

b705fed

* Integrate main i18n tool to build process * Resolve comments * Remove old task * Replace default Error with FailError

LeanidShutau added backported and removed backport pending labels Aug 29, 2018

Conversation

LeanidShutau commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 22, 2018

💚 Build Succeeded

Uh oh!

azasypkin commented Aug 24, 2018

Uh oh!

azasypkin commented Aug 24, 2018

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

yankouskia left a comment

Choose a reason for hiding this comment

Uh oh!

yankouskia Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 24, 2018

💔 Build Failed

Uh oh!

azasypkin commented Aug 24, 2018

Uh oh!

elasticmachine commented Aug 24, 2018

💚 Build Succeeded

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 27, 2018

💚 Build Succeeded

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

spalger Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 28, 2018

💚 Build Succeeded

Uh oh!

spalger commented Aug 28, 2018

Uh oh!

LeanidShutau commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LeanidShutau commented Aug 22, 2018 •

edited

Loading

spalger Aug 27, 2018 •

edited

Loading