Omer Frenkel has posted comments on this change.

Change subject: core, webadmin: Vm Icons REST related changes
......................................................................


Patch Set 4:

(3 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());
> I agree, it's wrong. But REST api requires it for listing icons collection.
not sure i agree but anyway this cannot be user query
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()));
> Michal gave it a thought and it is considered ok.
ok but i think its wrong, we should filter icons that are used by the caller 
permitted vms
Line 23:     }


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) {
> These are helper methods. REST is required to work with media_type and data
please add some doc for this emthod
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) {


-- 
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

Reply via email to