Improve conversion to support multiple packages

[smarterclayton]

OpenShift uses multiple API packages (types are split) which
Kube will also eventually have as we introduce more plugins.
These changes make the generators able to handle importing different
API object packages into a single generator function.

Other cleanups that make Kube only assumptions less hardcoded.

[k8s-bot]

GCE e2e build/test passed for commit 45b2899de912d95e48e9b783efc6f34c18b5915c.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[saad-ali]

@bgrant0607 for v1.0 triage

[smarterclayton]

This is not critical for us because we're carrying it as a patch - can be post 1.0

----- Original Message -----

@bgrant0607 for v1.0 triage

---

Reply to this email directly or view it on GitHub:
#9971 (comment)

[bgrant0607]

Ref #1744

[bgrant0607]

cc @wojtek-t

[wojtek-t]

I'm no the GOTO Amsterdam conference right now - I will try to look into it today evening.

[wojtek-t]

I took a very quick look now - generaly I like the direction of this change, but I will take a deeper look in the evening.

[wojtek-t]

What is the purpose of it? I don't see it used anywhere and I'm not sure if I understand why it is really needed.

[smarterclayton]

Right now conversions for Kube objects are private methods. OpenShift embeds some of the Kube objects (pod, type meta, object meta) and so when the OpenShift private convertor functions they have to go back to scheme.Convert in order to get access to the method. Ideally we'd either make the core conversions public or visible through a fast path named accessor, but this was the compromise I made for now.

On Jun 19, 2015, at 4:08 AM, Wojciech Tyczynski notifications@github.com wrote:

In pkg/runtime/conversion_generator.go:

OverwritePackage(pkg, overwrite string)

• AssumePrivateConversions()
  What is the purpose of it? I don't see it used anywhere and I'm not sure if I understand why it is really needed.

—
Reply to this email directly or view it on GitHub.

[wojtek-t]

OK I see - thanks for explanation.

[wojtek-t]

I took a deeper look (sorry for late response). I had just few minor comments.

[smarterclayton]

Comments addressed or answered.

[k8s-bot]

GCE e2e build/test passed for commit ab60585.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[wojtek-t]

LGTM - @smarterclayton do you want this to be merged before 1.0?
If so we need 2 leads to LGTM it according to our current policy.

[smarterclayton]

It's not critical for 1.0, although I believe it's low risk since it's a
prebuild thing.

On Jun 29, 2015, at 3:38 AM, Wojciech Tyczynski notifications@github.com
wrote:

LGTM - @smarterclayton https://github.com/smarterclayton do you want this
to be merged before 1.0?
If so we need 2 leads to LGTM it according to our current policy.

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

[wojtek-t]

I agree it's relatively low risk.

@davidopp as the first lead to LGTM

[davidopp]

I don't really know this code at all, but why do you say it's low risk? The changes to conversion_generator.go and deep_copy_generator.go look substantial.

[smarterclayton]

The observed output on a large sample size of our objects is identical
(except for the desired outcome). So if there are bugs, they don't yield a
different output on our API. But it's not zero risk.

On Jun 29, 2015, at 5:38 PM, David Oppenheimer notifications@github.com
wrote:

I don't really know this code at all, but why do you say it's low risk? The
changes to conversion_generator.go and deep_copy_generator.go look
substantial.

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

[thockin]

Unless this is directly tied to a v1 issue (it's not) it's not going in before 1.0. This looks good overall, but it can queue up for after 1.0

[wojtek-t]

@smarterclayton - in that case I think I will merge it jut after 1.0

[thockin]

low risk by itself is not sufficient criteria at this point :)

On Tue, Jun 30, 2015 at 1:09 AM, Wojciech Tyczynski <
notifications@github.com> wrote:

@smarterclayton https://github.com/smarterclayton - in that case I
think I will merge it jut after 1.0

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

[smarterclayton]

That's fine - this is just upstream refactors we carry.

On Tue, Jun 30, 2015 at 2:01 PM, Tim Hockin notifications@github.com
wrote:

low risk by itself is not sufficient criteria at this point :)

On Tue, Jun 30, 2015 at 1:09 AM, Wojciech Tyczynski <
notifications@github.com> wrote:

@smarterclayton https://github.com/smarterclayton - in that case I
think I will merge it jut after 1.0

—
Reply to this email directly or view it on GitHub
<
#9971 (comment)

.

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

[k8s-bot]

GCE e2e build/test passed for commit 4b5bc8c385f2a12afccb9dcaab26e5542eb758a1.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[pmorie]

@smarterclayton @thockin I'm not sure how this one is the in the 'checks passed' state -- I'm working on a change to make OpenShift run the upstream tests and found this as a breakage. The tests for pkg/runtime need to change since they call NewConversionGenerator. I tested in a local copy of this branch and the unit tests fail:

$ hack/test-go.sh pkg/runtime
Running tests for APIVersion: v1beta3 with etcdPrefix: registry
+++ [0722 17:42:48] Running unit tests without code coverage
# github.com/GoogleCloudPlatform/kubernetes/pkg/runtime_test
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/conversion_generation_test.go:37: not enough arguments in call to runtime.NewConversionGenerator
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/conversion_generation_test.go:50: not enough arguments in call to g.RegisterConversionFunctions
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/deep_copy_generation_test.go:36: not enough arguments in call to runtime.NewDeepCopyGenerator
_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/runtime/deep_copy_generation_test.go:50: too many arguments in call to g.WriteImports
FAIL    github.com/GoogleCloudPlatform/kubernetes/pkg/runtime [build failed]
!!! Error in hack/test-go.sh:159
  'go test "${goflags[@]:+${goflags[@]}}" ${KUBE_RACE} ${KUBE_TIMEOUT} "${@+${@/#/${KUBE_GO_PACKAGE}/}}" "${testargs[@]:+${testargs[@]}}"' exited with status 2
Call stack:
  1: hack/test-go.sh:159 runTests(...)
  2: hack/test-go.sh:221 main(...)
Exiting with status 1

[smarterclayton]

Probably because of another change since this last built? Something had to have changed.

[k8s-bot]

GCE e2e build/test passed for commit 4b5bc8c385f2a12afccb9dcaab26e5542eb758a1.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[wojtek-t]

LGTM

@smarterclayton - can you pleas rebase?

[pmorie]

@smarterclayton possibly -- but we need to verify whether the unit tests will actually pass at this point

[smarterclayton]

Rebased

On Wed, Jul 29, 2015 at 11:30 AM, Paul Morie notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton possibly -- but we
need to verify whether the unit tests will actually pass at this point

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

[k8s-bot]

GCE e2e build/test passed for commit 1d41f5a.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[wojtek-t]

LGTM

[mikedanese]

This broke unit tests. For some reason shippable decided not to run on this. 👎 I see that @pmorie commented on this.

[smarterclayton]

Fixed in #12033

On Thu, Jul 30, 2015 at 1:52 PM, Mike Danese notifications@github.com
wrote:

This broke unit tests. For some reason shippable decided not to run on
this. [image: 👎]

—
Reply to this email directly or view it on GitHub
#9971 (comment)
.

Clayton Coleman | Lead Engineer, OpenShift

[liggitt]

is this a typo? wasn't inKey/PkgPath() and outKey.PkgPath() already added above?

[smarterclayton]

Needs to be out value, otherwise values from a different package don't get added to the imports.