Replies to latest review comments in-line.


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
> 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?

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/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.

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