Vojtech Szocs has posted comments on this change. Change subject: docker: new docker plugin ......................................................................
Patch Set 2: Code-Review+1 (5 comments) Nice plugin! Just a few comments/questions (inline), otherwise looks good. http://gerrit.ovirt.org/#/c/25814/2/docker-plugin/docker-resources/launch-docker-dialog.html File docker-plugin/docker-resources/launch-docker-dialog.html: Line 1: <!DOCTYPE html> Line 2: <html> Line 3: <head></head> Line 4: <style> I think this <style> should be inside <head> Line 5: Line 6: .select { Line 7: border: 1px solid gray; Line 8: background-color: white; http://gerrit.ovirt.org/#/c/25814/2/docker-plugin/docker-resources/plugin.html File docker-plugin/docker-resources/plugin.html: Line 26: jQuery.ajax({ Line 27: type: "GET", Line 28: dataType: "json", Line 29: url: dcsUrl, Line 30: headers: { 'engineSessionId' : sessionId, 'Prefer' : 'persistent-auth' }, Hm, does oVirt REST backend expect "engineSessionId" header? My understanding is that upon REST session acquiry by WebAdmin's UI plugin infra, JSESSIONID cookie for /ovirt-engine/api/ is set in browser, so any request to /ovirt-engine/api/ (including the one above) will carry this cookie. Line 31: success: function(data) { Line 32: for (var index in data.data_center) { Line 33: dcs[index] = data.data_center[index].name; Line 34: } Line 31: success: function(data) { Line 32: for (var index in data.data_center) { Line 33: dcs[index] = data.data_center[index].name; Line 34: } Line 35: formWindow.updateDataCenters(dcs); You might want to check if formWindow is not null, for example: formWindow && formWindow.updateDataCenters(dcs); Line 36: } Line 37: }); Line 38: } Line 39: Line 113: return arguments.length == 0; Line 114: }, Line 115: onClick: function() { Line 116: api.showDialog('Create Docker VM', 'launch-docker', Line 117: '/ovirt-engine/webadmin/plugin/docker/launch-docker-dialog.html', I suggest to use relative URLs, for example: plugin/docker/launch-docker-dialog.html Line 118: '540px', '500px', Line 119: { Line 120: buttons: [ Line 121: { Line 150: restSessionId = sessionId; Line 151: }, Line 152: MessageReceived: function(data, sourceWindow) { Line 153: // If we get here, we already passed allowed source origin check Line 154: var eventDataPair = data.split('-'); Other content (i.e. main tab added by another UI plugin) can send messages too, so "data" might not always be string in given format. I suggest to do some verification of message's sender. For example, your content (main tab) sends message "docker-plugin:DoSomething:123" (string) and your UI plugin can check it: if (typeof data !== 'string') { return; } var tuple = data.split(':'); if (tuple[0] !== 'docker-plugin') { return; } var params = tuple.slice(1); alert(params[0]); // prints "DoSomething" alert(params[1]); // prints "123" Line 155: switch (eventDataPair[0]) { Line 156: case 'GetDataCenters': Line 157: formWindow = sourceWindow; Line 158: getDCList(restSessionId, config.apiEntryPoint); -- To view, visit http://gerrit.ovirt.org/25814 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6fbeecd38207815399f1e4afc86dd57465a010a9 Gerrit-PatchSet: 2 Gerrit-Project: samples-uiplugins Gerrit-Branch: master Gerrit-Owner: Oved Ourfali <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
