We use that pattern to implement "oneof" on the spec side in several places (e.g., VolumeSource), but this may be the only instance of it on the status side:

type ContainerState struct {
    Waiting     *ContainerStateWaiting    `json:"waiting,omitempty" description:"details about a waiting container"`
    Running     *ContainerStateRunning    `json:"running,omitempty" description:"details about a running container"`
    Termination *ContainerStateTerminated `json:"termination,omitempty" description:"details about a terminated container"`
}

I can see how that would be problematic. Would a field indicating the state help, like a tagged union?

cc @thockin @dchen1107

I can see how that would be problematic. Would a field indicating the state help, like a tagged union?

@bgrant0607 can you give an example of what you meant it should look like?

From my side I meant something like the below.
the point is that that the client can get a key, knowing in advance which key that is, and store its value.
"state": {
"type": "running"
"startedAt": "2015-04-17T14:12:29Z"
}

I was suggesting just adding a type field to indicate which map fields can be expected to be populated.

"state": {
  "type": "running",
  "running": {
    "startedAt": "2015-04-17T14:12:29Z"
  }
}

I was suggesting just adding a type field to indicate which map fields can be expected to be populated.

"state": {
"type": "running",
"running": {
"startedAt": "2015-04-17T14:12:29Z"
}
}

@bgrant0607
it's definitely better than the current implementation, but it's still not as simplified as I think it should be :)

@bgrant0607 consider that even given the 'type', clients will need to hardcode the logic to search for specific attributes per type.
so 'if it's running, then it might have a "startedAt" attribute inside the "running" section'
not sure why aren't those just flattened.
if it's not running then the startedAt might be empty (and might be just another key next to the type key)

@abonas

I'd like to flatten them, I think it would be a better API. I don't know
how to do it with Go's JSON libs, short of unmarshalling into dumb
structures and then decoding by hand (and breaking autogenerated docs) or
doing semi-manual decoding (and breaking automated docs).

Embedding does not work: http://play.golang.org/p/AWmxc9LYOF

Manual-assist decode is not too bad but, well, manually maintained:
http://play.golang.org/p/ZeWIc26RTc

Reflection could work, but I can't find the bug in this:
http://play.golang.org/p/wdJE9VNo2H

On Tue, Apr 21, 2015 at 1:41 AM, abonas notifications@github.com wrote:

@bgrant0607 https://github.com/bgrant0607 consider that even given the
'type', clients will need to hardcode the logic to search for specific
attributes per type.
so 'if it's running, then it might have a "startedAt" attribute inside the
"running" section'
not sure why aren't those just flattened.
if it's not running then the startedAt might be empty (and might be just
another key next to the type key)

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

@abonas I'd like to flatten them, I think it would be a better API. I don't know how to do it with Go's JSON libs, short of unmarshalling into dumb structures and then decoding by hand (and breaking autogenerated docs) or doing semi-manual decoding (and breaking automated docs). Embedding does not work: http://play.golang.org/p/AWmxc9LYOF Manual-assist decode is not too bad but, well, manually maintained: http://play.golang.org/p/ZeWIc26RTc Reflection could work, but I can't find the bug in this: http://play.golang.org/p/wdJE9VNo2H

@thockin , I'm still looking at all the info you provided here (such a detailed comment - thanks!)
but from a design point of view (not only REST api or Go aspects, but "deeper") - I am wondering why each status has different properties to begin with?
The way I see it, a status should be a unified object with "code" (running/failed/etc.), reason, startedAt, endedAt (failed ones will have both startedAt and endedAt populated, running ones will have only startedAt populated - it's quite standard and done for recurring schedulings for example too in other projects)
'Reason' is relevant for both waiting and failed/terminated, for example.
StartedAt imo should be present in failed ones (it is not present there today) as someone might want to know for how long did it run by substracting ended-started props. why deny this information from users? :)
So if it was a single type of object, I guess the REST api part would be easier as well?

Brian, Dawn

If we're willing to accept some custom JSON code, this pattern is pretty
clean:

http://play.golang.org/p/LYO6u0D1A9

On Wed, Apr 22, 2015 at 1:28 AM, abonas notifications@github.com wrote:

@abonas https://github.com/abonas I'd like to flatten them, I think it
would be a better API. I don't know how to do it with Go's JSON libs, short
of unmarshalling into dumb structures and then decoding by hand (and
breaking autogenerated docs) or doing semi-manual decoding (and breaking
automated docs). Embedding does not work:
http://play.golang.org/p/AWmxc9LYOF Manual-assist decode is not too bad
but, well, manually maintained: http://play.golang.org/p/ZeWIc26RTc
Reflection could work, but I can't find the bug in this:
http://play.golang.org/p/wdJE9VNo2H

