migrate i18n mixin to KP #81799

pgayvallet · 2020-10-27T15:50:43Z

Summary

Fix #50647

Migrate i18n loading to core.

Create a new i18n service
Move the Localization usage collector to the kibana_usage_collection plugin

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

…-i18n

…config

pgayvallet · 2020-11-02T09:56:18Z

src/core/server/server.ts

+// do not try to shorten the import to `./status`, it will break server test mocking
 import { StatusService } from './status/status_service';


TIL, after loosing 1h on that...

pgayvallet · 2020-11-02T10:01:09Z

src/plugins/telemetry/schema/legacy_plugins.json

-          "type": "long"
-        }
-      }
-    }
-  }
+  "properties": {}
 }


@elastic/kibana-telemetry This PR removes the last legacy usage collector. I guess this file and the generation / usage around it could now be safely deleted I guess. Not sure if it is trivial enough to be done in current PR though. Would it be fine to let you handle that in a follow-up?

We'd have to update the .telemetryrc.json file and it will automatically be removed. We can push that change in another PR

pgayvallet · 2020-11-02T10:03:05Z

src/core/server/i18n/i18n_service.ts

+  getLocale(): string;
+
+  /**
+   * Return the list of translation files currently in use.
+   */
+  getTranslationFiles(): string[];


I need that to be accessible for the localization usage collector.

The getLocale is not really necessary as the collector could use i18n.getLocale() instead, but from an architectural point of view, I think it is better to have core be the entry point for that (and the contract is needed anyway for getTranslationFiles so it did not cost much to also add an additional API to it)

According to our classification, @kbn/i18n should be implemented as a service because it's a stateful package. IIRC we were discussing to deprecate direct access to @kbn/i18n in favour of i18n Platform service, so 👍

pgayvallet · 2020-11-02T10:04:39Z

src/core/server/i18n/get_kibana_translation_files.ts

    getTranslationPaths({
      cwd: fromRoot('.'),
-      glob: `*/${I18N_RC}`,
+      nested: true,
    }),


AFAIK, this is only used to get the x-pack/.i18nrc.json file. However to avoid any risk / breaking changes (User could have custom i18nrc files in another non-plugin folder eventually), I kept it as it was done in legacy.

@pgayvallet only in x-pack. I believe it is safe to change it as long as x-pack and kibana's root dir are discoverable.

does it mean that it traverses all the node_modules and other unnecessary folders?

No. */something globbing is only scanning a single level (as opposed to **/something)

ok, I didn't check implementation. but nested: true traversing on a single level in-depth is confusing 😅

Yea, you're right. I kinda wanted to rename it to includeSubFolders, but is was way longer, and not really more explicit that we are only scanning one level down. No issue renaming if you have a better idea.

We will need something similar to maxScanDepth in discovery to support migration to Domain-based folder structure. But I'm okay to keep it as is for now.

pgayvallet · 2020-11-02T10:04:59Z

src/core/server/i18n/get_kibana_translation_files.ts

    getTranslationPaths({
      cwd: fromRoot('../kibana-extra'),
-      glob: `*/${I18N_RC}`,
+      nested: true,
    }),


Are we really still using kibana-extra? Kept that, but not sure if it is really necessary.

As long as we still support running plugins from that path then we should keep it. If not there is no need to.

We still search for a plugin in kibana-extra

kibana/packages/kbn-config/src/plugins/plugin_search_paths.ts

Line 34 in 2e37bd0

resolve(rootDir, '..', 'kibana-extra'),

and kbn-pm supports it as well https://github.com/elastic/kibana/blob/0068c87d6fe7713351acbf42080faa09ff73ef15/packages/kbn-pm/README.md
But I haven't checked whether it works

pgayvallet · 2020-11-02T10:14:49Z

src/core/server/i18n/get_kibana_translation_files.ts

-    ...(config.get('plugins.paths') as string[]).map((cwd) =>
-      getTranslationPaths({ cwd, glob: I18N_RC })
-    ),
-    ...(config.get('plugins.scanDirs') as string[]).map((cwd) =>
-      getTranslationPaths({ cwd, glob: `*/${I18N_RC}` })
-    ),
+    ...pluginPaths.map((pluginPath) => getTranslationPaths({ cwd: pluginPath, nested: false })),


There is a slight change of behavior here:

Legacy was just reading the plugin config and scanning the scanDirs (1 lvl deep) and paths (root lvl) for i18n rc files.

