First round of i18n in acs-engine based on gettext. Subsequent change… #627

JiangtianLi · 2017-05-15T16:41:57Z

…s will replace and extract resource strings in acs-engine and make acsengine/api package's i18n used by other go program.

This PR is to get preview and feedback. Subsequent change will do the real format in the code.

This change is

JiangtianLi · 2017-05-15T17:00:59Z

Note, please don't merge as there will be more commit.

…s will replace and extract resource strings in acs-engine and make acsengine/api package's i18n used by other go program.

…anslation resource files.

amanohar · 2017-05-18T15:29:21Z

Dockerfile

@@ -29,6 +29,10 @@ RUN git clone https://github.com/akesterson/cmdarg.git /tmp/cmdarg \
 RUN git clone https://github.com/akesterson/shunit.git /tmp/shunit \
    && cd /tmp/shunit && make install && rm -rf /tmp/shunit

+# Go tool for internationalization and localization
+RUN go get github.com/JiangtianLi/gettext/... \


Should ACS Engine be getting stuff from private repos?

I tried not to but I spent some time and didn't find a tool. That package is basically used for extract strings out of source file for translation so it will only run every time we update translation strings. I forked https://github.com/gosexy/gettext to address the parsing we need. I can try to merge into upstream.

I submitted a PR to that tool: gosexy/gettext#16

Just echoing others and calling out we can't include this change as is, the repo has to be publically accessible.

Looks like https://github.com/JiangtianLi/gettext is public now (might have been from the beginning), so we're OK to use the fork, as long as we've registered it at https://ossmsft.visualstudio.com/_oss.

amanohar · 2017-05-18T15:30:44Z

cmd/generate.go

-	classicMode       bool
-	noPrettyPrint     bool
-	parametersOnly    bool
+	translationsDirectory string


Caller should not need to pass translation dir to any command ever. This decreases usability of ACS Engine. This should happen automatically. In fact do we even care of localize ACS Engine commands? Localization is primarily needed for the RP.

