Alona Kaplan has posted comments on this change. Change subject: engine: update AddVdsGroupCommand ......................................................................
Patch Set 8: (50 comments) http://gerrit.ovirt.org/#/c/33416/8//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-10-08 04:15:26 +0300 Line 6: Line 7: engine: update AddVdsGroupCommand Line 8: Line 9: Update AddVdsGroupCommand with the new logic. What new logic? Please make the comment more informative. (Please note that AttachNetworkToVdsGroupCommand and UpdateNetworkNetworkOnClusterCommands were also updated). Line 10: Line 11: Change-Id: I196a583ae7d8d7e373a1aca2a48e592232b18a5b http://gerrit.ovirt.org/#/c/33416/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java: Line 46: getVdsGroup().setArchitecture(getArchitecture()); Line 47: Line 48: checkMaxMemoryOverCommitValue(); Line 49: getVdsGroup().setDetectEmulatedMachine(true); Line 50: DbFacadeLocator.getDbFacade().getVdsGroupDao().save(getVdsGroup()); Why have you done this change? Line 51: Line 52: alertIfFencingDisabled(); Line 53: Line 54: // add default network Line 68: Line 69: private void attachManagementNetwork() { Line 70: Network net = findManagementNetwork(getVdsGroup(), getManagementNetworkId()); Line 71: if (net != null) { Line 72: DbFacadeLocator.getDbFacade().getNetworkClusterDao().save(createNetworkCluster(net.getId())); Please use the standard way- DbFacade.getInstance() instead of the locator. Line 73: } Line 74: } Line 75: Line 76: private Guid getManagementNetworkId() { Line 76: private Guid getManagementNetworkId() { Line 77: return getParameters().getManagementNetworkId(); Line 78: } Line 79: Line 80: private Network findManagementNetwork(VDSGroup cluster, Guid managementNetworkId) { Why do you need parameters to this method? The method is private and can get the cluster and management network id by itself (getVdsGroup(), getManagementNetworkId()). Line 81: if (managementNetworkId == null) { Line 82: return defaultManagementNetworkFinder.findDefaultManagementNetwork(cluster.getStoragePoolId()); Line 83: } else { Line 84: return getManagementNetworkById(managementNetworkId); Line 84: return getManagementNetworkById(managementNetworkId); Line 85: } Line 86: } Line 87: Line 88: private Network getManagementNetworkById(Guid managementNetworkId) { 1. Same here- you don't need the parameter. You can get the id by using- getManagementNetworkId() 2. This method is called twice- consider storing the value. Line 89: return DbFacadeLocator.getDbFacade() Line 90: .getNetworkDao() Line 91: .get(managementNetworkId); Line 92: } Line 85: } Line 86: } Line 87: Line 88: private Network getManagementNetworkById(Guid managementNetworkId) { Line 89: return DbFacadeLocator.getDbFacade() Please use the standard way- DbFacade.getInstance() instead of the locator. Line 90: .getNetworkDao() Line 91: .get(managementNetworkId); Line 92: } Line 93: Line 201: Line 202: return result; Line 203: } Line 204: Line 205: private boolean validateManagementNetwork() { You're copying the logic of findManagementNetwork(). I suggest combining the canFoActionMessages in findManagementNetwork() and call it from the canDoAction. The result (mgmtNet) should be stored in a d.m and just the saving to the db should be done in the execution. Line 206: final Network managementNetwork; Line 207: Line 208: if (getManagementNetworkId() == null) { Line 209: managementNetwork = Line 220: } Line 221: } Line 222: Line 223: final NetworkClusterValidator networkClusterValidator = createNetworkClusterValidator(); Line 224: return validate(networkClusterValidator.networkBelongsToDataCenter(getVdsGroup(), managementNetwork)) This validation is not relevant for deafultManagementNetwork. Line 225: && validate(networkClusterValidator.managementNetworkAttachment(managementNetwork)) Line 226: && validate(networkClusterValidator.migrationPropertySupported()); Line 227: } Line 228: Line 221: } Line 222: Line 223: final NetworkClusterValidator networkClusterValidator = createNetworkClusterValidator(); Line 224: return validate(networkClusterValidator.networkBelongsToDataCenter(getVdsGroup(), managementNetwork)) Line 225: && validate(networkClusterValidator.managementNetworkAttachment(managementNetwork)) Same here. Line 226: && validate(networkClusterValidator.migrationPropertySupported()); Line 227: } Line 228: Line 229: private NetworkClusterValidator createNetworkClusterValidator() { Line 222: Line 223: final NetworkClusterValidator networkClusterValidator = createNetworkClusterValidator(); Line 224: return validate(networkClusterValidator.networkBelongsToDataCenter(getVdsGroup(), managementNetwork)) Line 225: && validate(networkClusterValidator.managementNetworkAttachment(managementNetwork)) Line 226: && validate(networkClusterValidator.migrationPropertySupported()); Why do you need this validation? Line 227: } Line 228: Line 229: private NetworkClusterValidator createNetworkClusterValidator() { Line 230: return new NetworkClusterValidator(createNetworkCluster(getManagementNetworkId()), Line 225: && validate(networkClusterValidator.managementNetworkAttachment(managementNetwork)) Line 226: && validate(networkClusterValidator.migrationPropertySupported()); Line 227: } Line 228: Line 229: private NetworkClusterValidator createNetworkClusterValidator() { Why do you need a method for this single line which is called only once? Line 230: return new NetworkClusterValidator(createNetworkCluster(getManagementNetworkId()), Line 231: getVdsGroup().getcompatibility_version()); Line 232: } Line 233: Line 230: return new NetworkClusterValidator(createNetworkCluster(getManagementNetworkId()), Line 231: getVdsGroup().getcompatibility_version()); Line 232: } Line 233: Line 234: private NetworkCluster createNetworkCluster(Guid networkId) { I think this method is redundant. You can create and store the network cluster in a d.m and- 1. pass the d.m to the validator 2. save it in NetworkClusterDao. Please notice that the cluster gets an 'id' just when it is saved to the db. It means that in canDoAction stage the cluster still doesn't have an 'id'. After saving the clsuter to the db you should update the id of the networkCluster d.m. Line 235: return new NetworkCluster(getVdsGroup().getId(), networkId, Line 236: NetworkStatus.OPERATIONAL, true, true, true, true); Line 237: } Line 238: http://gerrit.ovirt.org/#/c/33416/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/AttachNetworkToVdsGroupCommand.java: Line 35: NetworkClusterCommandBase<T> { Line 36: Line 37: public AttachNetworkToVdsGroupCommand(T parameters) { Line 38: super(parameters); Line 39: setVdsGroupId(parameters.getVdsGroupId()); You have this code in NetworkClusterCommandBase ctor, so it is redundant here. Line 40: } Line 41: Line 42: @Override Line 43: protected Network getNetwork() { Line 39: setVdsGroupId(parameters.getVdsGroupId()); Line 40: } Line 41: Line 42: @Override Line 43: protected Network getNetwork() { See the comments in- NetworkClusterCommandBase. This network should stay private. Line 44: return getParameters().getNetwork(); Line 45: } Line 46: Line 47: @Override Line 95: } Line 96: } Line 97: Line 98: @Override Line 99: protected boolean canDoAction() { This is not your bug- but since you added this validation, please add networkBelongsToDataCenter(network) validation here. Please make sure you pass the persisted network as a parameter to the validation. Line 100: return networkNotAttachedToCluster() Line 101: && vdsGroupExists() Line 102: && changesAreClusterCompatible() Line 103: && logicalNetworkExists() Line 103: && logicalNetworkExists() Line 104: && validateAttachment(); Line 105: } Line 106: Line 107: @Override I guess this change was done for- validator.managementNetworkChange(..). Since its logic should be separated fro Add and Update, this change can be reverted. Line 108: protected NetworkCluster getOldNetworkCluster() { Line 109: return null; Line 110: } Line 111: http://gerrit.ovirt.org/#/c/33416/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterCommandBase.java: Line 32: Line 33: private boolean validateExternalNetwork(NetworkClusterValidator validator) { Line 34: return validate(validator.externalNetworkSupported()) Line 35: && validate(validator.externalNetworkNotDisplay(getNetworkName())) Line 36: && validate(validator.externalNetworkNotRequired(getNetworkName())); Please add- validator.externalNetworkNotManagement Line 37: } Line 38: Line 39: protected abstract Version getClusterVersion(); Line 40: Line 39: protected abstract Version getClusterVersion(); Line 40: Line 41: protected boolean validateAttachment() { Line 42: NetworkClusterValidator validator = new NetworkClusterValidator(getNetworkCluster(), getClusterVersion()); Line 43: return validate(validator.managementNetworkAttachment(getNetwork())) 1. since managementNetworkAttachment should be splitted- please call here just validator.managementNetworkRequired (validator.externalNetworkNotManagement should be called from validateExternalNetwork). 2. managementNetworkAttachment(network) uses the 'name' and 'isExternal' properties of the network. AttachNetworkToVdsGroup.getNetwork retruns- getParameters().getNetwork() => there is a big chance that the 'name' and 'isExternal' are not initialized. You should pass the persistedNetwork or specifically the 'name' and 'isExternal' properties of the persisted network. Line 44: && validate(validator.managementNetworkChange(getOldNetworkCluster())) Line 45: && validate(validator.migrationPropertySupported()) Line 46: && (!getPersistedNetwork().isExternal() || validateExternalNetwork(validator)); Line 47: } Line 45: && validate(validator.migrationPropertySupported()) Line 46: && (!getPersistedNetwork().isExternal() || validateExternalNetwork(validator)); Line 47: } Line 48: Line 49: protected abstract NetworkCluster getOldNetworkCluster(); I guess this change was done for- validator.managementNetworkChange(..). Since its logic should be separated fro Add and Update, this change can be reverted. Line 50: Line 51: protected abstract Network getNetwork(); Line 52: Line 47: } Line 48: Line 49: protected abstract NetworkCluster getOldNetworkCluster(); Line 50: Line 51: protected abstract Network getNetwork(); See the previous comment- this method is redundant. Line 52: http://gerrit.ovirt.org/#/c/33416/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidator.java: Line 29: * the cluster to be checked against Line 30: * @param network Line 31: * network to be checked Line 32: */ Line 33: public ValidationResult networkBelongsToDataCenter(VDSGroup cluster, Network network) { 1. consider changing the method name to networkBelongsToClustersDataCenter or something similar. 2. Also consider passing more specific parameters- networkDcId, networkName, clusterDcId. Line 34: if (!cluster.getStoragePoolId().equals(network.getDataCenterId())) { Line 35: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_FROM_DIFFERENT_DC, Line 36: String.format(NETWORK_NAME_REPLACEMENT, network.getName())); Line 37: } Line 36: String Please expand the ValidationResult.failWith(..) method to get variableReplacements and then use the failWith(..).unless(..) syntax here. Line 42: * Make sure the management network attachment is valid: Line 43: * <ol> Line 44: * <li>The network must be required.</li> Line 45: * <li>Management network cannot be required</li> Line 46: * </ol> s/required/external Line 47: * Line 48: * @param network Line 49: * The attached network Line 50: * @return Error if the management network attachment is not valid. Line 48: * @param network Line 49: * The attached network Line 50: * @return Error if the management network attachment is not valid. Line 51: */ Line 52: public ValidationResult managementNetworkAttachment(Network network) { I would split it to two validations- 1. managementNetworkRequired(networkName, isRequired) 2. externalNetworkNotManagemet(networkName, isExternal) Line 53: if (networkCluster.isManagement()) { Line 54: if (!networkCluster.isRequired()) { Line 55: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_REQUIRED, Line 56: String.format(NETWORK_NAME_REPLACEMENT, network.getName())); Line 52: public ValidationResult managementNetworkAttachment(Network network) { Line 53: if (networkCluster.isManagement()) { Line 54: if (!networkCluster.isRequired()) { Line 55: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_REQUIRED, Line 56: String.format(NETWORK_NAME_REPLACEMENT, network.getName())); Please use ValidationResult.failsWith(..) api. Line 57: } Line 58: Line 59: if (network.isExternal()) { Line 60: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_EXTERNAL, Line 57: } Line 58: Line 59: if (network.isExternal()) { Line 60: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_EXTERNAL, Line 61: String.format(NETWORK_NAME_REPLACEMENT, network.getName())); same- Please use ValidationResult.failsWith(..) api. Line 62: } Line 63: } Line 64: return ValidationResult.VALID; Line 65: } Line 64: return ValidationResult.VALID; Line 65: } Line 66: Line 67: /** Line 68: * Make sure the management network change is valid - that's is allowed in an empty cluster only</li> typo- that's is Line 69: * Line 70: * @return Error if the management network change is not allowed. Line 71: */ Line 72: public ValidationResult managementNetworkChange(NetworkCluster oldNetworkCluster) { Line 68: * Make sure the management network change is valid - that's is allowed in an empty cluster only</li> Line 69: * Line 70: * @return Error if the management network change is not allowed. Line 71: */ Line 72: public ValidationResult managementNetworkChange(NetworkCluster oldNetworkCluster) { Please use the following syntax- return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED) .when(isManagementNetworkChanged(oldNetworkCluster) && isManagementNetworkChangeAllowed()); Line 73: if (isManagementNetworkChanged(oldNetworkCluster) && isManagementNetworkChangeAllowed()) { Line 74: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED); Line 75: } Line 76: return ValidationResult.VALID; Line 73: isManagementNetworkChangeAllowed Shouldn't it be !isManagementNetworkChangeAllowed()? Line 76: return ValidationResult.VALID; Line 77: } Line 78: Line 79: private boolean isManagementNetworkChangeAllowed() { Line 80: return !isClusterEmpty(); Shouldn't the method return isClusterEmpty()? Line 81: } Line 82: Line 83: private boolean isManagementNetworkChanged(NetworkCluster oldNetworkCluster) { Line 84: if (oldNetworkCluster == null) { Line 79: private boolean isManagementNetworkChangeAllowed() { Line 80: return !isClusterEmpty(); Line 81: } Line 82: Line 83: private boolean isManagementNetworkChanged(NetworkCluster oldNetworkCluster) { Seems this method has two different flows for attachNetworkToCluster (where oldNetworkCluster is null) and updateNetworkOnCluster. I would separate it. Consider adding two new classes (Add/UpdateNetworkClusterValidator) which will extend this class. Each class will implement isManagementNetworkChanged(..). Line 84: if (oldNetworkCluster == null) { Line 85: final Network currentManagementNetwork = Line 86: DbFacade.getInstance().getNetworkDao().getManagementNetwork(networkCluster.getClusterId()); Line 87: return currentManagementNetwork != null && Line 87: return currentManagementNetwork != null && Line 88: networkCluster.isManagement() Line 89: && !networkCluster.getNetworkId().equals(currentManagementNetwork.getId()); Line 90: } else { Line 91: return oldNetworkCluster.isManagement() != networkCluster.isManagement(); In case of UpdateNetworkOnCluster- changing the management property to false should be forbidden (to avoid leaving the cluster without mgmt net). Moving the mgmt property from net to net should be done by changing the new mgmt net property to true (since it is exclusive the old mgmt property will be changed to false). Line 92: } Line 93: } Line 94: Line 95: private boolean isClusterEmpty() { Line 109: Line 110: /** Line 111: * Make sure the external network attachment is supported for the version. Line 112: * Line 113: * @return Error if the external network attachment is not supported. iff=if and only if Line 114: */ Line 115: public ValidationResult externalNetworkSupported() { Line 116: return FeatureSupported.deviceCustomProperties(version) Line 117: ? ValidationResult.VALID http://gerrit.ovirt.org/#/c/33416/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkOnClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/cluster/UpdateNetworkOnClusterCommand.java: Line 41: return mgmtNetwork; Line 42: } Line 43: Line 44: @Override Line 45: protected NetworkCluster getOldNetworkCluster() { I guess this change was done for- validator.managementNetworkChange(..). Since its logic should be separated fro Add and Update, this change can be reverted. Line 46: if (oldNetworkCluster == null) { Line 47: oldNetworkCluster = getNetworkClusterDAO().get(getNetworkCluster().getId()); Line 48: } Line 49: Line 94: return validate(networkClusterAttachmentExists()) && validateAttachment(); Line 95: } Line 96: Line 97: @Override Line 98: protected Network getNetwork() { I don't think this method is necessary, it has the same functionality as getPersistentNetwork(). Line 99: if (network == null) { Line 100: network = getNetworkDAO().get(getNetworkCluster().getNetworkId()); Line 101: } Line 102: http://gerrit.ovirt.org/#/c/33416/8/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/cluster/NetworkClusterValidatorTest.java: Line 77: @Before Line 78: public void setup() { Line 79: when(version.getValue()).thenReturn(CLUSTER_VERSION); Line 80: when(network.getName()).thenReturn(NETWORK_NAME); Line 81: when(networkCluster.getClusterId()).thenReturn(TEST_CLUSTER_ID); I would add this line just to managementNetworkChange(..) tests. It is the only validation that actually needs the clusterId. Notice that for example networkBelongsToDataCenter and managementNetworkAttachment are called from AddVdsGroupCommand with 'clusterId=null'. Line 82: when(networkCluster.getNetworkId()).thenReturn(TEST_NETWORK_ID); Line 83: when(network.getId()).thenReturn(TEST_NETWORK_ID); Line 84: when(currentManagementNetwork.getId()).thenReturn(TEST_CURRENT_MANAGMENT_NETWORK_ID); Line 85: when(dbFacade.getVdsDao()).thenReturn(vdsDao); Line 78: public void setup() { Line 79: when(version.getValue()).thenReturn(CLUSTER_VERSION); Line 80: when(network.getName()).thenReturn(NETWORK_NAME); Line 81: when(networkCluster.getClusterId()).thenReturn(TEST_CLUSTER_ID); Line 82: when(networkCluster.getNetworkId()).thenReturn(TEST_NETWORK_ID); To avoid problems as I wrote in the previous comment I would init the expectations separately for each set of validation tests. Each set should init just the expectations it really needs. Line 83: when(network.getId()).thenReturn(TEST_NETWORK_ID); Line 84: when(currentManagementNetwork.getId()).thenReturn(TEST_CURRENT_MANAGMENT_NETWORK_ID); Line 85: when(dbFacade.getVdsDao()).thenReturn(vdsDao); Line 86: when(dbFacade.getNetworkDao()).thenReturn(networkDao); Line 83: when(network.getId()).thenReturn(TEST_NETWORK_ID); Line 84: when(currentManagementNetwork.getId()).thenReturn(TEST_CURRENT_MANAGMENT_NETWORK_ID); Line 85: when(dbFacade.getVdsDao()).thenReturn(vdsDao); Line 86: when(dbFacade.getNetworkDao()).thenReturn(networkDao); Line 87: DbFacadeLocator.setDbFacade(dbFacade); As I wrote in previous patches, please avoid you DbFacadeLocator.setDbFacade(..) it was written as some kind of hack. You can whether pass the networkDao and vdDao to NetworkClusterValidator via injection :). Or add getters for them and spy the validator. Line 88: Line 89: validator = new NetworkClusterValidator(networkCluster, version); Line 90: } Line 91: Line 105: assertThat(validator.managementNetworkAttachment(network), isValid()); Line 106: } Line 107: Line 108: @Test Line 109: public void managementNetworkAttachmentInvalidNotReqired() { typo- required Line 110: when(networkCluster.isManagement()).thenReturn(true); Line 111: when(networkCluster.isRequired()).thenReturn(Boolean.FALSE); Line 112: Line 113: assertThat(validator.managementNetworkAttachment(network), Line 136: testManagementNetworkChange(Boolean.FALSE, Boolean.FALSE, false, isValid()); Line 137: } Line 138: Line 139: @Test Line 140: public void managementNetworkChangeValidNoChangeAfterAttachment1() { 1. Please find more informative name then 1:) Same for the other methods with numbers. 2. What do you mean by saying after attachment? I guess on add. But since the validation of management network change should split between add and update I guess you"ll rewrite and rename this methods anyway. Line 141: testManagementNetworkChangeAfterAttachement(null, Boolean.FALSE, false, network, isValid()); Line 142: } Line 143: Line 144: @Test Line 137: } Line 138: Line 139: @Test Line 140: public void managementNetworkChangeValidNoChangeAfterAttachment1() { Line 141: testManagementNetworkChangeAfterAttachement(null, Boolean.FALSE, false, network, isValid()); This test should fail- you shouldn't be able to change the mgmt network to non-mgmt on empty or non-empty cluster. (Choosing another network as mgmt will do it automatically). Line 142: } Line 143: Line 144: @Test Line 145: public void managementNetworkChangeValidNoChangeAfterAttachment2() { Line 147: } Line 148: Line 149: @Test Line 150: public void managementNetworkChangeValidNoChangeAfterAttachment3() { Line 151: testManagementNetworkChangeAfterAttachement(null, Boolean.TRUE, false, null, isValid()); I"m not sure this test is needed- the validator assumes non-empty cluster has management network. In case it doesn't its behaviour is unprocurable. Line 152: } Line 153: Line 154: @Test Line 155: public void managementNetworkChangeValidNoChangeAfterAttachment4() { Line 154: @Test Line 155: public void managementNetworkChangeValidNoChangeAfterAttachment4() { Line 156: testManagementNetworkChangeAfterAttachement(null, Boolean.TRUE, true, currentManagementNetwork, isValid()); Line 157: } Line 158: Please add the following test cases- 1. testManagementNetworkChangeAfterAttachement(null, Boolean.FALSE, true, currentManagementNetwork, isValid()); 2. testManagementNetworkChangeAfterAttachement(null, Boolean.FALSE, true, network, failsWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_REMOVE_MANAGEMET_NETWORK)); 3. testManagementNetworkChangeAfterAttachement(null, Boolean.TRUE, true, network, isValid()); Line 159: @Test Line 160: public void managementNetworkChangeInnalidAfterAttachment() { Line 161: testManagementNetworkChangeAfterAttachement( Line 162: null, Line 156: testManagementNetworkChangeAfterAttachement(null, Boolean.TRUE, true, currentManagementNetwork, isValid()); Line 157: } Line 158: Line 159: @Test Line 160: public void managementNetworkChangeInnalidAfterAttachment() { 1. typo- invalid 2. This and all the attachments methods should be part of the AddNetworkClusterValidatorTest. 3. The method should be called- managementNetworkChangeInvalidNonEmptyCluster Line 161: testManagementNetworkChangeAfterAttachement( Line 162: null, Line 163: Boolean.TRUE, Line 164: false, Line 164: false, Line 165: currentManagementNetwork, Line 166: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_CANNOT_BE_CHANGED)); Line 167: } Line 168: Please add test case- testManagementNetworkChangeAfterAttachement(null, Boolean.FALSE, false, currentManagementNetwork, isValid()); Line 169: @Test Line 170: public void managementNetworkChangeValidEmptyCluster() { Line 171: testManagementNetworkChange(Boolean.FALSE, Boolean.FALSE, true, isValid()); Line 172: } Line 168: Line 169: @Test Line 170: public void managementNetworkChangeValidEmptyCluster() { Line 171: testManagementNetworkChange(Boolean.FALSE, Boolean.FALSE, true, isValid()); Line 172: } What about the other test cases with empty cluster? Please see the comment after managementNetworkChangeValidNoChangeAfterAttachment4 Line 173: Line 174: @Test Line 175: public void managementNetworkChangeInvalidNonEmptyCluster1() { Line 176: testManagementNetworkChange( Line 171: testManagementNetworkChange(Boolean.FALSE, Boolean.FALSE, true, isValid()); Line 172: } Line 173: Line 174: @Test Line 175: public void managementNetworkChangeInvalidNonEmptyCluster1() { Please find more informative name then 1:) Line 176: testManagementNetworkChange( Line 177: Boolean.TRUE, Line 178: Boolean.FALSE, Line 179: false, Line 211: } Line 212: when(networkCluster.isManagement()).thenReturn(managementAfter); Line 213: when(vdsDao.getAllForVdsGroup(TEST_CLUSTER_ID)).thenReturn(emptyCluster ? Line 214: Collections.<VDS> emptyList() : Line 215: Collections.<VDS> singletonList(null)); You can return the cluster mock instead of null. Line 216: assertThat(validator.managementNetworkChange(nc), expectedResult); Line 217: } Line 218: Line 219: @Test Line 282: Line 283: @Test Line 284: public void networkBelongsToDataCenterNotValid() throws Exception { Line 285: when(cluster.getStoragePoolId()).thenReturn(TEST_DC_ID); Line 286: when(network.getDataCenterId()).thenReturn(TEST_NETWORK_ID); Please add constant for TEST_OTHER_DC_ID, it is confusing you're using the TEST_NETWORK_ID. Line 287: Line 288: assertThat(validator.networkBelongsToDataCenter(cluster, network), Line 289: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_FROM_DIFFERENT_DC)); Line 290: } http://gerrit.ovirt.org/#/c/33416/8/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties File frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties: You should also update the user portal AppErrors file. Line 1: DIRECTORY_GROUP_CANNOT_REMOVE_DIRECTORY_GROUP_ATTACHED_TO_VM_POOL=Cannot remove Directory Group. Detach Group from VM-Pool first. Line 2: IO_CD_IMAGE_FILE_ALREADY_EXIST=Image file already exists. Line 3: IO_CD_IMAGE_FILE_NOT_EXIST=Image file does not exist. Line 4: IO_INVALID_CD_IMAGE_EXTENSION=Invalid CD image extension. -- To view, visit http://gerrit.ovirt.org/33416 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I196a583ae7d8d7e373a1aca2a48e592232b18a5b Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