I chose to change that by retrieving the list of enabled plugins from the plugin service, and check if there is a i18n file at the root level of each plugin instead. This is basically the same, except that we are not loading translation files for disabled plugins (note that this only concerns 3rd parties plugin, as kibana translations are all defined from a single x-pack/.i18nrc.json file anyway). Overall, I think it should not really impact 3rd parties, and not loading translations for disabled plugins does make sense imho. (This is also way better at the code level in term of responsibilities per service. I was really awkward to load the plugins config from the i18n service)

elasticmachine · 2020-11-02T10:15:41Z

Pinging @elastic/kibana-platform (Team:Platform)

afharo

Telemetry changes LGTM!

Bamieh

LGTM! Thanks

Bamieh · 2020-11-02T11:36:13Z

src/core/server/i18n/get_kibana_translation_files.ts

    getTranslationPaths({
      cwd: fromRoot('.'),
-      glob: `*/${I18N_RC}`,
+      nested: true,
    }),


@pgayvallet only in x-pack. I believe it is safe to change it as long as x-pack and kibana's root dir are discoverable.

Bamieh · 2020-11-02T11:37:43Z

src/core/server/i18n/get_kibana_translation_files.ts

    getTranslationPaths({
      cwd: fromRoot('../kibana-extra'),
-      glob: `*/${I18N_RC}`,
+      nested: true,
    }),


As long as we still support running plugins from that path then we should keep it. If not there is no need to.

Bamieh · 2020-11-02T11:40:41Z

src/plugins/telemetry/schema/legacy_plugins.json

-          "type": "long"
-        }
-      }
-    }
-  }
+  "properties": {}
 }


We'd have to update the .telemetryrc.json file and it will automatically be removed. We can push that change in another PR

…-i18n

docs/development/core/server/kibana-plugin-core-server.i18nservicesetup.gettranslationfiles.md

mshustov

I'd like to adjust #81799 (comment) if necessary. Otherwise looks good

…-i18n

kibanamachine · 2020-11-10T14:05:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3eb3a91

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	42766	42768	+2
`oss`	22455	22457	+2

History

💚 Build #86772 succeeded 6b6f58d
💚 Build #86662 succeeded 9c8f087
💚 Build #86049 succeeded 1914c2f
💚 Build #85162 succeeded 7e7d247
💚 Build #85076 succeeded fd1419c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* migrate i18n mixin to KP * directly load the config from i18n service * add base tests * update telemetry schema * update legacy telemetry schema * fix server tests * use paths from config service instead of manually loading the plugin config * add tests for get_translation_paths * add tests for i18n service * add plugin service test * update generated doc * improve tsdoc

