Code review comment for lp:~mattyw/juju-core/add-service-owner-to-status

Revision history for this message
William Reade (fwereade) wrote :

On 2014/02/27 13:45:51, fwereade wrote:
> On 2014/02/27 10:40:25, dimitern wrote:
> > On 2014/02/25 10:15:52, dimitern wrote:
> > > Looks good, but make sure you're familiar with Martin's recent
1.16
> > > compatibility for status changes:
https://codereview.appspot.com/66590043/
> > >
> > > You'll need to use the FullStatus API call.
> >
> > Sorry, so your changes are in statecmd, which gets called internally
in
> > apiserver for both FullStatus and Status.
> > This means you don't need to care about FullStatus in your CL.
> > Your changes LGTM assuming you've tested them live as well, just in
case.

> I'm fairly strongly -1 on this, because status is bloated enough
already. Can we
> please start adding flags to status for additional information?

And especially in status, adding the owner to every unit feels wrong. If
units ever get explicit owners we can add it then; for now, the service
feels like a much better place. So not lgtm without discussion, at
least.

https://codereview.appspot.com/67870043/

« Back to merge proposal