https://codereview.appspot.com/93480044/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/api/client.go#newcode114
state/api/client.go:114: Relations       map[string]RelationStatus
On 2014/06/03 11:57:09, menn0 wrote:
> > Hmm... yeah, I think it probably would. I also find myself wondering
whether
> we
> > should actually be representing the endpoint data as well, lest
clients have
> to
> > mess around parsing keys? Don't let this block you, it's addressable
in a
> > followup, but take a quick look at the AddRelation API: what we
chose there
> > *may* have bearing on what we should expose here.

> Funnily enough, my initial implementation did include endpoint data in
> RelationStatus but I removed it because the data was mostly repeating
what was
> already in the relation name. I'm happy to put this back in though.

> Looking at the AddRelation API, it returns a map[string]charm.Relation
where the
> key is the service name. I'm not sure that it makes sense for
RelationStatus to
> represent endpoints in a map - a slice probably makes more sense.

> How about each RelationStatus holds a slice of EndpointStatus structs
which have
> a subset of what AddRelation returns? The EndpointStatus fields would
be
> something like:

>    ServiceName string
>    Interface   string
>    Role        charm.RelationRole (string)

> How does that sound?


SGTM, but let's also record the relation scope (it should match across
the endpoints, so I think it's relation-level status).

Also, please see if you can think of a nice way to show which (if any)
is the subordinate in a given relation.(It's potentially subtle: if foo
is subordinate to bar via some locally-scoped relation, it's only
subordinate in the context of that relation: it can have a perfectly
good global relation with some other service.) This information is
encoded elsewhere in status output, though, so it's optional to some
degree; but it might be a cleaner way of doing some things, so bear it
in mind.

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/93480044/diff/20001/state/apiserver/client/api_test.go#newcode211
state/apiserver/client/api_test.go:211: AgentStateData:
params.StatusData{"foo": "bar"},
On 2014/06/03 11:57:09, menn0 wrote:
> On 2014/05/28 08:52:38, fwereade wrote:
> > On 2014/05/26 21:47:43, menn0 wrote:
> > > On 2014/05/26 06:45:34, fwereade wrote:
> > > > Were we going to add the components of the "(error: blam)" stuff
into
> > > StatusData
> > > > (or something similar)?
> > >
> > > Yes that's coming (in a separate branch). This branch is just
about getting
> > the
> > > supporting infrastructure in place.
> >
> > Great, sorry for the confusion, I fear you will have to get used to
me asking
> > stupid questions to which I should already know the answers :).
> >
> > > The test data here isn't realistic. This test is just checking
that
> StatusData
> > > is making it where it should the status response.
> > >
> > > > The contents of StatusData are technically arbitrary, but
they're actually
> > > > pretty well-defined. We could tighten them up to prevent key
collisions
> with
> > > > whatever we pick; or we could put the arbitrary data into its
own dict?
> > >
> > > Agreed that we should be careful about what we put in StatusData.
How about
> > > namespacing the keys along the lines of "category.item" to help
avoid
> > > collisions?
> >
> > Depends what we mean by StatusData -- the stuff that gets set by the
agent, or
> > the stuff we return over status? For the latter, I think it may be
better to
> > separate the explicitly-set-by-agent bits from the
also-relevant-to-status
> bits.

> I haven't been making a distinction - these changes currently let the
> StatusData-as-set-by-the-agent straight out - but I guess we probably
should to
> ensure that internal-only data doesn't leak out. I don't see many
places in the
> code that currently make use of StatusData. I'll whitelist the allowed
keys or
> something.

SGTM. We can loosen it up at that end as we need to; and if we make a
point of only *taking* specific keys at this end, those changes won't
pollute this side of the code (ie ninja-change the API) until someone
explicitly includes them in status output.

https://codereview.appspot.com/93480044/