Maor Lipchuk has posted comments on this change.

Change subject: core: Use allow snapshot when Adding a disk to VM.
......................................................................


Patch Set 3: (5 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 89:             ImagesHandler.setDiskAlias(getParameters().getDiskInfo(), 
getVm());
Agreed, added a condition.

After GUI will implement the allow snapshot field, this check should make more 
sence

Line 142:     private void setAllowSnapshotForDisk() {
I agree, but since the GUI does not reflect the property now, and we always get 
false here there is not much logic in the condition now.
But I think it still need to be in the can do action, since more validation 
should be added after GUI will reflect it to the user.

Line 335:     private void setVmSnapshotIdForDisk(AddImageFromScratchParameters 
parameters) {
See my previous comment.
For now this is the basic validation since GUI not reflecting it to the user, 
it should have more condition after user can pass us true or false.

Line 338:         } else {
It is Guid.Empty by default.

I prefer to initialize the value with Null since Guid.empty is more ugly IMHO.

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddImageFromScratchCommand.java
Line 49:             
mNewCreatedDiskImage.setShareable(getParameters().getDiskInfo().isShareable());
This field should be relevant when user will want to choose which disks he want 
to make non snapshotable.

--
To view, visit http://gerrit.ovirt.org/4231
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I135669c5f7bfb85fda561a47258834cd35d7688b
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to