Ori Liel has posted comments on this change. Change subject: engine: support non-unique vm and template names across DCs ......................................................................
Patch Set 2: (21 comments) https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetInstanceTypeQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetInstanceTypeQuery.java: Line 13: protected void executeQueryCommand() { Line 14: InstanceType instance; Line 15: GetVmTemplateParameters params = getParameters(); Line 16: if (params.getName() != null) { Line 17: instance = DbFacade.getInstance().getVmTemplateDao() > You should use injection of the DAO here Done Line 18: .getInstanceByName(params.getName(), getUserID(), getParameters().isFiltered()); Line 19: } Line 20: else { Line 21: instance = DbFacade.getInstance().getVmTemplateDao() Line 17: instance = DbFacade.getInstance().getVmTemplateDao() Line 18: .getInstanceByName(params.getName(), getUserID(), getParameters().isFiltered()); Line 19: } Line 20: else { Line 21: instance = DbFacade.getInstance().getVmTemplateDao() > same Done Line 22: .get(getParameters().getId(), getUserID(), getParameters().isFiltered()); Line 23: } Line 24: getQueryReturnValue().setReturnValue(instance); Line 25: } https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmTemplateQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmTemplateQuery.java: Line 32: Line 33: // Get the DC ID. If no DC info available, the query will return the first VM Line 34: // with the given name found. Line 35: private Guid getStoragePoolId(GetVmTemplateParameters params) { Line 36: return params.getStoragePoolId() != null ? params.getStoragePoolId() : params.getVdsGroupId() != null ? DbFacade.getInstance().getVdsGroupDao().get(params.getVdsGroupId()).getStoragePoolId() : null; > also, DbFacade.getInstance().getVdsGroupDao().get(params.getVdsGroupId()) c Done Line 37: } Line 32: Line 33: // Get the DC ID. If no DC info available, the query will return the first VM Line 34: // with the given name found. Line 35: private Guid getStoragePoolId(GetVmTemplateParameters params) { Line 36: return params.getStoragePoolId() != null ? params.getStoragePoolId() : params.getVdsGroupId() != null ? DbFacade.getInstance().getVdsGroupDao().get(params.getVdsGroupId()).getStoragePoolId() : null; > please format this line , its too long ... Done Line 37: } https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java: Line 236: return true; Line 237: } Line 238: Line 239: protected boolean isVmTemplateWithSameNameExist() { Line 240: return DbFacade.getInstance() > please inject the DAO Done Line 241: .getVmTemplateDao() Line 242: .getByName(getParameters().getVmTemplate().getName(), getParameters().getStoragePoolId(), null, false) != null; Line 243: } Line 244: Line 236: return true; Line 237: } Line 238: Line 239: protected boolean isVmTemplateWithSameNameExist() { Line 240: return DbFacade.getInstance() > why not keep using that method and just add a parameter (dcId) to it? Makes mocking for new tests very difficult. Line 241: .getVmTemplateDao() Line 242: .getByName(getParameters().getVmTemplate().getName(), getParameters().getStoragePoolId(), null, false) != null; Line 243: } Line 244: https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsVmTemlateWithSameNameExistQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsVmTemlateWithSameNameExistQuery.java: Line 17: getParameters().getDatacenterId())); Line 18: } Line 19: Line 20: public boolean isVmTemlateWithSameNameExist(String name, Guid datacenterId) { Line 21: return DbFacade.getInstance().getVmTemplateDao().getByName(name, datacenterId, null, false) != null; > please inject the DAO Done Line 22: } https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java: Line 101: 'Instance' > an instance type Done Line 104: if (isInstanceWithSameNameExists(getVmTemplateName())) { Line 105: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NAME_ALREADY_USED); Line 106: } Line 107: } else { Line 108: if (isVmTemlateWithSameNameExist(getVmTemplateName(), getVdsGroup().getStoragePoolId())) { > - what about blank template? Newly the "Blank" template can be edited and r Good point, I'll look into it Line 109: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NAME_ALREADY_USED); Line 110: } Line 111: } Line 112: } Line 104: if (isInstanceWithSameNameExists(getVmTemplateName())) { Line 105: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NAME_ALREADY_USED); Line 106: } Line 107: } else { Line 108: if (isVmTemlateWithSameNameExist(getVmTemplateName(), getVdsGroup().getStoragePoolId())) { > since blank has no dc/cluster any more, i think its ok to have another "bla Good point, I'll look into it Line 109: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NAME_ALREADY_USED); Line 110: } Line 111: } Line 112: } https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmTemplateCommand.java: Line 61: protected void executeCommand() { Line 62: throw new NotImplementedException(); Line 63: } Line 64: Line 65: public boolean isVmTemlateWithSameNameExist(String name, Guid datacenterId) { > i would leave it static and use it from other places as before The problem is with the tests, you can't mock it when it's static. It wasn't tested before and my new tests test it. Line 66: return DbFacade.getInstance().getVmTemplateDao().getByName(name, datacenterId, null, false) != null; Line 67: } Line 68: Line 69: public boolean isInstanceWithSameNameExists(String name) { Line 62: throw new NotImplementedException(); Line 63: } Line 64: Line 65: public boolean isVmTemlateWithSameNameExist(String name, Guid datacenterId) { Line 66: return DbFacade.getInstance().getVmTemplateDao().getByName(name, datacenterId, null, false) != null; > please inject the DAO Done Line 67: } Line 68: Line 69: public boolean isInstanceWithSameNameExists(String name) { Line 70: return DbFacade.getInstance().getVmTemplateDao().getInstanceByName(name, null, false) != null; Line 66: return DbFacade.getInstance().getVmTemplateDao().getByName(name, datacenterId, null, false) != null; Line 67: } Line 68: Line 69: public boolean isInstanceWithSameNameExists(String name) { Line 70: return DbFacade.getInstance().getVmTemplateDao().getInstanceByName(name, null, false) != null; > please inject the DAO Done Line 71: } Line 72: Line 73: public static boolean isVmTemplateImagesReady(VmTemplate vmTemplate, Line 74: Guid storageDomainId, https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetVmTemplateParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetVmTemplateParameters.java: Line 6: private static final long serialVersionUID = 8906662143775124331L; Line 7: Line 8: private Guid _id; Line 9: private String _name; Line 10: private Guid vdsGroupId; > vdsGroupId => clusterId Done Line 11: private Guid storagePoolId; Line 12: Line 13: public GetVmTemplateParameters(Guid id) { Line 14: _id = id; Line 37: public void setStoragePoolId(Guid storagePoolId) { Line 38: this.storagePoolId = storagePoolId; Line 39: } Line 40: Line 41: public Guid getVdsGroupId() { > I would use the new term : Cluster in new code .... Done Line 42: return vdsGroupId; Line 43: } Line 44: Line 45: public void setVdsGroupId(Guid vdsGroupId) { Line 42: return vdsGroupId; Line 43: } Line 44: Line 45: public void setVdsGroupId(Guid vdsGroupId) { Line 46: this.vdsGroupId = vdsGroupId; > same Done Line 47: } Line 48: https://gerrit.ovirt.org/#/c/41263/2/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmTemplateDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmTemplateDAO.java: Line 54: * @param isFiltered Line 55: * Whether the results should be filtered according to the user's permissions Line 56: * @return The Instance which has this name of null if no such exists. Line 57: */ Line 58: public VmTemplate getInstanceByName(String name, Guid userID, boolean isFiltered); > please rename to getInstanceTypeByName - the instance is a specific VM base Done Line 59: Line 60: /** Line 61: * Retrieves all templates with optional filtering. Line 62: * @param entityType https://gerrit.ovirt.org/#/c/41263/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java: Line 618: boolean isNameUnique = (Boolean) returnValue; Line 619: templateListModel.postNameUniqueCheck(isNameUnique); Line 620: Line 621: } Line 622: }), name, model.getDataCenterWithClustersList().getSelectedItem().getDataCenter().getId()); > - you can simplify this to model.getSelectedDataCenter().getId() Thanks, I'll consider this Line 623: } else { Line 624: postNameUniqueCheck(true); Line 625: } Line 626: } https://gerrit.ovirt.org/#/c/41263/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java: Line 653: } Line 654: Line 655: } Line 656: }), Line 657: name, model.getDataCenterWithClustersList().getSelectedItem().getDataCenter().getId()); > you can simplify this to model.getSelectedDataCenter().getId() Done Line 658: } Line 659: } Line 660: Line 661: private void postNameUniqueCheck(UserPortalListModel userPortalListModel) https://gerrit.ovirt.org/#/c/41263/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java: Line 1345: } Line 1346: Line 1347: } Line 1348: }), Line 1349: name, model.getDataCenterWithClustersList().getSelectedItem().getDataCenter().getId()); > you can simplify this to getSelectedDataCenter().getId() Done Line 1350: } Line 1351: } Line 1352: Line 1353: private void postNameUniqueCheck() https://gerrit.ovirt.org/#/c/41263/2/packaging/dbscripts/vm_templates_sp.sql File packaging/dbscripts/vm_templates_sp.sql: Line 463: BEGIN Line 464: RETURN QUERY SELECT vm_templates.* Line 465: FROM vm_templates_view vm_templates Line 466: WHERE name = v_vmt_name Line 467: AND (v_storage_pool_id is null OR storage_pool_id = v_storage_pool_id) > please remove TWS Done Line 468: AND (NOT v_is_filtered OR EXISTS (SELECT 1 Line 469: FROM user_vm_template_permissions_view Line 470: WHERE user_id = v_user_id AND entity_id = vmt_guid)); Line 471: END; $procedure$ -- To view, visit https://gerrit.ovirt.org/41263 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f3244ec1885d54e58b475d0e74f59e26fa492a0 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ori Liel <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
