cloudstack-pull-rats #696 SUCCESS
This pull request looks good

cloudstack-pull-analysis #646 SUCCESS
This pull request looks good

@desc: Test that Volume snapshot for root volume not allowed ... === TestName: test_01_test_vm_volume_snapshot | Status : SUCCESS ===
ok

Ran 1 test in 165.417s

OK

LGTM!!
Allow vm snapshot and volume snapshot coexist ,which was not supported earlier .
it is just one line modification removing self.assertRaises(Exception) ,so not much to test here

@desc: Test that Volume snapshot for root volume not allowed ... === TestName: test_01_test_vm_volume_snapshot | Status : SUCCESS ===
ok

Sanju1010 , is the above description part of script ? The description looks misleading with the current functionality. If it is not part of the code then LGTM !! , else please modify the description so that it wont be confusing.

Sanjeev N on dev@cloudstack.apache.org replies:
@pavanb018 good catch !! It is not part of the code but will change.

cloudstack-pull-rats #703 SUCCESS
This pull request looks good

cloudstack-pull-analysis #653 SUCCESS
This pull request looks good

LGTM !!

@pavanb018 @nitt10prashant Could you please add what you tested next time? A LGTM without any explanation doesn't really tell me anything to be honest.

An LGTM is given after going through the code and understanding what the code is accomplishing. The PR already has the test results and if i am not wrong an LGTM is for reviewing the code to make sure the functionality is working as expected ?

@remibergsma Next time sure I will add some details .for this PR i have updated my comments .

@pavanb018 it always helps other folks reviewing to know what you did. For example.
Did you just check the Travis green light ? Did you just check the code through github ? Did you pull the patch, apply and compile ? Did you do the build then run the tests. It may sound obvious to you what you did by giving the copy paste form a test, but writing a small friendly sentence for the next reviewer will go a long way to help us merge those fixes as a group.

thanks

Hi @sanju1010,

@serg38 and I are working on this PR #1677 which solves vm snapshot creation issue. After running test_vm_snapshots.py we're getting exception on test_01_test_vm_volume_snapshot as it was expected before your changes in this PR.

We're getting: 'CloudstackAPIException: Execute cmd: createsnapshot failed, due to: errorCode: 431, errorText:Volume snapshot is not allowed, please detach it from VM with VM Snapshots\n']

This is consistent with VolumeApiServiceImpl.allocSnapshot method, in which it is:

        if (volume.getInstanceId() != null) {
            // Check that Vm to which this volume is attached does not have VM Snapshots
            if (_vmSnapshotDao.findByVm(volume.getInstanceId()).size() > 0) {
                throw new InvalidParameterValueException("Volume snapshot is not allowed, please detach it from VM with VM Snapshots");
            }
        }

Do you want us to adapt test_vm_snapshots.py in mentioned PR to support this or would you like to address it yourself?

Thanks

@jburwell @rhtyd @karuturi @nvazquez I assume the current code doesn’t allow volume snapshots on the top of vmsnapshots on purpose. In that case is it best to revert PR871 or create a new PR to address the Marvin script?

From: Nicolas Vazquez notifications@github.com
Reply-To: apache/cloudstack reply@reply.github.com
Date: Wednesday, September 21, 2016 at 11:15 AM
To: apache/cloudstack cloudstack@noreply.github.com
Cc: Sergey Levitskiy Sergey.Levitskiy@autodesk.com, Mention mention@noreply.github.com
Subject: Re: [apache/cloudstack] CLOUDSTACK-8893: Fixing script as per the latest functionality (#871)

Hi @sanju1010https://github.com/sanju1010,

@serg38https://github.com/serg38 and I are working on this PR #1677#1677 which solves vm snapshot creation issue. After running test_vm_snapshots.py we're getting exception on test_01_test_vm_volume_snapshot as it was expected before your changes in this PR.

We're getting: 'CloudstackAPIException: Execute cmd: createsnapshot failed, due to: errorCode: 431, errorText:Volume snapshot is not allowed, please detach it from VM with VM Snapshots\n']

This is consistent with VolumeApiServiceImpl.allocSnapshot method, in which it is:

    if (volume.getInstanceId() != null) {

        // Check that Vm to which this volume is attached does not have VM Snapshots

        if (_vmSnapshotDao.findByVm(volume.getInstanceId()).size() > 0) {

            throw new InvalidParameterValueException("Volume snapshot is not allowed, please detach it from VM with VM Snapshots");

        }

    }

Do you want us to adapt test_vm_snapshots.py in mentioned PR to support this or would you like to address it yourself?

Thanks

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/871#issuecomment-248696674, or mute the threadhttps://github.com/notifications/unsubscribe-auth/APbWPq2NlxyRQADHpD6ucL5jzTkZ_Zy1ks5qsXQpgaJpZM4GBeLo.