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 :

Replies inline.

Given that there is another branch in the works which builds on this
one, adding the extra items to StatusData when required and actually
using them in the client, what's still needed to get this branch ready
for landing?

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/05/26 06:45:34, fwereade wrote:
> so we're turning int ids into strings to satisfy json? fair enough, I
guess, but
> I have a bit of the same old "eww" reaction.

I discovered the hard way what happens if you try to serialise map[int]
over the API: seemingly hung tests (perhaps due to API retries?)

I didn't like converting the relation ids to strings much either. I was
considering making Relations a []RelationStatus instead and including Id
in RelationStatus. That way the client may end up generating a map
itself which is bit crappy but hey. Would that be preferable?

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

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?

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

« Back to merge proposal