CONSOLE-2425: Support localization of dynamic plugins #9196

jhadvig · 2021-06-09T14:22:04Z

Implementing additional endpoint for handling locales. The endpoint has following format:
api/locales/<language>/<plugin-locale-file>
eg. api/locales/en/olm.json

When the request hits the endpoint:

it will parse out language and plugin's locale file
then it will compare the plugin name against the list of enabled plugins
if it matches any of the enabled plugins, it will redirect to its service
if it doesn't match any of the enabled plugins, use redirect to loadPath, which is set to static/locales/{{lng}}/{{ns}}.json

Hopefully I understood the desired logic correctly.
@spadgett shall I also update the i18n enhancement ?

PTAL

spadgett

Thanks, @jhadvig. We'll want to update the frontend code as well to use the new endpoint.

spadgett · 2021-06-09T14:27:47Z

pkg/server/server.go

@@ -55,6 +55,7 @@ const (
 	devfileEndpoint                  = "/api/devfile/"
 	devfileSamplesEndpoint           = "/api/devfile/samples/"
 	pluginsEndpoint                  = "/api/plugins/"
+	localesEndpoint                  = "api/locales/"


I wouldn't put this under /api because this request doesn't require auth. Any request under the /api path will have the user's session cookie.

spadgett · 2021-06-09T18:25:02Z

pkg/plugins/handlers.go

+
+// parseLangAndPluginName will parse the language and plugin locale file
+func parseLangAndPluginLocaleFile(urlPath string) (string, string, error) {
+	langAndPluginLocaleFile := strings.SplitN(urlPath, "/", 2)


I wish we were using something like https://github.com/gorilla/mux to handle variables in the path, although that seems like overkill for one endpoint.

I'd suggest simply using query parameters lng and ns to avoid having to parse the path. We might eventually support multi-loading, and the query parameters would fit better with that (although that could be difficult if we need to proxy some requests).

https://github.com/i18next/i18next-http-backend#backend-options

spadgett · 2021-06-09T18:30:11Z

pkg/plugins/handlers.go

+		klog.Errorf(redirectTo)
+	}
+
+	resp, err := p.Client.Get(redirectTo)


Is this actually making an HTTP request for static files? Can we serve static files directly instead using http.ServeFile?

spadgett · 2021-06-25T17:23:08Z

pkg/plugins/handlers.go

+	// In case of the dynamic plugin the localization namespace should contain name of the plugin prefixed with 'plugin:' prefix
+	// No file extension is needed since for the dynamic plugins we will fetch 'default.json' from the plugin pod.
+	// eg. 'plugin:helm-plugin'
+	pluginLocale := query.Get("ns")


This isn't a locale. I'd stick to namespace as the variable name.

spadgett · 2021-06-25T17:26:43Z

pkg/plugins/handlers.go

+	// In case of the dynamic plugin the localization namespace should contain name of the plugin prefixed with 'plugin:' prefix
+	// No file extension is needed since for the dynamic plugins we will fetch 'default.json' from the plugin pod.
+	// eg. 'plugin:helm-plugin'


Can you open a PR to update the dynamic plugin enhancement with this proposal? Then we can get agreement from stakeholders.

cc @christianvogt @vojtechszocs

@spadgett openshift/enhancements#815

spadgett · 2021-06-25T17:28:39Z

pkg/plugins/handlers.go

+	}
+	// in case of dynamic-plugin we need to trim the "plugin:" prefix.
+	pluginName := strings.TrimPrefix(pluginLocale, "plugin:")
+	http.Redirect(w, r, path.Join(r.Host, "api/plugins", pluginName, "locales", lang, "default.json"), http.StatusSeeOther)


We should confirm that i18next-http-backend handles redirects. It might be better just to proxy the content here if it's not too hard.

yeah, proxying makes more sense ... updated

jhadvig · 2021-06-28T11:31:00Z

@spadgett comments addressed.

jhadvig · 2021-07-01T07:19:14Z

@spadgett updated the PR per updates in the enhancement

