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

Thanks for digging through this Fernando
[0] 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.

Agreed, this got way too large for a simple review. I should have decomposed this branch into at least 2 parts when it took folks greater than a week to grab the review in the 1st place and a couple weeks to get through it. Those were indicators that maybe this could be made easier.

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

Added a validation in class __init__ and log a hook error exit(1) if provider is unsupported. Thanks
> [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

[2-3] Done and Done
> +
> [4]
> + def _ec2_describe_volumes(self, volume_id=None):
> + import euca2ools.commands.euca.describevolumes as describevolumes
Added docstring

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

The reason I left PROVIDER_COMMANDS in place is because there was no difference in how the command was run or how the output was handled so this avoided additional methods. The original reason I broke our the _ec2_* versus _nova_* methods was only in cases where the implementation of the methods differed significantly either due to library or command differences. We do get coverage on both of these commands because we testing them in TestEC2Util versus TestNovaUtil classes. But, if you think we should do this for conformity's sake, I agree with you that it should probably be a separate branch.