Maor Lipchuk has posted comments on this change. Change subject: core: Block removing network when it's related to iSCSI bond. ......................................................................
Patch Set 3: (10 comments) http://gerrit.ovirt.org/#/c/30913/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkValidator.java: Line 143: getNetworkNameReplacement()) Line 144: : ValidationResult.VALID; Line 145: } Line 146: Line 147: public ValidationResult notISCSIBondNetwork() { > s/notISCSIBondNetwork/notIscsciBondNetwork done Line 148: List<IscsiBond> iscsiBonds = getDbFacade().getIscsiBondDao().getIscsiBondsByNetworkId(network.getId()); Line 149: if (!iscsiBonds.isEmpty()) { Line 150: List<String> bondNames = new ArrayList<>(); Line 151: for (IscsiBond iscsiBond : iscsiBonds) { Line 146: Line 147: public ValidationResult notISCSIBondNetwork() { Line 148: List<IscsiBond> iscsiBonds = getDbFacade().getIscsiBondDao().getIscsiBondsByNetworkId(network.getId()); Line 149: if (!iscsiBonds.isEmpty()) { Line 150: List<String> bondNames = new ArrayList<>(); > Please use ReplacementUtils.replaceWithNameable() instead of this loop. nice!! done Line 151: for (IscsiBond iscsiBond : iscsiBonds) { Line 152: bondNames.add(iscsiBond.getName()); Line 153: } Line 154: return new ValidationResult(VdcBllMessages.NETWORK_CANNOT_REMOVE_ISCSI_BOND_NETWORK, Line 149: if (!iscsiBonds.isEmpty()) { Line 150: List<String> bondNames = new ArrayList<>(); Line 151: for (IscsiBond iscsiBond : iscsiBonds) { Line 152: bondNames.add(iscsiBond.getName()); Line 153: } > please add a space line empty lines does not have any meaning here, it also make the code much longer and un-readable. An empty line should only be before a comment, and also not at the end of a block, since the Brackets already indicates it is the end of the scope. Line 154: return new ValidationResult(VdcBllMessages.NETWORK_CANNOT_REMOVE_ISCSI_BOND_NETWORK, Line 155: getNetworkNameReplacement(), Line 156: getIscsiBondsReplacement(bondNames)); Line 157: } Line 153: } Line 154: return new ValidationResult(VdcBllMessages.NETWORK_CANNOT_REMOVE_ISCSI_BOND_NETWORK, Line 155: getNetworkNameReplacement(), Line 156: getIscsiBondsReplacement(bondNames)); Line 157: } > please add a space line see comment above Line 158: return ValidationResult.VALID; Line 159: } Line 160: Line 161: private String getNetworkNameReplacement() { Line 161: private String getNetworkNameReplacement() { Line 162: return String.format("$NetworkName %s", network.getName()); Line 163: } Line 164: Line 165: private String getIscsiBondsReplacement(List iscsiBonds) { > please add type to List removed with ReplacementUtils.replaceWithNameable() Line 166: return String.format("$IscsiBonds %s", iscsiBonds.toString()); Line 167: } Line 168: Line 169: protected ValidationResult networkNotUsed(List<? extends Nameable> entities, VdcBllMessages entitiesReplacement) { Line 162: return String.format("$NetworkName %s", network.getName()); Line 163: } Line 164: Line 165: private String getIscsiBondsReplacement(List iscsiBonds) { Line 166: return String.format("$IscsiBonds %s", iscsiBonds.toString()); > please avoid relying on List.toString() for formatting the text and use: removed with ReplacementUtils.replaceWithNameable() Line 167: } Line 168: Line 169: protected ValidationResult networkNotUsed(List<? extends Nameable> entities, VdcBllMessages entitiesReplacement) { Line 170: if (entities.isEmpty()) { http://gerrit.ovirt.org/#/c/30913/3/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 610: TAGS_SPECIFIED_TAG_CANNOT_BE_THE_PARENT_OF_ITSELF=Operation canceled, recursive Tag hierarchy cannot be defined. Line 611: VM_CANNOT_MOVE_TO_CLUSTER_IN_OTHER_STORAGE_POOL=VM can be moved only to a Cluster in the same Data Center. Line 612: VM_CLUSTER_IS_NOT_VALID=Cannot ${action} ${type}. Cluster ID is not valid. Line 613: NETWORK_CANNOT_REMOVE_MANAGEMENT_NETWORK=The Management Network ('${NetworkName}') is mandatory and cannot be removed. Line 614: NETWORK_CANNOT_REMOVE_ISCSI_BOND_NETWORK=The Network ('${NetworkName}') could not be removed since it is part of the following iSCSI bonds:\n ${IscsiBonds}.\nPlease, remove the network first from those iSCSI bonds, and try again. > Is the comma after "Please" needed ? I think it can work either way, but I don't mind removing it. Line 615: NETWORK_OLD_NETWORK_NOT_SPECIFIED=Previous network name is required. Line 616: ACTION_TYPE_FAILED_DETECTED_ACTIVE_VMS=Cannot ${action} ${type}. Active VMs were detected.\n\ Line 617: -Please ensure all VMs associated with this Storage Domain are stopped and in the Down state first. Line 618: ACTION_TYPE_FAILED_HOST_NOT_EXIST=The provided Host does not exist. http://gerrit.ovirt.org/#/c/30913/3/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/IscsiBondDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/IscsiBondDaoTest.java: Line 118: Line 119: @Test Line 120: public void testGetEmptyIscsiBondIdByNetworkId() { Line 121: List<IscsiBond> fetchedIscsiBonds = dao.getIscsiBondsByNetworkId(networkId); Line 122: assertEquals(0, fetchedIscsiBonds.size()); > assertTrue(fetchedIscsiBonds.isEmpty()) ? I think those are quite the same, although I prefer to keep the assertEquals since the default error message of this, should be much more informative, and indicate the size of the list on failure, while assertTrue will only indicate that the list is not empty. Line 123: } Line 124: Line 125: @Test Line 126: public void testGetEmptyIscsiBondIdByNotExistingNetworkId() { Line 124: Line 125: @Test Line 126: public void testGetEmptyIscsiBondIdByNotExistingNetworkId() { Line 127: List<IscsiBond> fetchedIscsiBonds = dao.getIscsiBondsByNetworkId(Guid.Empty); Line 128: assertEquals(0, fetchedIscsiBonds.size()); > same see comment above Line 129: } Line 130: Line 131: @Test Line 132: public void testRemoveNetworkFromIscsiBond() { http://gerrit.ovirt.org/#/c/30913/3/frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties File frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties: Line 580: TAGS_SPECIFIED_TAG_CANNOT_BE_THE_PARENT_OF_ITSELF=Operation canceled, recursive Tag hierarchy cannot be defined. Line 581: VM_CANNOT_MOVE_TO_CLUSTER_IN_OTHER_STORAGE_POOL=VM can be moved only to a Cluster in the same Data Center. Line 582: VM_CLUSTER_IS_NOT_VALID=Cannot ${action} ${type}. Cluster ID is not valid. Line 583: NETWORK_CANNOT_REMOVE_MANAGEMENT_NETWORK=The Management Network ('${NetworkName}') is mandatory and cannot be removed. Line 584: NETWORK_CANNOT_REMOVE_ISCSI_BOND_NETWORK=The Network ('${NetworkName}') could not be removed since it is part of the following iSCSI bonds:\n ${IscsiBonds}.\nPlease, remove the network first from those iSCSI bonds, and try again. > this change is not needed. network actions are supported only from the weba I agree, but I think it is better to keep that for consistency and flexibility reasons. Line 585: NETWORK_OLD_NETWORK_NOT_SPECIFIED=Previous network name is required. Line 586: ACTION_TYPE_FAILED_DETECTED_ACTIVE_VMS=Cannot ${action} ${type}. Active VMs were detected.\n\ Line 587: -Please ensure all VMs associated with this Storage Domain are stopped and in the Down state first. Line 588: ACTION_TYPE_FAILED_HOST_NOT_EXIST=The provided Host does not exist. -- To view, visit http://gerrit.ovirt.org/30913 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I68bd390175d42ee4e33773e12d6184a97a755eed Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