jhadvig · 2021-07-01T15:15:14Z

/test e2e-gcp-console

jhadvig · 2021-07-01T15:50:23Z

@spadgett I've addted locales to the dynamic-demo-plugin and updated the two Dockerfiles we have for it.

spadgett · 2021-07-01T17:12:52Z

@vojtechszocs @christianvogt FYI

frontend/dynamic-demo-plugin/locales/en/dynamic-demo-plugin.json

pkg/plugins/handlers.go

spadgett · 2021-07-01T17:52:27Z

pkg/plugins/handlers.go

+	// in case of dynamic-plugin we need to trim the "plugin__" prefix.
+	pluginName := strings.TrimPrefix(namespace, "plugin__")


I personally wouldn't trim the prefix here. I'd make the the filename match the actual namespace used like plugin__kubevirt.json since that's how i18next normally works.

Either way, we should make that more clear in the enhancement proposal with a specific example.

On the plus side, this would prevent e.g. plugin__kubevirt-plugin.json file names 😃

Having i18n namespace match the file name sounds pretty straight-forward, sounds OK to me.

@spadgett I trim the plugin__ prefix to get the plugin service endpoint from the map of all the active plugins, where we are mapping the plugin name to its service endpoint. Based on the conversation I dont think ConsolePlugin resource name will contain the prefix, thats why Im trimming it to get the real name.

pkg/plugins/handlers.go

spadgett · 2021-07-01T18:06:45Z

frontend/dynamic-demo-plugin/locales/en/dynamic-demo-plugin.json

