Code review comment for lp:~jameinel/maas/cpu_mem_tag_constraints

As mentioned on IRC, the error message in InvalidConstraint seems misleading. As far as I can see, "Invalid 'X' constraint 'Y'" can mean that Y is not an acceptable value for constraint X. So far, so good. But it can also mean that X is not a known constraint in the first place! I would suggest either splitting these two meanings into separate exception classes, or differentiating the error message so that it will contain the constraint value only if relevant. (Presumably it wouldn't need to be a separate attribute on the exception class).

The AcquisitionConstrainer looks like it wants to be a module, not a class. Its self.nodes looks like it wants to be a parameter and return value to the various filter functions, rather than implicit mutable state. And looking up a computed method name can be a handy shorthand, but if you're duplicating the list of accepted constraints here anyway (you also had it in the constraints map), then you might as well make the lookup in an explicit dict:

def constrain_identical(nodes, key, value):
    """Generic constraint: filter nodes by exact value match."""
    return nodes.filter(key=value)

def constrain_int_gte(nodes, key, value):
    """Integer constraint: greater than or equal to `value`."""
    try:
        int_value = int(value)
    except ValueError as e:
        raise InvalidConstraint(key, value, e)
    return nodes.filter(**{"%s__gte" % key: value})

def constrain_tags(nodes, key, tag_expression):
    """Tags match: restrict to nodes that have all of given tags list."""
    # We support both comma-separated and space-separated tag lists.
    tag_names = tag_expression.replace(",", " ").strip().split()
    for tag_name in tag_names:
        tag = get_one(Tag.objects.filter(name=tag_name))
        if tag is None:
            raise InvalidConstraint('tags', tag_name, "No such tag.")
        nodes = nodes.filter(tags=tag)
    return nodes

# How to filter each individual constraint.
constraint_filters = {
    # This only supports an exact match on arch type. Using an i386 image
    # on amd64 hardware will wait.
    'architecture': constrain_identical,
    'name': constrain_identical,
    'cpu_count': constrain_int_gte,
    'memory': constrain_int_gte,
    'tags': constrain_tags,
}

def constrain_nodes(nodes, constraints):
    for constraint, value in constraints.items():
        filter = constraint_filters.get(constraint)
        if filter is not None:
            nodes = filter(nodes, constraint, value)
    return nodes

I think that's easier to follow, and despite having more documentation it's fewer lines as well.

(By the way, does an equality comparison on a tag value really do an "is in" comparison!? The tests verify it, but )