Martin Mucha has posted comments on this change. Change subject: core: validations from LabelNicCommand refactored out to HostInterfaceValidator ......................................................................
Patch Set 30: (6 comments) https://gerrit.ovirt.org/#/c/35977/30/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidator.java: Line 40: * @param nics existing host interfaces Line 41: * @return Validation result evaluated as: isBond ==> isCorrectBond. If <code>iface</code> is not a bond, validation Line 42: * is successful. Line 43: */ Line 44: public ValidationResult labeledValidBond(List<VdsNetworkInterface> nics) { > it has the same logic a validBond, just different error message. Mabe you c not that same logic. One scans through all nics, other one stops when two slaves are found. For me it's the same (I believe there won't be hundreds of nics to make the difference) So do you agree that full list of nics will be iterated over just to find that there are more than 2 slaves? Also one message takes number of all nics, while another does not. If we want to preserve this behavior and reduce code duplicity, the code become more complex. So lets start from beginning. How do you think system should behave in those 2 cases, when there are not enough of slaves? Line 45: if (!Boolean.TRUE.equals(iface.getBonded())) { Line 46: return ValidationResult.VALID; Line 47: } Line 48: https://gerrit.ovirt.org/#/c/35977/30/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/HostInterfaceValidatorTest.java: Line 131: @Test Line 132: public void testAddLabelToNicAndValidateWhenUsingInvalidLabel() throws Exception { Line 133: VdsNetworkInterface vdsNetworkInterface = createVdsNetworkInterfaceWithName(); Line 134: vdsNetworkInterface.setLabels(new HashSet<String>()); Line 135: assertThat(new HostInterfaceValidator(vdsNetworkInterface).addLabelToNicAndValidate("☠☠uups!☠☠", > Consider changing the skull to * for example. why? Skull is perfectly friendly character! ;) Just like bomb! 💣 Done. Line 136: new ArrayList<Class<?>>()), Line 137: failsWith(VdcBllMessages.IMPROPER_INTERFACE_IS_LABELED)); Line 138: } Line 139: Line 131: @Test Line 132: public void testAddLabelToNicAndValidateWhenUsingInvalidLabel() throws Exception { Line 133: VdsNetworkInterface vdsNetworkInterface = createVdsNetworkInterfaceWithName(); Line 134: vdsNetworkInterface.setLabels(new HashSet<String>()); Line 135: assertThat(new HostInterfaceValidator(vdsNetworkInterface).addLabelToNicAndValidate("☠☠uups!☠☠", > ☠ :) Done Line 136: new ArrayList<Class<?>>()), Line 137: failsWith(VdcBllMessages.IMPROPER_INTERFACE_IS_LABELED)); Line 138: } Line 139: Line 150: @Test Line 151: public void testAddLabelToNicAndValidateWhenUsingValidLabel() throws Exception { Line 152: VdsNetworkInterface vdsNetworkInterface = createVdsNetworkInterfaceWithName(); Line 153: vdsNetworkInterface.setLabels(new HashSet<String>()); Line 154: List<Class<?>> validationGroups = Arrays.<Class<?>>asList(Default.class, UpdateEntity.class, CreateEntity.class); > Does the LabelNicCommand has any validation group? I didn't find where it i replaced with emptyList (to allow specifying validation groups if validator class is reused).Done. Line 155: assertThat(new HostInterfaceValidator(vdsNetworkInterface).addLabelToNicAndValidate("ok", validationGroups), Line 156: isValid()); Line 157: } Line 158: Line 164: assertThat(validator.validBond(Collections.<VdsNetworkInterface> emptyList()), isValid()); Line 165: } Line 166: Line 167: @Test Line 168: public void testValidBondWhenInsufficientNumberOfSlaves() throws Exception { > Has the same logic as the labeledValidBond tests, please create a common me will do. Please notice following code: bond.setBonded(true); does it even make sense (~ it doesn't) to bond be not bonded? Ie. if anything, we probably should set this flag in constructor. Line 169: String bondName = "bondName"; Line 170: VdsNetworkInterface vdsNetworkInterface = createVdsNetworkInterfaceWithName(bondName); Line 171: vdsNetworkInterface.setBonded(true); Line 172: Line 183: } Line 184: } Line 185: Line 186: @Test Line 187: public void testValidBondWhenSufficientNumberOfSlaves() throws Exception { > same. Done Line 188: String bondName = "bondName"; Line 189: VdsNetworkInterface vdsNetworkInterface = createVdsNetworkInterfaceWithName(bondName); Line 190: vdsNetworkInterface.setBonded(true); Line 191: -- To view, visit https://gerrit.ovirt.org/35977 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3039fb4a92627036582f03147526522b19c1a2e5 Gerrit-PatchSet: 30 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
