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

As discussed on IRC, I think the code could be refactored to reduce duplication, for example as in https://pastebin.canonical.com/107195/.
Tests could then mock _run_command rather than subprocess.check_output.

It should be even possible to perform the line.split("\n") in the _run_command method, returning a list of lists (so the parse_* methods don't need to perform it), I'm not sure if it fits all the use cases, though.

#1:
+ data["device"] = ""
+ data["volume_label"] = ""
+ data["instance_id"] = ""
+ data["tags"] = {"volume_name": ""}

These can be set with a single data.update()

#2:
+ volume_data.update({"instance_id": data["instance_id"]})
+ volume_data.update({"device": data["device"]})

Same as #1

#3:
+ for tag_volume_id in volume_tag_data.keys():
+ additional_tags = volume_tag_data[tag_volume_id]["tags"]
+ if tag_volume_id not in result:
+ hookenv.log(
+ "Ignoring tags for volume-id %s that doesn't exist" %
+ tag_volume_id)
             else:

The first line inside the loop can be moved inside the else clause.
Also there's an extra indentation space on the hookenv.log() call

#4:
I think you can drop the checks on the response_type in the parse_* methods, since it's already checked by call sites (related tests can be dropped then).