First round of i18n in acs-engine based on gettext. Subsequent change… #627
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ import ( | |
|
|
||
| "github.com/Azure/acs-engine/pkg/acsengine" | ||
| "github.com/Azure/acs-engine/pkg/api" | ||
| "github.com/Azure/acs-engine/pkg/i18n" | ||
| "github.com/leonelquinteros/gotext" | ||
| ) | ||
|
|
||
| const ( | ||
|
|
@@ -18,17 +20,19 @@ const ( | |
| ) | ||
|
|
||
| type generateCmd struct { | ||
|
There was a problem hiding this comment. What about deploy and update commands? There was a problem hiding this comment. Thanks for reminding. I didn't notice that and will address it. |
||
| translationsDirectory string | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| apimodelPath string | ||
| outputDirectory string // can be auto-determined from clusterDefinition | ||
| caCertificatePath string | ||
| caPrivateKeyPath string | ||
| classicMode bool | ||
| noPrettyPrint bool | ||
| parametersOnly bool | ||
|
|
||
| // Parsed from inputs | ||
| containerService *api.ContainerService | ||
| apiVersion string | ||
| locale *gotext.Locale | ||
| } | ||
|
|
||
| func NewGenerateCmd() *cobra.Command { | ||
|
|
@@ -44,6 +48,7 @@ func NewGenerateCmd() *cobra.Command { | |
| } | ||
|
|
||
| f := generateCmd.Flags() | ||
| f.StringVar(&gc.translationsDirectory, "translations-directory", "", "translations directory (translations in the current directory if absent)") | ||
| f.StringVar(&gc.apimodelPath, "api-model", "", "") | ||
| f.StringVar(&gc.outputDirectory, "output-directory", "", "output directory (derived from FQDN if absent)") | ||
| f.StringVar(&gc.caCertificatePath, "ca-certificate-path", "", "path to the CA certificate to use for Kubernetes PKI assets") | ||
|
|
@@ -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) | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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). |
||
| if err != nil { | ||
| log.Fatalf("error loading translation files: %s", err.Error()) | ||
| } | ||
|
|
||
| i18n.Initialize(locale) | ||
|
|
||
| if gc.apimodelPath == "" { | ||
| if len(args) > 0 { | ||
| gc.apimodelPath = args[0] | ||
|
|
@@ -75,7 +87,12 @@ func (gc *generateCmd) validate(cmd *cobra.Command, args []string) { | |
| log.Fatalf("specified api model does not exist (%s)", gc.apimodelPath) | ||
| } | ||
|
|
||
| apiloader := &api.Apiloader{ | ||
| Translator: &i18n.Translator{ | ||
| Locale: locale, | ||
| }, | ||
| } | ||
| containerService, apiVersion, err := apiloader.LoadContainerServiceFromFile(gc.apimodelPath) | ||
| if err != nil { | ||
| log.Fatalf("error parsing the api model: %s", err.Error()) | ||
| } | ||
|
|
@@ -93,13 +110,19 @@ func (gc *generateCmd) validate(cmd *cobra.Command, args []string) { | |
|
|
||
| gc.containerService = containerService | ||
| gc.apiVersion = apiVersion | ||
| gc.locale = locale | ||
| } | ||
|
|
||
| func (gc *generateCmd) run(cmd *cobra.Command, args []string) error { | ||
| gc.validate(cmd, args) | ||
| log.Infoln("Generating...") | ||
|
|
||
| ctx := acsengine.Context{ | ||
| Translator: &i18n.Translator{ | ||
| Locale: gc.locale, | ||
| }, | ||
| } | ||
| templateGenerator, err := acsengine.InitializeTemplateGenerator(ctx, gc.classicMode) | ||
| if err != nil { | ||
| log.Fatalln("failed to initialize template generator: %s", err.Error()) | ||
| } | ||
|
|
@@ -120,7 +143,12 @@ func (gc *generateCmd) run(cmd *cobra.Command, args []string) error { | |
| } | ||
| } | ||
|
|
||
| writer := &acsengine.ArtifactWriter{ | ||
| Translator: &i18n.Translator{ | ||
| Locale: gc.locale, | ||
| }, | ||
| } | ||
| if err = writer.WriteArtifacts(gc.containerService, gc.apiVersion, template, parameters, gc.outputDirectory, certsGenerated, gc.parametersOnly); err != nil { | ||
| log.Fatalf("error writing artifacts: %s \n", err.Error()) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should ACS Engine be getting stuff from private repos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I submitted a PR to that tool: gosexy/gettext#16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just echoing others and calling out we can't include this change as is, the repo has to be publically accessible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.