Suggest that we use "" to mean internal - same rule that I made for the Kind: field. Set on the wire, blank in memory, because it's implicit in the go type. This will mean much less changes to tests.

There's an argument to pass to go test to get this timeout behavior-- let's not recreate it in every test?

Hrm, I'd argue that its relevant here because it tells you specifically what is timing out.

If it times out via the test, I believe you get a stack trace, which is even more relevant because you can see where it's stuck.

Personally I find the massive golang stack dumps to be not particularly helpful when debugging a test vs. a focused message. But I don't super care. I've added the timeout to the test script, since that's a good idea anyway. If you feel strongly I'll revert this part of the change.

Fair enough. They are super long and force me to type 2>&1|less way more often than I care to.

I don't get it - once we have v1beta2, v1beta3, etc, how do you externalize to those? I liked that the first cut of this had the internalize/externalize in the v1beta1 package - that just seems "righter" to me.

I won't have time to re-review this this week, so I'll roll with whatever you and @lavalamp decide, but I don't get it yet :)

My thought was that when that happens, externalize will have to take a version parameter. We can add that now if it aids clarity.

The old code didn't handle this correctly either, it just placed the conversion in the package.

Probably the right thing to do is to move externalize into the v1beta1 package, and reference it from here.

We can do that in a second pass.

I agree-- the interface for externalize should take the destination version as an additional parameter. I think we can change that later, though. As for placing the translation functions in the v1betaX packages, that may be possible-- I'm OK with it as long as only the api package has to include those other packages. I want to contain the ugly translations and keep it separate from the rest of the codebase as much as possible.

SGTM

On Tue, Jul 22, 2014 at 10:29 PM, Daniel Smith notifications@github.com
wrote:

In pkg/api/helper.go:

•   }
  

•   result.APIVersion = ""
  

•   return &result, nil
  

• default:

•   fn, ok := internalFuncs[reflect.ValueOf(cObj).Elem().Type().Name()]
  

•   if !ok {
  

•       fmt.Printf("unknown object to internalize: %s", reflect.ValueOf(cObj).Type().Name())
  

•       panic(fmt.Sprintf("unknown object to internalize: %s", reflect.ValueOf(cObj).Type().Name()))
  

•   }
  

•   return fn(cObj)
  

• }
• return obj, nil
  +}

+// TODO: switch to registered functions for each type.
+func externalize(obj interface{}) (interface{}, error) {

I agree-- the interface for externalize should take the destination
version as an additional parameter. I think we can change that later,
though. As for placing the translation functions in the v1betaX packages,
that may be possible-- I'm OK with it as long as only the api package has
to include those other packages. I want to contain the ugly translations
and keep it separate from the rest of the codebase as much as possible.

Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/565/files#r15271699
.

Sounds like we're all on the same page here.