@thockin https://github.com/thockin , I'm still looking at all the info
you provided here (such a detailed comment - thanks!)
but from a design point of view (not only REST api or Go aspects, but
"deeper") - I am wondering why each status has different properties to
begin with?
The way I see it, a status should be a unified object with "code"
(running/failed/etc.), reason, startedAt, endedAt (failed ones will have
both startedAt and endedAt populated, running ones will have only startedAt
populated - it's quite standard and done for recurring schedulings for
example too in other projects)
'Reason' is relevant for both waiting and failed/terminated, for example.
StartedAt imo should be present in failed ones (it is not present there
today) as someone might want to know for how long did it run by
substracting ended-started props. why deny this information from users? :)
So if it was a single type of object, I guess the REST api part would be
easier as well?

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

@thockin, any verdict on how and when this will be fixed?

No verdict yet. The bar is pretty high for API changes between now and
1.0. I've linked this off the v1 accumulator bug, but I don't have high
hopes for it.

On Tue, Apr 28, 2015 at 4:46 AM, abonas notifications@github.com wrote:

@thockin https://github.com/thockin, any verdict on how and when this
will be fixed?

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

We don't have time to change this now, and don't have a good solution.

At most, I suggest we add the type field.

@bgrant0607
'type' field will be great as (at least) temporary solution.
any idea when we can expect that?

A PR would be welcome. :-)

@abonas I agree, though I'm not sure we have time to make the change soon.

Since all the "states" contain a subset of the fields of ContainerStateTerminated, I think it would be simpler to essentially use those fields (some renamed), plus a status field.

type ContainerState struct {
    ContainerID string
    Status       string
    Reason      string
    Message     string
    StartTime   util.Time
    TerminationTime  util.Time
    ExitCode    *int
    Signal      *int
}

Brian,

For longer term, are you deeply against using custom JSON unmarshalling to
get a better API? I know it causes some problems with doc-generation, but
I don't think we should let that dictate our API design, and the API is,
first and foremost, the JSON documents and not the go structs.

For example, suppose we used something like 'go generate' to expand a
templated form of the JSON marshal/unmarshal code. Effectively, the
v2beta1 api.go file would look like:

type RunningStatus struct {
StartedAt string
Foo       int // overlaps with FailedStatus.Foo
}

type FailedStatus struct {
EndedAt string
Foo     int // overlaps with RunningStatus.Foo
}

type ThingStatus struct {
Status  string
Running *RunningStatus `json:",omitempty"`
Failed  *FailedStatus  `json:",omitempty"`
}

// go:generate json_discriminated_oneof ThingStatus Status

and elsewhere

type FooFrobber struct {
Path  string
Foo      int // overlaps with BarFrobber.Foo
}

type BarFrobber struct {
Token string
Foo     string // overlaps with FooFrobber.Foo
}

type Frobber struct {
Type  string
Foo *FooFrobber `json:",omitempty"`
Bar  *BarFrobber  `json:",omitempty"`
}

// go:generate json_discriminated_oneof Frobber Type

This seems imminently doable and produces a nearly-ideal JSON api for
things like volumes, statuses, etc.

On Wed, May 6, 2015 at 11:55 PM, Brian Grant notifications@github.com
wrote:

@abonas https://github.com/abonas I agree, though I'm not sure we have
time to make the change soon.

Since all the "states" contain a subset of the fields of
ContainerStateTerminated, I think it would be simpler to essentially use
those fields (some renamed), plus a status field.

type ContainerState struct {
ContainerID string
Status string
Reason string
Message string
StartTime util.Time
TerminationTime util.Time
ExitCode *int
Signal *int
}

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

Discussion on custom unmarshaling moved to #7203.

But for ContainerState, there's no good reason, IMO. Different info isn't required for different "states". Furthermore, we're planning to eliminate Phase #7856.

@abonas I agree, though I'm not sure we have time to make the change soon.

Since all the "states" contain a subset of the fields of ContainerStateTerminated, I think it would be simpler to essentially use those fields (some renamed), plus a status field.

type ContainerState struct {
ContainerID string
Status string
Reason string
Message string
StartTime util.Time
TerminationTime util.Time
ExitCode *int
Signal *int
}

@bgrant0607
Does this change go just into v1 and not the other api versions?
Does v1beta3/2/1 require conversion for this or can it be done without the conversion?

The change would go into at most v1. We'll be making decisions about what's in and out of v1 over the next few days. v1beta3, etc. wouldn't change.

Note that we plan to get rid of v1beta1 and v1beta2 very soon.

The change would go into at most v1. We'll be making decisions about what's in and out of v1 over the next few days. v1beta3, etc. wouldn't change.

Note that we plan to get rid of v1beta1 and v1beta2 very soon.

So does that mean that a PR containing this change for v1 must also include conversion handling for v1beta3?

Yes, and v1beta1 and v1beta2 until we actually disable/remove them.

@abonas Were you planning to work on this? (Since you asked about conversions.)

@caesarxuchao will rename state.termination to state.terminated (ditto for lastState).

@abonas Were you planning to work on this? (Since you asked about conversions.)

I wanted to, but now it seems I can't make it given the timeframes.

@abonas There are no sig labels on this issue. Please add a sig label by:
(1) mentioning a sig: @kubernetes/sig-<team-name>-misc
(2) specifying the label manually: /sig <label>

Note: method (1) will trigger a notification to the team. You can find the team list here.

/sig node

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

This isn't going to change any time soon. Ref #8190