Code review comment for lp:~chad.smith/charms/precise/block-storage-broker/bsb-ec2-support

Hey Chad, just a few points.

After looking the entire branch, I guess maybe we could have proposed this in a different way.

1 - Single branch that introduce the EC2 changes into the old util_nova.py so that we could see the diff better. Or maybe add a separate util_ec2.py which would be even cleaner. Same for tests
2 - Single branch that merged the two into a new module
3 - Single branch that introduced the mappings etc.

But it's only possible to see it when we reach this point :)

The points:

[1]
+ self.environment_map = ENVIRONMENT_MAP[provider]
+ self.commands = PROVIDER_COMMANDS[provider]
+ self.required_config_options = REQUIRED_CONFIG_OPTIONS[provider]

I'm wondering if we could make it a little bit more safer on grabbing values from those mappings.
Maybe we could have a helper function like get_value_from_mapping(mapping, value) that would return the value if it succeeds or log something useful so the user can see it in the logs. Maybe "ERROR: ce2 provider is not supported".
I'm not sure if we are doing it someplace else, if we are, disregard.

[2]
+ def validate_credentials(self):
+ """Attempt to contact nova volume service or exit(1)"""

Since we are accessing self.commands and it could be either nova or ec2, I think it should also mention ec2, or even not mention either, since it's generic.

[3]
+ def get_volume_id(self, volume_designation=None, instance_id=None):
+ """Return the ec2 volume id associated with this unit
+
+ Optionally, C{volume_designation} can be either a volume-id or
+ volume-display-name and the matching C{volume-id} will be returned.
+ If no matching volume is return C{None}.
+ """

Same as [2]

[4]
+ def _ec2_describe_volumes(self, volume_id=None):
+ import euca2ools.commands.euca.describevolumes as describevolumes

Missing docstring for that one

[5]
I see we have PROVIDER_COMMANDS and we also have provider specific methods. I guess that we could maybe lose the mapping and align with what you've done for the other methods as well. Meaning, add _ec2_validate, _ec2_detattach, _nova_validate, _nova_detattach methods. They would be testable like the others in the end and we'd be green on the coverage side of things. Feel free to refactor in a separate branch.

I'm going for a real test now and will check it later.

Thanks!