Code review comment for lp:~menno.smits/juju-core/1194481-relation_name_in_status.0
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
https:/
File state/api/client.go (right):
https:/
state/api/
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]
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:/
File state/apiserver
https:/
state/apiserver
params.
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-
also-relevant-
> bits.
> 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.
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:/