Code review comment for lp://qastaging/~jason-hobbs/charms/trusty/cinder/remove-volume-group

On 07/22/2015 08:19 AM, Corey Bryant wrote:
> Hey Jason, thanks for the patch! I have a couple minor comments below. Also can you run 'make lint' and fix the results? The amulet failure above might have just been an osci hiccup.

Thanks for the review - I fixed the lint and docstring copy/paste errors
you pointed out.

>> if vg_found is False and len(new_devices) > 0:
>> + if overwrite:
>> + ensure_lvm_volume_group_non_existent(volume_group)
>
> I wouldn't block merging on this but it seems like you could get rid of ensure_lvm_volume_group_non_existent and just add it's logic to remove_lvm_volume_group.

I'd prefer to leave it how it is - I like that 'remove_lvm_volume_group'
does just one thing - it issues the vgremove command. I also think it'd
be less clear to always call "remove_lvm_volume_group" even if it's not
there to begin with.

Jason