…kibana into bootstrap-node-details-overlay * 'bootstrap-node-details-overlay' of github.com:phillipb/kibana: (49 commits) [Security Solution] Fix DNS Network table query (elastic#82778) [Workplace Search] Consolidate groups routes (elastic#83015) Adds cloud links to user menu (elastic#82803) [Security Solution][Detections] - follow up cleanup on auto refresh rules (elastic#83023) [App Search] Added the log retention panel to the Settings page (elastic#82982) [Maps] show icon when layer is filtered by time and allow layers to ignore global time range (elastic#83006) [DOCS] Consolidates drilldown pages (elastic#82081) [Maps] add on-prem EMS config (elastic#82525) migrate i18n mixin to KP (elastic#81799) [bundle optimization] fix imports of react-use lib (elastic#82847) [Discover] Add metric on adding filter (elastic#82961) [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (elastic#82829) skip flaky suite (elastic#82804) Fix SO query for searching across spaces (elastic#83025) renaming built-in alerts to Stack Alerts (elastic#82873) [TSVB] Disable using top_hits in pipeline aggregations (elastic#82278) [Visualizations] Remove kui usage (elastic#82810) [Visualizations] Make the icon buttons labels more descriptive (elastic#82585) [Lens] Do not reset formatting when switching between custom ranges and auto histogram (elastic#82694) Fix ilm navigation (elastic#81664) ...

…na into alerts/stack-alerts-public * 'alerts/stack-alerts-public' of github.com:gmmorris/kibana: [Security Solution] Fix DNS Network table query (elastic#82778) [Workplace Search] Consolidate groups routes (elastic#83015) Adds cloud links to user menu (elastic#82803) [Security Solution][Detections] - follow up cleanup on auto refresh rules (elastic#83023) [App Search] Added the log retention panel to the Settings page (elastic#82982) [Maps] show icon when layer is filtered by time and allow layers to ignore global time range (elastic#83006) [DOCS] Consolidates drilldown pages (elastic#82081) [Maps] add on-prem EMS config (elastic#82525) migrate i18n mixin to KP (elastic#81799) [bundle optimization] fix imports of react-use lib (elastic#82847) [Discover] Add metric on adding filter (elastic#82961) [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (elastic#82829) skip flaky suite (elastic#82804) Fix SO query for searching across spaces (elastic#83025) renaming built-in alerts to Stack Alerts (elastic#82873) [TSVB] Disable using top_hits in pipeline aggregations (elastic#82278) [Visualizations] Remove kui usage (elastic#82810) [Visualizations] Make the icon buttons labels more descriptive (elastic#82585) [Lens] Do not reset formatting when switching between custom ranges and auto histogram (elastic#82694) :

migrate i18n mixin to KP

1250c9a

pgayvallet added Feature:Legacy Removal Issues related to removing legacy Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11 chore labels Oct 27, 2020

timroes added v7.11.0 and removed v7.11 labels Oct 29, 2020

pgayvallet added 12 commits October 30, 2020 17:50

Merge remote-tracking branch 'upstream/master' into kbn-50647-migrate…

39559b6

…-i18n

directly load the config from i18n service

19f2155

add base tests

f022822

update telemetry schema

ea70bbf

update legacy telemetry schema

305d323

Merge remote-tracking branch 'upstream/master' into kbn-50647-migrate…

e28215e

…-i18n

fix server tests

dbdea7a

use paths from config service instead of manually loading the plugin …

19cf5fe

…config

add tests for get_translation_paths

51b1d04

add tests for i18n service

9812a93

add plugin service test

bbc9bbd

update generated doc

fd1419c

pgayvallet commented Nov 2, 2020

View reviewed changes

pgayvallet marked this pull request as ready for review November 2, 2020 10:15

pgayvallet requested review from a team as code owners November 2, 2020 10:15

afharo approved these changes Nov 2, 2020

View reviewed changes

Bamieh approved these changes Nov 2, 2020

View reviewed changes

pgayvallet added 3 commits November 2, 2020 14:51

Merge remote-tracking branch 'upstream/master' into kbn-50647-migrate…

7e7d247

…-i18n

Merge remote-tracking branch 'upstream/master' into kbn-50647-migrate…

1914c2f

…-i18n

Merge remote-tracking branch 'upstream/master' into kbn-50647-migrate…

9c8f087

…-i18n

mshustov reviewed Nov 9, 2020

View reviewed changes

docs/development/core/server/kibana-plugin-core-server.i18nservicesetup.gettranslationfiles.md Show resolved Hide resolved

improve tsdoc

6b6f58d

mshustov approved these changes Nov 10, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-50647-migrate…

3eb3a91

…-i18n

pgayvallet merged commit f49ee06 into elastic:master Nov 10, 2020

pgayvallet mentioned this pull request Nov 10, 2020

[7.x] migrate i18n mixin to KP (#81799) #83055

Merged

pgayvallet mentioned this pull request Nov 12, 2020

Migrate /translations route to core #83280

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate i18n mixin to KP #81799

migrate i18n mixin to KP #81799

pgayvallet commented Oct 27, 2020 •

edited

Loading

pgayvallet Nov 2, 2020

pgayvallet Nov 2, 2020

Bamieh Nov 2, 2020

pgayvallet Nov 2, 2020

mshustov Nov 9, 2020

pgayvallet Nov 2, 2020

Bamieh Nov 2, 2020

mshustov Nov 9, 2020

pgayvallet Nov 10, 2020

mshustov Nov 10, 2020

pgayvallet Nov 10, 2020

mshustov Nov 10, 2020

pgayvallet Nov 2, 2020 •

edited

Loading

Bamieh Nov 2, 2020

mshustov Nov 9, 2020

pgayvallet Nov 2, 2020

elasticmachine commented Nov 2, 2020

afharo left a comment

Bamieh left a comment

Bamieh Nov 2, 2020

Bamieh Nov 2, 2020

Bamieh Nov 2, 2020

mshustov left a comment

kibanamachine commented Nov 10, 2020

		// do not try to shorten the import to `./status`, it will break server test mocking
		import { StatusService } from './status/status_service';

migrate i18n mixin to KP #81799

migrate i18n mixin to KP #81799

Conversation

pgayvallet commented Oct 27, 2020 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 2, 2020

afharo left a comment

Choose a reason for hiding this comment

Bamieh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 10, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

pgayvallet commented Oct 27, 2020 •

edited

Loading

pgayvallet Nov 2, 2020 •

edited

Loading