You are right, localization of ACS Engine command has lower priority. The primary translation happens in acsengine and api package, for the purpose of RP. I added here for the e2e test and it is nice to have for ACS Engine. For the translationsDirectory parameter, it will not be needed and default to the translation folder if acs-engine is run at the package root directory (I think that's the most used scenario?) It has the issue if go install is used without deploy translation file. Another approach is to used go-bindata to avoid such caveat. I'll look into it first.

amanohar · 2017-05-18T15:33:59Z

cmd/generate.go

@@ -18,17 +20,19 @@ const (
 )

 type generateCmd struct {


What about deploy and update commands?

Thanks for reminding. I didn't notice that and will address it.

amanohar · 2017-05-18T15:36:52Z

cmd/generate.go

@@ -59,6 +64,13 @@ func (gc *generateCmd) validate(cmd *cobra.Command, args []string) {
 	var caCertificateBytes []byte
 	var caKeyBytes []byte

+	locale, err := i18n.LoadTranslations(gc.translationsDirectory)


It seems that a lot of setup is happening even before ACS Engine functions are called. Which means evey time someone needs to use the primary ACS Engine functions (in the commands or RP) these steps need to be followed. IMO this should be as simple as translation dir being passed to InitializeTemplateGenerator. And then everything is setup inside it.

Thanks for the suggestion. The translation init routine includes set the locale, bind to translation directory, and select domain. I thought about the approach of have package level init function and have each exported function call it if it haven't called before. GNU gettext doc has the guidance of how to init in library. However, in our case, the ACS Engine package is called by RP, which serves requests with different language on multiple thread. For example, if InitializeTemplateGenerator is called by the same thread on different language, the locale needs to be updated. If InitializeTemplateGenerator is called by different thread, each thread needs to keep its own locale. If InitializeTemplateGenerator is called by the same thread with the same language, then we don't need to init again. I haven't found a simple way to handle that other than the caller needs to do some initialization and pass the locale in the context (adopting Go context pattern).

…ngtli-translation

…r go-bindata

JiangtianLi · 2017-05-19T00:38:41Z

@acs-bot test this please

…ngtli-translation

JiangtianLi · 2017-05-20T00:16:09Z

@amanohar Can you please take a look? There is line ending issue caused in commit so it appears to have a lot of changes. Although those are benign, I discussed with Cole and will address it later (Cole already has related #650). So please don't merge.

…ngtli-translation

JiangtianLi · 2017-06-01T23:38:53Z

@acs-bot test this please

JiangtianLi · 2017-06-01T23:57:27Z

After merge and due to #663, the CRLF changes have been merged and there are much less changes now.

shrutir25 · 2017-07-05T17:09:18Z

@JiangtianLi how do we want to proceed with this PR ?

JiangtianLi · 2017-07-05T17:14:32Z

@shrutir25 It is pending approval. It has been a while so merge is probably needed.

…ngtli-translation

JiangtianLi · 2017-07-07T22:39:41Z

@rjtsdl @weinong PTAL

rjtsdl · 2017-07-09T03:43:22Z

pkg/api/apiloader.go

 	contents, e := ioutil.ReadFile(jsonFile)
 	if e != nil {
-		return nil, "", fmt.Errorf("error reading file %s: %s", jsonFile, e.Error())
+		return nil, "", a.Translator.Errorf("error reading file %s: %s", jsonFile, e.Error())


Do you need to replace more places with Translater.Errorf? At least i know i introduced quite a few of them ...

@rjtsdl Yes, the change is incremental and we can to that in several iterations. The bottom line is to get the framework setup so that people can start use it.

rjtsdl · 2017-07-09T03:48:24Z

@acs-bot test this please

rjtsdl

LGTM, we can iterate later to enforce this to the existing fmt.Errorf

weinong · 2017-07-11T18:22:33Z

Reviewed 1 of 28 files at r2, 114 of 1420 files at r4, 9 of 16 files at r5, 11 of 33 files at r6, 15 of 18 files at r7.
Review status: 144 of 145 files reviewed at latest revision, 8 unresolved discussions.

pkg/i18n/i18n.go, line 117 at r7 (raw file):

}

// NT translates a text string into the appropriate plural form, based on GNU's gettext library.

any example?

pkg/i18n/i18n.go, line 127 at r7 (raw file):

}

// NErrorf produces an error with a translated error string in the appropriate plural form.

any example of using this?

test/i18n/i18ntestinput.go, line 8 at r7 (raw file):

)

// This file is used to generate the test translation file

these instructions need to be in README.md perhaps under pkg/i18n

Comments from Reviewable

weinong · 2017-07-11T18:33:08Z

LGTM
@seanknox as well

weinong

please also add README about how to do the localization.

seanknox

Hey Jiangtian, some issues:

some code is copyrighted by you personally
can't include private Github repos (you mentioned a PR opened for gosexy/gettext to fix this)
Didn't see any unit tests for the changes

seanknox · 2017-07-11T18:55:47Z

Dockerfile

@@ -29,6 +29,10 @@ RUN git clone https://github.com/akesterson/cmdarg.git /tmp/cmdarg \
 RUN git clone https://github.com/akesterson/shunit.git /tmp/shunit \
    && cd /tmp/shunit && make install && rm -rf /tmp/shunit

+# Go tool for internationalization and localization
+RUN go get github.com/JiangtianLi/gettext/... \


Just echoing others and calling out we can't include this change as is, the repo has to be publically accessible.

seanknox · 2017-07-11T18:57:34Z

pkg/i18n/translations/default/LC_MESSAGES/acsengine.po

+# Test translations for acsengine package.
+# Copyright (C) 2017
+# Jiangtian Li <[email protected]>, 2017.
+#


I don't think we can include code that isn't MIT/Apache v2/etc.

seanknox · 2017-07-11T18:57:51Z

pkg/i18n/translations/en_US/LC_MESSAGES/acsengine.po

+# Test translations for acsengine package.
+# Copyright (C) 2017
+# Jiangtian Li <[email protected]>, 2017.
+#


Copyright problem here too, and other spots. Won't call all of them out.

…ngtli-translation

JiangtianLi · 2017-07-21T01:02:52Z

Review status: 144 of 145 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

pkg/i18n/i18n.go, line 117 at r7 (raw file):

Previously, weinong (Weinong Wang) wrote…

any example?

Yes, example is in i18n_test.go

pkg/i18n/i18n.go, line 127 at r7 (raw file):

Previously, weinong (Weinong Wang) wrote…

any example of using this?

Also in i18n_test.go

Comments from Reviewable

…ngtli-translation

JiangtianLi · 2017-07-25T17:00:38Z

@seanknox @anhowe @amanohar PTAL

Dismissing review as it's been signed off by others.

JiangtianLi · 2017-07-26T18:59:20Z

@acs-bot test this please

JiangtianLi · 2017-07-28T17:19:59Z

Review status: 123 of 160 files reviewed at latest revision, 10 unresolved discussions.

pkg/i18n/translations/default/LC_MESSAGES/acsengine.po, line 4 at r7 (raw file):

Previously, seanknox (Sean Knox) wrote…

I don't think we can include code that isn't MIT/Apache v2/etc.

Discussed with Sean offline. It is generated by poedit and will be modified by loc team. Currently it follows the same as Kubernetes translation files.

Comments from Reviewable

JiangtianLi · 2017-07-28T17:20:30Z

Review status: 123 of 160 files reviewed at latest revision, 10 unresolved discussions.

pkg/i18n/translations/en_US/LC_MESSAGES/acsengine.po, line 4 at r7 (raw file):

Previously, seanknox (Sean Knox) wrote…

Copyright problem here too, and other spots. Won't call all of them out.

The same as other translation file copyright.

Comments from Reviewable

JiangtianLi · 2017-07-28T17:20:46Z

Review status: 123 of 160 files reviewed at latest revision, 10 unresolved discussions.

test/i18n/i18ntestinput.go, line 8 at r7 (raw file):

Previously, weinong (Weinong Wang) wrote…

these instructions need to be in README.md perhaps under pkg/i18n

Added README.md

Comments from Reviewable

JiangtianLi · 2017-07-28T17:21:34Z

Review status: 123 of 160 files reviewed at latest revision, 10 unresolved discussions.

Dockerfile, line 33 at r2 (raw file):

Previously, seanknox (Sean Knox) wrote…

Looks like https://github.com/JiangtianLi/gettext is public now (might have been from the beginning), so we're OK to use the fork, as long as we've registered it at https://ossmsft.visualstudio.com/_oss.

Resolved offline. The dependence is all MIT licence.

Comments from Reviewable

JiangtianLi · 2017-07-28T17:26:37Z

@acs-bot test this please

…ngtli-translation

JiangtianLi · 2017-07-31T21:02:24Z

@acs-bot test this please

msftclas added the cla-not-required label May 15, 2017

First round of i18n in acs-engine based on gettext. Subsequent change…

ecdd9b9

…s will replace and extract resource strings in acs-engine and make acsengine/api package's i18n used by other go program.

JiangtianLi force-pushed the jiangtli-translation branch from a7459db to ecdd9b9 Compare May 16, 2017 22:04

Update acs-engine go source to use translation functions. Generate tr…

78be458

…anslation resource files.

amanohar previously requested changes May 18, 2017

View reviewed changes

JiangtianLi added 5 commits May 18, 2017 11:47

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

261ffdf

…ngtli-translation

Vendor github.com/leonelquinteros/gotext package using glide

1867b0c

Rebase and update translation for update and deploy command

638031b

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

201c149

…ngtli-translation

Move test translation files so that translations directory is used fo…

f9e7193

…r go-bindata

JiangtianLi added 3 commits May 18, 2017 19:43

Use go-bindata to add resource strings to acs-engine binary

150b711

Fix reading bindata and unit test

db742c9

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

1a54a26

…ngtli-translation

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

bcbcac0

…ngtli-translation

Update translation files

f16bb48

JiangtianLi force-pushed the jiangtli-translation branch from 66f6df2 to f16bb48 Compare June 1, 2017 23:43

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

3847870

…ngtli-translation

rjtsdl reviewed Jul 9, 2017

View reviewed changes

rjtsdl approved these changes Jul 10, 2017

View reviewed changes

weinong approved these changes Jul 11, 2017

View reviewed changes

seanknox suggested changes Jul 11, 2017

View reviewed changes

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

9ea4527

…ngtli-translation

JiangtianLi added 4 commits July 24, 2017 17:01

More fix/refactor after rebase and add README

67642cb

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

8337029

…ngtli-translation

Update resource files

39fbb34

Add LCG files converted from PO files.

7816133

Update translation bindata

91ad8cd

seanknox approved these changes Jul 26, 2017

View reviewed changes

JiangtianLi added 3 commits July 28, 2017 13:20

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

e7dc96a

…ngtli-translation

Merge branch 'master' of https://github.com/Azure/acs-engine into jia…

4aa260e

…ngtli-translation

Remove go generated translation bindata

68e8ddb

JiangtianLi force-pushed the jiangtli-translation branch from afad27e to 68e8ddb Compare July 31, 2017 18:55

JiangtianLi merged commit b41df8f into Azure:master Jul 31, 2017

seanknox mentioned this pull request Aug 1, 2017

chore(vendor): add JiangtianLi/gettext to glide, hardcode vers, bump Azure vers #1146

Merged

JiangtianLi deleted the jiangtli-translation branch October 24, 2017 22:15

First round of i18n in acs-engine based on gettext. Subsequent change… #627

First round of i18n in acs-engine based on gettext. Subsequent change… #627

Conversation

JiangtianLi commented May 15, 2017 • edited by acs-bot Loading

JiangtianLi commented May 15, 2017

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

JiangtianLi commented May 19, 2017

JiangtianLi commented May 20, 2017

JiangtianLi commented Jun 1, 2017

JiangtianLi commented Jun 1, 2017

shrutir25 commented Jul 5, 2017

JiangtianLi commented Jul 5, 2017

JiangtianLi commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjtsdl commented Jul 9, 2017

rjtsdl left a comment

Choose a reason for hiding this comment

weinong commented Jul 11, 2017

weinong commented Jul 11, 2017

weinong left a comment

Choose a reason for hiding this comment

seanknox 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

JiangtianLi commented Jul 21, 2017

JiangtianLi commented Jul 25, 2017

JiangtianLi commented Jul 26, 2017

JiangtianLi commented Jul 28, 2017

JiangtianLi commented Jul 28, 2017

JiangtianLi commented Jul 28, 2017

JiangtianLi commented Jul 28, 2017

JiangtianLi commented Jul 28, 2017

JiangtianLi commented Jul 31, 2017

JiangtianLi commented May 15, 2017 •

edited by acs-bot

Loading