Moti Asayag has posted comments on this change. Change subject: core: Allow decreasing a cluster compatability version ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/26940/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java: Line 172: .getcompatibility_version())) { Line 173: addCanDoActionMessage(VersionSupport.getUnsupportedVersionMessage()); Line 174: result = false; Line 175: } Line 176: sorry for commenting about a generic issue with this method, but still... Dragging the result variable through the entire method doesn't seem like a good practice - we can quit early in the method if the validation fails. Changing the entire method is probably out of the scope of this patch, but changing it will ease the maintenance of this method. Line 177: if (result) { Line 178: allForVdsGroup = getVdsDAO().getAllForVdsGroup(oldGroup.getId()); Line 179: } Line 180: // decreasing of compatibility version is only allowed when no hosts exists, and not beneath the DC version Line 177: if (result) { Line 178: allForVdsGroup = getVdsDAO().getAllForVdsGroup(oldGroup.getId()); Line 179: } Line 180: // decreasing of compatibility version is only allowed when no hosts exists, and not beneath the DC version Line 181: if (result && getVdsGroup().getcompatibility_version().compareTo(oldGroup.getcompatibility_version()) < 0) { please separate into 2 cases (messages) so the user will know why this action isn't supported: either due to DC version restriction or due to the fact the cluster has hosts in it. Line 182: if ((allForVdsGroup != null && allForVdsGroup.size() > 0) Line 183: || (oldGroup.getStoragePoolId() != null && getVdsGroup().getcompatibility_version() Line 184: .compareTo(getStoragePoolDAO().get(oldGroup.getStoragePoolId()).getcompatibility_version()) < 0)) { Line 185: result = false; Line 178: allForVdsGroup = getVdsDAO().getAllForVdsGroup(oldGroup.getId()); Line 179: } Line 180: // decreasing of compatibility version is only allowed when no hosts exists, and not beneath the DC version Line 181: if (result && getVdsGroup().getcompatibility_version().compareTo(oldGroup.getcompatibility_version()) < 0) { Line 182: if ((allForVdsGroup != null && allForVdsGroup.size() > 0) allForVdsGroup cannot be null. It can only be empty/not empty, so this should be replaced with if (!allForVdsGroup.isEmpty() || ...) { ... } Line 183: || (oldGroup.getStoragePoolId() != null && getVdsGroup().getcompatibility_version() Line 184: .compareTo(getStoragePoolDAO().get(oldGroup.getStoragePoolId()).getcompatibility_version()) < 0)) { Line 185: result = false; Line 186: getReturnValue() http://gerrit.ovirt.org/#/c/26940/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStoragePoolCommandTest.java: Line 3: import static org.junit.Assert.assertFalse; Line 4: import static org.junit.Assert.assertTrue; Line 5: import static org.mockito.Matchers.any; Line 6: import static org.mockito.Matchers.anyBoolean; Line 7: import static org.mockito.Matchers.anyString; please remove this file from the patch. Line 8: import static org.mockito.Mockito.doReturn; Line 9: import static org.mockito.Mockito.spy; Line 10: import static org.mockito.Mockito.when; Line 11: -- To view, visit http://gerrit.ovirt.org/26940 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2a8f52c63a82871e4ac1847f0414c82095f43075 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Eli Mesika <elimes...@gmail.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