@@ -0,0 +1,3 @@
+{
+    "Subject": "Subject"


Is this used anywhere in the demo plugin?

I would only add the JSON files if we're actually using it somewhere in the demo plugin to give a real example.

no its not, in that case I can just put there and empty json

We can have a follow-up PR that illustrates localization usage in dynamic demo plugin.

localizing strings in console-extensions.json

localizing strings in a plain function

localizing strings in a React component

Once this PR is merged and the conventions are settled, I can write that PR.

jhadvig · 2021-07-16T10:03:42Z

@spadgett @vojtechszocs I've rebased the PR and addressed comments. Also remove the i18n changes in the dynamic plugin since we will address those as follow up

PTAL

spadgett · 2021-07-16T13:13:52Z

Test failure looks like it might be related.

Error: Failed: Missing i18n key "YAML" in namespace "public" and language "en."

spadgett · 2021-07-16T13:18:58Z

browser.log has a large amount of missing key errors:

https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_console/9196/pull-ci-openshift-console-master-e2e-gcp-console/1415975376487714816/artifacts/e2e-gcp-console/test/artifacts/gui_test_screenshots/browser.log

spadgett · 2021-07-16T13:20:25Z

Looking deeper, I see the files are failing to load.

[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=ceph-storage-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=console-shared - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=devconsole - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=container-security - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=console-app - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=git-service - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=gitops-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=helm-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=insights-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=kubevirt-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=knative-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=lso-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=metal3-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=notification-drawer - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=olm - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=public - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=pipelines-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=rhoas-plugin - Failed to load resource: the server responded with a status of 404 (Not Found)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/locales/resource.json?lng=en&ns=topology - Failed to load resource: the server responded with a status of 404 (Not Found)

jhadvig · 2021-07-16T13:36:21Z

Hmm wondering if thats related to the PR since there are also other errors

[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/api/console/knative-event-sources - Failed to load resource: the server responded with a status of 401 (Unauthorized)
[WARNING] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/static/main-chunk-ab5505283ddc025dfa57.min.js 0:380641 "Error fetching CRDs for dynamic event sources" Error: Unauthorized
    at https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/static/main-chunk-ab5505283ddc025dfa57.min.js:1:123823
    at Generator.next (<anonymous>)
    at https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/static/main-chunk-ab5505283ddc025dfa57.min.js:1:123209
    at new Promise (<anonymous>)
    at s (https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/static/main-chunk-ab5505283ddc025dfa57.min.js:1:122957)
    at u (https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/static/main-chunk-ab5505283ddc025dfa57.min.js:1:123309)
    at https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/static/main-chunk-ab5505283ddc025dfa57.min.js:1:124936
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/api/console/knative-channels - Failed to load resource: the server responded with a status of 401 (Unauthorized)
[SEVERE] https://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/static/vendors~main-chunk-d7eaff6a7eb98f5b246e.min.js 87 WebSocket connection to 'wss://console-openshift-console.apps.ci-op-cnxvm8dw-75d12.**********************/api/graphql' failed: HTTP Authentication failed; no valid credentials available

spadgett · 2021-07-16T13:49:19Z

@jhadvig Those are expected on initial console load since you're not logged in yet.

jhadvig · 2021-07-22T11:41:31Z

/test e2e-gcp-console

spadgett · 2021-07-22T17:00:53Z

pkg/plugins/handlers.go

 	}
 }

-func (p *PluginsHandler) HandlePlugins(w http.ResponseWriter, r *http.Request) {
+func (p *PluginsHandler) HandleLocales(w http.ResponseWriter, r *http.Request) {


These are resources, not locales.

Suggested change

func (p *PluginsHandler) HandleLocales(w http.ResponseWriter, r *http.Request) {

func (p *PluginsHandler) HandleI18nResources(w http.ResponseWriter, r *http.Request) {

spadgett · 2021-07-22T17:04:24Z

pkg/plugins/handlers.go

+	// In case of console app or static plugins, the localization namespace should contain name of the json file that contains the localized strings,
+	// since various console packages have different names for the localization file.
+	// eg. namespace query for the local-storage-operator-plugin locales - '&ns=lso-plugin'
+	// In case of the dynamic plugin the localization namespace should contain name of the plugin prefixed with 'plugin__' prefix.
+	// No file extension is needed since for the dynamic plugins we will fetch file from the plugin pod, based on the namespace.
+	// eg. 'plugin__helm' will fetch `/locales/{lang}/plugin__helm.json`


I think this is the important bit, so I'd trim the comment to just this:

Suggested change

// In case of console app or static plugins, the localization namespace should contain name of the json file that contains the localized strings,

// since various console packages have different names for the localization file.

// eg. namespace query for the local-storage-operator-plugin locales - '&ns=lso-plugin'

// In case of the dynamic plugin the localization namespace should contain name of the plugin prefixed with 'plugin__' prefix.

// No file extension is needed since for the dynamic plugins we will fetch file from the plugin pod, based on the namespace.

// eg. 'plugin__helm' will fetch `/locales/{lang}/plugin__helm.json`

// In case of the dynamic plugins, the namespace should contain name of the plugin prefixed with 'plugin__' prefix.

// eg. 'plugin__helm' will fetch `locales/{lang}/plugin__helm.json` from the plugin service

pkg/plugins/handlers.go

spadgett

/lgtm
/hold for approvals
/assign @yapei @ahardin-rh @sferich888

openshift-ci · 2021-07-22T17:48:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahardin-rh · 2021-07-22T19:51:18Z

/label docs-approved

sferich888 · 2021-07-22T20:11:10Z

/label px-approved
/unassign @sferich888

yapei · 2021-07-23T11:48:09Z

tested the changes and it is working well
/label qe-approved

jhadvig · 2021-07-23T11:53:48Z

/hold cancel

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2021

openshift-ci bot requested review from alecmerdler and spadgett June 9, 2021 14:22

openshift-ci bot added the component/backend Related to backend label Jun 9, 2021

spadgett reviewed Jun 9, 2021

View reviewed changes

openshift-ci bot added the component/core Related to console core functionality label Jun 16, 2021

jhadvig force-pushed the CONSOLE-2425 branch from 67397c9 to c497a7c Compare June 23, 2021 10:58

spadgett reviewed Jun 25, 2021

View reviewed changes

jhadvig mentioned this pull request Jun 28, 2021

CONSOLE-2425: Support localization of dynamic plugins openshift/enhancements#815

Merged

jhadvig force-pushed the CONSOLE-2425 branch from c497a7c to 69a4756 Compare June 28, 2021 10:22

jhadvig changed the title ~~[WIP] CONSOLE-2425: Support localization of dynamic plugins~~ CONSOLE-2425: Support localization of dynamic plugins Jun 28, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2021

jhadvig force-pushed the CONSOLE-2425 branch 3 times, most recently from 60fbab5 to b38a92b Compare July 1, 2021 15:45

vojtechszocs suggested changes Jul 1, 2021

View reviewed changes

frontend/dynamic-demo-plugin/locales/en/dynamic-demo-plugin.json Outdated Show resolved Hide resolved

pkg/plugins/handlers.go Outdated Show resolved Hide resolved

pkg/plugins/handlers.go Outdated Show resolved Hide resolved

pkg/plugins/handlers.go Outdated Show resolved Hide resolved

spadgett reviewed Jul 1, 2021

View reviewed changes

jhadvig force-pushed the CONSOLE-2425 branch from b38a92b to 5aa826f Compare July 16, 2021 09:58

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2021

jhadvig force-pushed the CONSOLE-2425 branch from 5aa826f to 4409ebf Compare July 16, 2021 10:01

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2021

jhadvig force-pushed the CONSOLE-2425 branch from 4409ebf to 8c40162 Compare July 16, 2021 10:03

shdn-ibm mentioned this pull request Jul 19, 2021

<feature> add flashsystem dashboardpage and storage efficiency card IBM/ibm-storage-odf-console#10

Merged

jhadvig force-pushed the CONSOLE-2425 branch from 8c40162 to e826da3 Compare July 22, 2021 09:57

spadgett reviewed Jul 22, 2021

View reviewed changes

CONSOLE-2425: Support localization of dynamic plugins

dd2a9e8

jhadvig force-pushed the CONSOLE-2425 branch from e826da3 to dd2a9e8 Compare July 22, 2021 17:38

spadgett approved these changes Jul 22, 2021

View reviewed changes

openshift-ci bot assigned ahardin-rh and sferich888 Jul 22, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

openshift-ci bot assigned yapei and spadgett Jul 22, 2021

spadgett added this to the v4.9 milestone Jul 22, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2021

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jul 22, 2021

openshift-ci bot unassigned sferich888 Jul 22, 2021

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jul 22, 2021

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 23, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2021

openshift-merge-robot merged commit 0871fcf into openshift:master Jul 23, 2021

shdn-ibm mentioned this pull request Aug 4, 2021

feature: init i18n IBM/ibm-storage-odf-console#24

Merged

		// in case of dynamic-plugin we need to trim the "plugin__" prefix.
		pluginName := strings.TrimPrefix(namespace, "plugin__")

	func (p PluginsHandler) HandleLocales(w http.ResponseWriter, r http.Request) {
	func (p PluginsHandler) HandleI18nResources(w http.ResponseWriter, r http.Request) {

CONSOLE-2425: Support localization of dynamic plugins #9196

CONSOLE-2425: Support localization of dynamic plugins #9196

Conversation

jhadvig commented Jun 9, 2021

spadgett 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

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

jhadvig commented Jun 28, 2021

jhadvig commented Jul 1, 2021

jhadvig commented Jul 1, 2021

jhadvig commented Jul 1, 2021

spadgett commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig Jul 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs Jul 2, 2021 • edited

Choose a reason for hiding this comment

jhadvig commented Jul 16, 2021

spadgett commented Jul 16, 2021

spadgett commented Jul 16, 2021

spadgett commented Jul 16, 2021

jhadvig commented Jul 16, 2021

spadgett commented Jul 16, 2021

jhadvig commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 22, 2021

ahardin-rh commented Jul 22, 2021

sferich888 commented Jul 22, 2021

yapei commented Jul 23, 2021

jhadvig commented Jul 23, 2021

jhadvig Jul 16, 2021 •

edited

vojtechszocs Jul 2, 2021 •

edited