Code review comment for lp:~tpatil/nova/os-security-groups

In '_format_security_group_rule()' and '_format_security_group()', you should avoid the use of single-character variable names. I can sort of guess what they represent, but it's better to use descriptive names.

There are several 'standards' for the indentation of continued lines, but right-alignment is not one of them. It does nothing for either consistency of indentation or readability. The preferred method is to indent two level (i.e., 8 spaces) on a continued line; you can also indent more than that if you wish to align the text with something on the line above.

One specific case is line 322: it's a continuation line, but is only indented one level, making it the same indentation level as the block that follows. While the Python interpreter can make sense of this, it is visually inaccurate to humans reading the code.

Lines 642-6 actually outdent continued lines. That should never happen.

I'm concerned about the security of some of the methods. You check for context.is_admin in the index() method to limit the returned values, but it looks like destructive methods like 'delete()' can be invoked on any ID whether one is an admin or not, with no restriction on project. Can you explain where the ability to interact with a security group you do not have rights to is enforced?

The two methods '_validate_security_group_name()' and '_validate_security_group_description()' use the same logic and is essentially duplicated code. That logic should be refactored out into a single method that checks the value that is called by those methods. It could also be greatly simplified:

def validate_name(self, value, typ):
    # NOTE: 'typ' will be either 'name' or 'description', depending on the caller.
    try:
        val = value.strip()
    except AttributeError:
        msg = _("Security group %s is not a string or unicode") % typ
        raise exc.HTTPBadRequest(explanation=msg)
    if not val:
        msg = _("Security group %s cannot be empty.") % typ
        raise exc.HTTPBadRequest(explanation=msg)
    if len(val) > 255:
        msg = _("Security group %s should not be greater than 255 characters.") % typ
        raise exc.HTTPBadRequest(explanation=msg)

The localization is incorrect in lines 227-8. Since localized strings are extracted from the script file, not during runtime, the text to be localized will never be included. The lines should be re-written as:
msg = _("Authorize security group ingress %s")
LOG.audit(msg, security_group['name'], context=context)