Vojtech Szocs has posted comments on this change. Change subject: Gluster Nagios-dashboard plugin. ......................................................................
Patch Set 20: Code-Review+2 (4 comments) Very cool plugin! Just a few small comments, looks great overall. http://gerrit.ovirt.org/#/c/25064/20/gluster-nagios-dashboard/.gitignore File gluster-nagios-dashboard/.gitignore: Line 1: #Ignore list Feel free to add these ignores into global samples-uiplugins .gitignore file. Line 2: Line 3: # Local Node.js modules Line 4: node_modules/ Line 5: http://gerrit.ovirt.org/#/c/25064/20/gluster-nagios-dashboard/src/js/dashboard.js File gluster-nagios-dashboard/src/js/dashboard.js: Line 1: 'use strict' Line 2: Line 3: var summaryCtrl = function ($scope, $q, clusterService, volumeService, hostService) { Hm, it might be better to prevent variables from polluting global scope via Immediately-Invoked Function Expression, just like in dashboard-init.js: ( // Definition of anonymous function function (dashboardApp) { // Closure ensures that variables defined below have access to "dashboardApp" // parameter and global (Window) object, but their "var" declaration doesn't pollute // the outer scope -- these "var" declarations are scoped to this anonymous function var summaryCtrl = ... var alertsCtrl = ... var clustersCtrl = ... ... } // Execution of function via "()" operator ( // Arguments passed to function angular.module('dashboardApp', ['ui.bootstrap', 'plugin.dashboard.services']) ) ); Line 4: $scope.summary = { Line 5: no_of_clusters: 0, Line 6: hosts_up: 0, Line 7: hosts_down: 0, Line 102: dashboardApp.controller("SummaryCtrl", ['$scope', '$q', 'ClusterService', 'VolumeService', 'HostService', summaryCtrl]); Line 103: dashboardApp.controller("AlertsCtrl", ['$scope', 'AlertService', alertsCtrl]); Line 104: dashboardApp.controller("ClustersCtrl", ['$scope', 'ClusterService', 'VolumeService', clustersCtrl]); Line 105: Line 106: function fetchVolumes($http, cluster) { This function declaration (without "var" statement) essentially adds global variable. You can consider moving this function into some AngularJS service that can be injected into other services or controllers. Also, "$http" could be injected as part of service declaration so users wouldn't have to worry about it. For example: appropriateAngularModule.factory('httpUtils', ['$http', function($http) { return { fetchVolumes: function(cluster) { ... } }; }]); Line 107: cluster.volumes = [ {name:'vol1', capacity:50, used: 40} ]; Line 108: var volumeUrl = '/api/clusters/'+cluster.id+'/glustervolumes'; Line 109: $http({method: 'GET', url: volumeUrl, headers: {Accept: 'application/json'}}). Line 110: success(function (data) { http://gerrit.ovirt.org/#/c/25064/20/gluster-nagios-dashboard/src/js/services.js File gluster-nagios-dashboard/src/js/services.js: Line 4: Line 5: var alertService = function ($http) { Line 6: return { Line 7: getAlerts: function() { Line 8: return $http({method: 'GET', url: '/api/events?search=severity%3Dalert', headers: {Accept: 'application/json'}}). In future, this can be done more easily (code-friendly) with oVirt JavaScript SDK :-) Line 9: then(function (response) { Line 10: if(typeof response.data.event !== 'undefined') { Line 11: return response.data.event; Line 12: } -- To view, visit http://gerrit.ovirt.org/25064 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I122f1b8640d72dbebc0c04b6fa42c98a212dca92 Gerrit-PatchSet: 20 Gerrit-Project: samples-uiplugins Gerrit-Branch: master Gerrit-Owner: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
