Jakub Niedermertl has posted comments on this change. Change subject: core, webadmin: Vm Icons REST related changes ......................................................................
Patch Set 4: (6 comments) https://gerrit.ovirt.org/#/c/42047/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllVmIconsQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllVmIconsQuery.java: Line 15: } Line 16: Line 17: @Override Line 18: protected void executeQueryCommand() { Line 19: setReturnValue(vmIconDao.getAll()); > why is this needed? will this return all the icons from all users? in a use I agree, it's wrong. But REST api requires it for listing icons collection. To keep rest api consistent. Line 20: } https://gerrit.ovirt.org/#/c/42047/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmIconQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmIconQuery.java: Line 18: } Line 19: Line 20: @Override Line 21: protected void executeQueryCommand() { Line 22: setReturnValue(vmIconDao.get(getParameters().getId())); > since this is a user query we need to think if its reasonable to return ico Michal gave it a thought and it is considered ok. Line 23: } https://gerrit.ovirt.org/#/c/42047/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmManagementCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmManagementCommandBase.java: Line 60: } Line 61: return null; Line 62: } Line 63: Line 64: protected Guid getSmallIconId() { > why you dont use these in updateVmCommand? Done Line 65: if (getParameters().getVmStaticData() != null) { Line 66: return getParameters().getVmStaticData().getSmallIconId(); Line 67: } Line 68: return null; https://gerrit.ovirt.org/#/c/42047/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/IconUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/IconUtils.java: Line 27: Line 28: private IconUtils() { Line 29: } Line 30: Line 31: public static ValidationResult validateIconId(Guid iconId, String nameForErrorMessage, VmIconDao vmIconDao) { > why this is not in the icon validator? and then you wouldnt need to pass th Done, moved Line 32: if (vmIconDao.exists(iconId)) { Line 33: return ValidationResult.VALID; Line 34: } Line 35: return new ValidationResult(VdcBllMessages.ICON_OF_PROVIDED_ID_DOES_NOT_EXIST, https://gerrit.ovirt.org/#/c/42047/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmIcon.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmIcon.java: Line 38: public Pair<String, String> getTypeAndData() { Line 39: return dataUrlToTypeAndData(this.dataUrl); Line 40: } Line 41: Line 42: public static Pair<String, String> dataUrlToTypeAndData(String dataUrl) { > what is this? These are helper methods. REST is required to work with media_type and data separately and these methods converts data to/from dataurl. Methods are used from VmIcon.set/getTypeAndData and also from rest code, that's why they are public. Line 43: final String dataUrlRegex = "^data:(\\w+/\\w+);base64,([\\w+/]+={0,2})$"; Line 44: final Matcher matcher = Pattern.compile(dataUrlRegex).matcher(dataUrl); Line 45: final boolean matches = matcher.find(); Line 46: if (!matches) { https://gerrit.ovirt.org/#/c/42047/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetVmIconDefaultParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetVmIconDefaultParameters.java: Line 6: private int operatingSystemId; Line 7: Line 8: private GetVmIconDefaultParameters() {} Line 9: Line 10: public static GetVmIconDefaultParameters create(int operatingSystemId) { > why using this approach? not sure i understand this.. Done Line 11: final GetVmIconDefaultParameters result = new GetVmIconDefaultParameters(); Line 12: result.operatingSystemId = operatingSystemId; Line 13: return result; Line 14: } -- To view, visit https://gerrit.ovirt.org/42047 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I20440c2443f9217d0fa896fff2911943467954e9 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
