Code review comment for lp:~themue/pyjuju/go-state-add-relation
Revision history for this message
| Gustavo Niemeyer (niemeyer) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
67d34a1
(Get the code!)
LGTM, but see the comments please:
https:/
File state/state.go (right):
https:/
state/state.go:276: if scope == ScopeGlobal {
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > This seems weird. Why is it only introducing the settings if the
relation is
> > global? I don't know if that makes sense, and if it does we
certainly need a
> > clear comment here explaining the background.
> I tried to make the comments more clear, they have been almost a 1:1
copy of the
> Python code. We should talk about the container scope and if the whole
method(s)
> can't be modified to handle this too. So far I don't know enough about
the
> meaning of the scope and have to analyze it first.
Okay, this branch sounds fine besides what we already talked about and
the minor points, but please do understand the scope logic. As you have
seen in the relation+topology story, writing logic when you have no idea
of what is going on is hard.
https:/
state/state.go:334: relationKey, err := s.addRelation(
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > Have we verified that the endpoints make sense at this point? It
looks like
> > we've checked that there is a good number of them, but I can't see
it
> verifying
> > that the endpoints make sense (service names, relation names,
interface names,
> > etc). It'd be bad to be creating state in ZooKeeper just to later
tell
> something
> > trivial to the user that we could have verified upfront. We
shouldn't be
> > duplicating this logic, though.
> Today this isn't done. I would like to do it in a new small branch
after this
> and RemoveRelation() are in.
I filed a bug to keep track of this and assigned to you:
https:/
https:/
state/state.go:369: Service: serviceRelation
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > serviceRelation has a public API that we can use it here, rather
than fiddling
> > with its internals assuming we know what's going on.
> >
> > Same below.
> The name can be retrieved with RelationName() but the service key is
internal so
> far. Shall a public accessor be added?
No, you're right. serviceKey is internal.
https:/
File state/state.go (right):
https:/
state/state.go:263: // container occurs in ServiceRelation
That's a lot nicer, thanks.
Would you mind to just drop the () as we tend to refer to methods by
their name without the call syntax.
https:/
state/state.go:279: if scope == ScopeGlobal {
That's a bit strange. This method only does anything at all if the scope
is not global, and the scope is only provided so it can be checked here
for not doing anything at all.
Instead, we can drop the test here, drop the parameter, and check at the
scope at the call site. That makes both this function and the call site
more clear.
https:/
state/state.go:294: return nil, nil, fmt.Errorf("state: can't add
non-peer relation with a single service")
Can you please drop all the "state:" prefixes being _added_ in this
branch. After much debate, we'll start dropping those prefixes and
trusting more on the message itself. Among other reasons, the prefix
looks really bad once they're compounded ("pkg1: foo bar baz: pkg2: blah
blah: pkg4: etc").
Don't worry about previously existing prefixes. We'll cover these by
themselves in an isolated branch.
https:/