Code review comment for lp:~menno.smits/juju-core/1194481-relation_name_in_status.0
Revision history for this message
| Menno Finlay-Smits (menno.smits) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
Replies to latest review comments in-line.
https:/
File state/api/client.go (right):
https:/
state/api/
> 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]
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:/
File state/apiserver
https:/
state/apiserver
params.
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-
also-relevant-
I haven't been making a distinction - these changes currently let the
StatusData-
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:/