Merge lp://qastaging/~jason-hobbs/charms/trusty/cinder/remove-volume-group into lp://qastaging/~openstack-charmers-archive/charms/trusty/cinder/next

charm_lint_check #6567 cinder-next for jason-hobbs mp265461
    LINT FAIL: lint-test failed

LINT Results (max last 2 lines):
make: *** [lint] Error 1
ERROR:root:Make target returned non-zero.

Full lint test output: http://paste.ubuntu.com/11916989/
Build: http://10.245.162.77:8080/job/charm_lint_check/6567/

charm_unit_test #6199 cinder-next for jason-hobbs mp265461
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6199/

charm_amulet_test #5244 cinder-next for jason-hobbs mp265461
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/11917122/
Build: http://10.245.162.77:8080/job/charm_amulet_test/5244/

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.

charm_lint_check #6625 cinder-next for jason-hobbs mp265461
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/6625/

charm_unit_test #6252 cinder-next for jason-hobbs mp265461
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6252/

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

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

Fair enough, I'm convinced.
>
> Jason

Just waiting on the amulet results to post before landing this.

charm_amulet_test #5254 cinder-next for jason-hobbs mp265461
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/5254/

Can this be landed now?