Use i18n for clusterctl command descriptions and update dep

[wangzhen127]

What this PR does / why we need it:
This PR uses i18n for existing clusterctl command descriptions and also include kubernetes repo in the vendor directory. This is the 1st PR of #168. There will be follow-up PRs to add validate cluster command.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

@kubernetes/kube-deploy-reviewers

[k8s-ci-robot]

Hi @wangzhen127. Thanks for your PR.

I'm waiting for a kubernetes or kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[wangzhen127]

/cc @karan @jessicaochen

[jessicaochen]

The amount and kind of code being vendored in seems a lot more than what should be needed for internationalization. Can you provide context as to why all this vendored code is needed to add internationalization?

[wangzhen127]

Because it is using k8s.io/kubernetes/pkg/kubectl/util/i18n. I am just following other kubernetes cmd tools' practices. Note kops is also using this package for internalization.

I only added one repository in Gopkg.toml, i.e. k8s.io/kubernetes. The majority of the added vendor code is from that.

[karan]

I don't see it in the design proposal. What is the motivation for adding i18n so early in the project?

I know i18n is important, do other k8s projects do it? How do they do it? Can we not vendor in k8s/k8s please -- this is a very hard to review PR, and will make the repo unnecessarily large.

[wangzhen127]

Why is using i18n a big problem? This is just some implementation detail. I actually treat this PR as a very small one. The real manual changes are

1. a few lines in Gopkg.toml to add k8s.io/kubernetes
2. a few lines in clusterctl/cmd/ files to use i18n.T

All other changes are automatically applied by running "dep ensure".

And why is vendoring k8s.io/kubernetes a problem? If it is purely due to hard-to-review PR. I can prepare this PR as 2 commits: 1) manual changes; 2) auto changes by "dep ensure". And then before merge, I will squash those two.

And even if adding k8s.io/kubernetes makes the repo large, I don't see it as a problem. That's the purpose of the vendor/ directory. This repo is not getting much more complex by having some vendor code.

And as I am aware of, both kubectl and kops are using k8s.io/kubernetes/pkg/kubectl/util/i18n.

With the above being said, I do not have a strong opinion on adding i18n support or not right now. I originally treat this as a simple fix PR. This PR exists because in #203 I used i18n and I found build issues. So I prepared this separate PR to add the i18n support. If you guys have strong opinions, I can just drop the i18n support now. But my personal feeling is that we may need some library from kubernetes in the future any way.

[wangzhen127]

I updated the PR to make it easier to review. I split the original 1 commit to 2 commits now. You can go to commits tab to see them.

1. First commit contains manual changes and is the one you should review. Only 5 files are changed and should be easy to review.
2. Second commit contains changes automatically applied by running "dep ensure".

I will squash them before merge.

[karan]

afaik we decided that for now cluster api would be released independent of k8s releases. does this tie us to a specific k8s version now?

[wangzhen127]

Maybe, I am not sure. I think it depends on which kubernetes library we use and if they differ in different versions.

I am using the same version as other k8s.io repositories. For example, k8s.io/apiserver etc are all using release-1.9. Later when we bump versions, I think we should bump them together.

[karan]

And even if adding k8s.io/kubernetes makes the repo large, I don't see it as a problem

Personally, at such an early stage of the project, I see i18n as a premature optimization.

The problem is build times. Because we do not have automated testing currently (or pretty much any testing), changes need to be tested by building the api locally. This can slow down develop significantly if we are pulling in k8s/k8s.

If you can make sure that the build times do not go up significantly, I don't have any issues.

[wangzhen127]

Here is some experiment on my macbook:

(master)zhenw@zhenw-macbookpro2[~/go/src/sigs.k8s.io/cluster-api/clusterctl]$ go clean
(master)zhenw@zhenw-macbookpro2[~/go/src/sigs.k8s.io/cluster-api/clusterctl]$ time go build

real	0m3.544s
user	0m3.576s
sys	0m1.292s
(master)zhenw@zhenw-macbookpro2[~/go/src/sigs.k8s.io/cluster-api/clusterctl]$ git checkout update-i18n
Checking out files: 100% (10915/10915), done.
Switched to branch 'update-i18n'
Your branch is up to date with 'origin/update-i18n'.
(update-i18n)zhenw@zhenw-macbookpro2[~/go/src/sigs.k8s.io/cluster-api/clusterctl]$ go clean
(update-i18n)zhenw@zhenw-macbookpro2[~/go/src/sigs.k8s.io/cluster-api/clusterctl]$ time go build

real	0m3.302s
user	0m3.447s
sys	0m1.116s

I would say the build time difference is negligible.

[jessicaochen]

I did a sanity check and it does look like adding i18n does indeed pull in 10k+ files into vendor (mostly k8s testing files). Thanks for breaking up the actual manual changes from the generated changes. It helps a lot for reviewing.

lgtm (leaving backslash for the secondary reviewer)
/approve
/ok-to-test

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jessicaochen, wangzhen127

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 [jessicaochen]

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

[wangzhen127]

/hold

Should I hold this for @karan ? Otherwise I think it will be auto merged?

[jessicaochen]

Thanks for being thoughtful, I purposely did not give a lgtm with a backslash so it would not auto-merge. The secondary reviewer can give you that once they lgtm.

[maisem]

We should not be pulling in k8s.io/k8s. That seems like an unnecessary dependency which we have been purposefully trying to avoid.

@k4leung4 @roberthbailey @krousey

[krousey]

/unapprove
@maisem agreed. Rescinding approval until we can find a better solution.

[krousey]

Guess I have to say
/approve cancel

[krousey]

This seems like a beast of an import when all you use is the following function:

// T translates a string, possibly substituting arguments into it along
// the way. If len(args) is > 0, args1 is assumed to be the plural value
// and plural translation is used.
func T(defaultValue string, args ...int) string {
	if len(args) == 0 {
		return gettext.PGettext("", defaultValue)
	}
	return fmt.Sprintf(gettext.PNGettext("", defaultValue, defaultValue+".plural", args[0]),
		args[0])
}

Which only relies on "github.com/chai2010/gettext-go/gettext". I would much rather you duplicate that function than import over 2 million lines of code.

[wangzhen127]

Sounds good. I will drop the internationalization for now. We can come back to this later.