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

Some comments:

[1]

+class ServiceUnitRelationResolvedAlreadyEnabled(StateError):
+ """The unit has already been marked resolved.

Documentation needs updating.

[2]

+ def get_resolved(self):
+ """Get the value of the resolved flag if any.

The result of this method isn't a flag.

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

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

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

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

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

[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):

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