Code review comment for lp:~hazmat/pyjuju/resolved-state-api

Excerpts from Gustavo Niemeyer's message of Tue Apr 26 18:04:27 UTC 2011:
> Review: Needs Fixing
> Some comments:
>
>
> [1]
>
> +class ServiceUnitRelationResolvedAlreadyEnabled(StateError):
> + """The unit has already been marked resolved.
>
> Documentation needs updating.

how so?

>
> [2]
>
> + def get_resolved(self):
> + """Get the value of the resolved flag if any.
>
> The result of this method isn't a flag.
>

fixed.

> [3]
>
> + def get_resolved(self):
> (...)
> + except zookeeper.NoNodeException:
> + # We get to the same end state.
> + returnValue(None)
>
> This is a bit misleading. We're just reading, so there's no "end state" to be
> achieved. The existence of the file is actually what determines if resolved
> was requested or not.
>

fixed.

> [4]
>
> + unit_resolve_path = "/units/%s/resolved" % self.internal_id
>
> Given that this was used 5 times, it would be handy to have something like:
>
> @property
> def _unit_resolved_path
> return "/units/%s/resolved" % self.internal_id
>
> Then you can use self._unit_resolved_path everywhere.
>

done.

> [5]
>
> + rel_resolved_path = "/units/%s/relation_resolved" % self._internal_id
>
> Our file paths so far are all dashed. It'd be good to keep the convention
> inside ZooKeeper too.
>
> May also be factored out into a property as per the point above.

done
>
> [6]
>
> + def set_relation_resolved(self, relation_map):
> + """Mark a unit's relations as in need of being resolved.
>
> Doesn't this actually mean something like:
>
> "Mark the problem in the unit's relation as being resolved."
>
> IOW, it's not requesting for the problem to be resolved, but stating
> that it has been.
>

It is actually making a request for the problem to be solved, not solving the
problem directly, as there are behavioral aspects to transitions that need be
done inside of the unit agent.

> [7]
>
> + :param relation_map: A map of internal relation ids, to retry booleans.
>
> This feels like a non-ideal API for a couple of reasons:
>
> a) It's exposing internal ids rather than using the relation names as we have
> elsewhere.
>
> b) set_relation_resolved({"db": True, "cache": False}) feels like marking db as
> resolved, and cache as unresolved.
>
> It's a bit hard to suggest something else without knowing exactly how you
> intend to use. Let's talk about it.
>

As per irc discussion, we've deferred a) to a latter time, there's a commit in the branch
which does switch the syntax to

set_relation_resolved(db, retry_hooks=True)

but its then inconsistent with its get_resolved api which retrieves a dictionary of
relation ids : retry hook boolean

> [8]
>
> + d1_keys = set(d1.keys())
> + d2_keys = set(d2.keys())
> +
> + must_match = d1_keys.intersection(d2_keys)
>
> As a hint, this could be spelled as:
>
> must_match = set(d1).intersection(d2):

fixed.

>
> [9]
>
> + running = workflow_state in ("up",)
>
> FWIW, this pattern feels slightly weird when there's a single value.
> workflow_state == "up" would be more straightforward.
>
> [10]
>
> + self.assertIdentical(results.pop(0).type_name, "created")
>
> assertIdentical is being overused a bit. There's no reason why these should be
> allocated in the same memory location.
>

agreed, fixed.