BearND has submitted this change and it was merged.

Change subject: Revert "Revert "Update to service-template-node v0.5.0 (minus 
eslint)""
......................................................................


Revert "Revert "Update to service-template-node v0.5.0 (minus eslint)""

This reverts commit 674e1a98792ef42616a3e3473e88ee01827b8c83.

This was reverted because some tests persistently failed when running in 
Docker.  However, this failure only occurred when running Docker for Mac; the 
tests pass as expected when running in Docker on a Linux host.[1]  Therefore 
this should be safe to merge and deploy.

[1] Tested on Ubuntu Wily and Debian Jessie hosts.

Change-Id: I4010bf58d86e241d4c03d6149f9547f00a45ba45
---
A .eslintrc.yml
M .travis.yml
M config.dev.yaml
M lib/api-util.js
M lib/util.js
M package.json
M routes/info.js
M routes/root.js
M scripts/gen-init-scripts.rb
9 files changed, 98 insertions(+), 105 deletions(-)

Approvals:
  BearND: Looks good to me, approved
  Mobrovac: Looks good to me, but someone else must approve
  jenkins-bot: Verified



diff --git a/.eslintrc.yml b/.eslintrc.yml
new file mode 100644
index 0000000..9e2c225
--- /dev/null
+++ b/.eslintrc.yml
@@ -0,0 +1 @@
+extends: 'eslint-config-node-services'
\ No newline at end of file
diff --git a/.travis.yml b/.travis.yml
index fa7f265..c417c7f 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -3,8 +3,6 @@
 sudo: false
 
 node_js:
-  - "0.10"
-  - "0.12"
   - "4"
   - "6"
-  = "node"
+  - "node"
diff --git a/config.dev.yaml b/config.dev.yaml
index c0e04e2..3b0d891 100644
--- a/config.dev.yaml
+++ b/config.dev.yaml
@@ -64,14 +64,14 @@
       # the template used for contacting the MW API
       mwapi_req:
         method: post
-        uri: 'https://{{domain}}/w/api.php'
+        uri: https://{{domain}}/w/api.php
         headers:
           user-agent: '{{user-agent}}'
         body: '{{ default(request.query, {}) }}'
       # the template used for contacting RESTBase
       restbase_req:
-        method: get
-        uri: 'https://{{domain}}/api/rest_v1/{+path}'
+        method: '{{request.method}}'
+        uri: https://{{domain}}/api/rest_v1/{+path}
         query: '{{ default(request.query, {}) }}'
         headers: '{{request.headers}}'
         body: '{{request.body}}'
diff --git a/lib/api-util.js b/lib/api-util.js
index e763c67..e8da8da 100644
--- a/lib/api-util.js
+++ b/lib/api-util.js
@@ -23,14 +23,14 @@
 
     const request = app.mwapi_tpl.expand({
         request: {
-            params: { domain: domain },
+            params: { domain },
             headers: { 'user-agent': app.conf.user_agent },
-            query: query
+            query
         }
     });
 
-    return preq(request).then(function(response) {
-        if(response.status < 200 || response.status > 399) {
+    return preq(request).then((response) => {
+        if (response.status < 200 || response.status > 399) {
             // there was an error when calling the upstream service, propagate 
that
             throw new HTTPError({
                 status: response.status,
@@ -67,7 +67,7 @@
     const request = app.restbase_tpl.expand({
         request: {
             method: restReq.method,
-            params: { domain: domain, path: path },
+            params: { domain, path },
             query: restReq.query,
             headers: Object.assign({ 'user-agent': app.conf.user_agent }, 
restReq.headers),
             body: restReq.body
@@ -87,7 +87,7 @@
 function setupApiTemplates(app) {
 
     // set up the MW API request template
-    if(!app.conf.mwapi_req) {
+    if (!app.conf.mwapi_req) {
         app.conf.mwapi_req = {
             uri: 'http://{{domain}}/w/api.php',
             headers: {
@@ -99,7 +99,7 @@
     app.mwapi_tpl = new Template(app.conf.mwapi_req);
 
     // set up the RESTBase request template
-    if(!app.conf.restbase_req) {
+    if (!app.conf.restbase_req) {
         app.conf.restbase_req = {
             method: '{{request.method}}',
             uri: 'http://{{domain}}/api/rest_v1/{+path}',
@@ -130,9 +130,9 @@
 
 
 module.exports = {
-    mwApiGet: mwApiGet,
-    restApiGet: restApiGet,
-    setupApiTemplates: setupApiTemplates,
-    checkResponseStatus: checkResponseStatus
+    mwApiGet,
+    restApiGet,
+    setupApiTemplates,
+    checkResponseStatus
 };
 
diff --git a/lib/util.js b/lib/util.js
index 4560e96..0e607e1 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -2,7 +2,6 @@
 
 
 const BBPromise = require('bluebird');
-const util = require('util');
 const express = require('express');
 const uuid = require('cassandra-uuid');
 const bunyan = require('bunyan');
@@ -11,36 +10,31 @@
 /**
  * Error instance wrapping HTTP error responses
  */
-function HTTPError(response) {
+class HTTPError extends Error {
 
-    Error.call(this);
-    Error.captureStackTrace(this, HTTPError);
+    constructor(response) {
+        super();
+        Error.captureStackTrace(this, HTTPError);
 
-    if(response.constructor !== Object) {
-        // just assume this is just the error message
-        const msg = response;
-        response = {
-            status: 500,
-            type: 'internal_error',
-            title: 'InternalError',
-            detail: msg
-        };
+        if (response.constructor !== Object) {
+            // just assume this is just the error message
+            response = {
+                status: 500,
+                type: 'internal_error',
+                title: 'InternalError',
+                detail: response
+            };
+        }
+
+        this.name = this.constructor.name;
+        this.message = `${response.status}`;
+        if (response.type) {
+            this.message += `: ${response.type}`;
+        }
+
+        Object.assign(this, response);
     }
-
-    this.name = this.constructor.name;
-    this.message = response.status + '';
-    if(response.type) {
-        this.message += ': ' + response.type;
-    }
-
-    for (const key in response) {
-        this[key] = response[key];
-    }
-
 }
-
-util.inherits(HTTPError, Error);
-
 
 /**
  * Generates an object suitable for logging out of a request object
@@ -62,9 +56,9 @@
         remotePort: req.connection.remotePort
     };
 
-    if(req.headers && whitelistRE) {
-        Object.keys(req.headers).forEach(function(hdr) {
-            if(whitelistRE.test(hdr)) {
+    if (req.headers && whitelistRE) {
+        Object.keys(req.headers).forEach((hdr) => {
+            if (whitelistRE.test(hdr)) {
                 ret.headers[hdr] = req.headers[hdr];
             }
         });
@@ -89,7 +83,7 @@
     ret.detail = err.detail;
 
     // log the stack trace only for 500 errors
-    if(Number.parseInt(ret.status) !== 500) {
+    if (Number.parseInt(ret.status) !== 500) {
         ret.stack = undefined;
     }
 
@@ -120,31 +114,29 @@
  */
 function wrapRouteHandlers(route, app) {
 
-    route.router.stack.forEach(function(routerLayer) {
+    route.router.stack.forEach((routerLayer) => {
         let path = (route.path + routerLayer.route.path.slice(1))
             .replace(/\/:/g, '/--')
             .replace(/^\//, '')
-            .replace(/[\/?]+$/, '');
+            .replace(/[/?]+$/, '');
         path = app.metrics.normalizeName(path || 'root');
-        routerLayer.route.stack.forEach(function(layer) {
+        routerLayer.route.stack.forEach((layer) => {
             const origHandler = layer.handle;
             layer.handle = function(req, res, next) {
                 const startTime = Date.now();
-                BBPromise.try(function() {
-                    return origHandler(req, res, next);
-                })
+                BBPromise.try(() => origHandler(req, res, next))
                 .catch(next)
-                .finally(function() {
+                .finally(() => {
                     let statusCode = parseInt(res.statusCode) || 500;
-                    if(statusCode < 100 || statusCode > 599) {
+                    if (statusCode < 100 || statusCode > 599) {
                         statusCode = 500;
                     }
-                    const statusClass = Math.floor(statusCode / 100) + 'xx';
-                    const stat = path + '.' + req.method + '.';
+                    const statusClass = `${Math.floor(statusCode / 100)}xx`;
+                    const stat = `${path}.${req.method}.`;
                     app.metrics.endTiming([
                         stat + statusCode,
                         stat + statusClass,
-                        stat + 'ALL'
+                        `${stat}ALL`
                     ], startTime);
                 });
             };
@@ -162,17 +154,17 @@
  */
 function setErrorHandler(app) {
 
-    app.use(function(err, req, res, next) {
+    app.use((err, req, res, next) => {
         let errObj;
         // ensure this is an HTTPError object
-        if(err.constructor === HTTPError) {
+        if (err.constructor === HTTPError) {
             errObj = err;
-        } else if(err instanceof Error) {
+        } else if (err instanceof Error) {
             // is this an HTTPError defined elsewhere? (preq)
-            if(err.constructor.name === 'HTTPError') {
+            if (err.constructor.name === 'HTTPError') {
                 const o = { status: err.status };
-                if(err.body && err.body.constructor === Object) {
-                    Object.keys(err.body).forEach(function(key) {
+                if (err.body && err.body.constructor === Object) {
+                    Object.keys(err.body).forEach((key) => {
                         o[key] = err.body[key];
                     });
                 } else {
@@ -190,7 +182,7 @@
                     stack: err.stack
                 });
             }
-        } else if(err.constructor === Object) {
+        } else if (err.constructor === Object) {
             // this is a regular object, suppose it's a response
             errObj = new HTTPError(err);
         } else {
@@ -203,24 +195,23 @@
             });
         }
         // ensure some important error fields are present
-        if(!errObj.status) { errObj.status = 500; }
-        if(!errObj.type) { errObj.type = 'internal_error'; }
+        if (!errObj.status) { errObj.status = 500; }
+        if (!errObj.type) { errObj.type = 'internal_error'; }
         // add the offending URI and method as well
-        if(!errObj.method) { errObj.method = req.method; }
-        if(!errObj.uri) { errObj.uri = req.url; }
+        if (!errObj.method) { errObj.method = req.method; }
+        if (!errObj.uri) { errObj.uri = req.url; }
         // some set 'message' or 'description' instead of 'detail'
         errObj.detail = errObj.detail || errObj.message || errObj.description 
|| '';
         // adjust the log level based on the status code
         let level = 'error';
-        if(Number.parseInt(errObj.status) < 400) {
+        if (Number.parseInt(errObj.status) < 400) {
             level = 'trace';
-        } else if(Number.parseInt(errObj.status) < 500) {
+        } else if (Number.parseInt(errObj.status) < 500) {
             level = 'info';
         }
         // log the error
-        (req.logger || app.logger).log(level + '/' +
-                (errObj.component ? errObj.component : errObj.status),
-                errForLog(errObj));
+        const component = (errObj.component ? errObj.component : 
errObj.status);
+        (req.logger || app.logger).log(`${level}/${component}`, 
errForLog(errObj));
         // let through only non-sensitive info
         const respBody = {
             status: errObj.status,
@@ -248,13 +239,11 @@
         mergeParams: true
     };
 
-    if(opts && opts.constructor === Object) {
-        Object.keys(opts).forEach(function(key) {
-            options[key] = opts[key];
-        });
+    if (opts && opts.constructor === Object) {
+        Object.assign(options, opts);
     }
 
-    return express.Router(options);
+    return new express.Router(options);
 
 }
 
@@ -268,15 +257,18 @@
 function initAndLogRequest(req, app) {
     req.headers = req.headers || {};
     req.headers['x-request-id'] = req.headers['x-request-id'] || 
generateRequestId();
-    req.logger = app.logger.child({request_id: req.headers['x-request-id'], 
request: reqForLog(req, app.conf.log_header_whitelist)});
-    req.logger.log('trace/req', {msg: 'incoming request'});
+    req.logger = app.logger.child({
+        request_id: req.headers['x-request-id'],
+        request: reqForLog(req, app.conf.log_header_whitelist)
+    });
+    req.logger.log('trace/req', { msg: 'incoming request' });
 }
 
 
 module.exports = {
-    HTTPError: HTTPError,
-    initAndLogRequest: initAndLogRequest,
-    wrapRouteHandlers: wrapRouteHandlers,
-    setErrorHandler: setErrorHandler,
+    HTTPError,
+    initAndLogRequest,
+    wrapRouteHandlers,
+    setErrorHandler,
     router: createRouter
 };
diff --git a/package.json b/package.json
index 50adf24..4b30e76 100644
--- a/package.json
+++ b/package.json
@@ -40,19 +40,19 @@
   "dependencies": {
     "bluebird": "^3.4.6",
     "body-parser": "^1.15.2",
-    "bunyan": "^1.8.3",
+    "bunyan": "^1.8.5",
     "cassandra-uuid": "^0.0.2",
     "compression": "^1.6.2",
     "core-js": "^2.4.1",
     "domino": "^1.0.27",
     "escape-string-regexp": "^1.0.5",
     "express": "^4.14.0",
-    "js-yaml": "^3.6.1",
+    "js-yaml": "^3.7.0",
     "locutus": "^2.0.5",
     "mediawiki-title": "^0.5.6",
-    "preq": "^0.4.12",
-    "service-runner": "^2.1.7",
-    "swagger-router": "^0.5.2",
+    "preq": "^0.5.0",
+    "service-runner": "^2.1.11",
+    "swagger-router": "^0.5.5",
     "underscore": "^1.8.3"
   },
   "devDependencies": {
@@ -64,7 +64,9 @@
     "mocha-jshint": "^2.3.1",
     "mocha-lcov-reporter": "^1.2.0",
     "nsp": "^2.6.2",
-    "sepia": "^2.0.1"
+    "sepia": "^2.0.1",
+    "mocha-eslint": "^3.0.1",
+    "eslint-config-node-services": "^1.0.6"
   },
   "deploy": {
     "target": "debian",
diff --git a/routes/info.js b/routes/info.js
index 0f64132..832af0d 100644
--- a/routes/info.js
+++ b/routes/info.js
@@ -19,7 +19,7 @@
  * GET /
  * Gets some basic info about this service
  */
-router.get('/', function(req, res) {
+router.get('/', (req, res) => {
 
     // simple sync return
     res.json({
@@ -36,7 +36,7 @@
  * GET /name
  * Gets the service's name as defined in package.json
  */
-router.get('/name', function(req, res) {
+router.get('/name', (req, res) => {
 
     // simple return
     res.json({ name: app.info.name });
@@ -48,7 +48,7 @@
  * GET /version
  * Gets the service's version as defined in package.json
  */
-router.get('/version', function(req, res) {
+router.get('/version', (req, res) => {
 
     // simple return
     res.json({ version: app.info.version });
@@ -61,29 +61,28 @@
  * Redirects to the service's home page if one is given,
  * returns a 404 otherwise
  */
-router.all('/home', function(req, res) {
+router.all('/home', (req, res) => {
 
     const home = app.info.homepage;
-    if(home && /^http/.test(home)) {
+    if (home && /^http/.test(home)) {
         // we have a home page URI defined, so send it
         res.redirect(301, home);
-        return;
     } else {
         // no URI defined for the home page, error out
-        res.status(404).end('No home page URL defined for ' + app.info.name);
+        res.status(404).end(`No home page URL defined for ${app.info.name}`);
     }
 
 });
 
 
-module.exports = function(appObj) {
+module.exports = (appObj) => {
 
     app = appObj;
 
     return {
         path: '/_info',
         skip_domain: true,
-        router: router
+        router
     };
 
 };
diff --git a/routes/root.js b/routes/root.js
index c39c8c5..cf0ffda 100644
--- a/routes/root.js
+++ b/routes/root.js
@@ -19,7 +19,7 @@
  * GET /robots.txt
  * Instructs robots no indexing should occur on this domain.
  */
-router.get('/robots.txt', function(req, res) {
+router.get('/robots.txt', (req, res) => {
 
     res.set({
         'User-agent': '*',
@@ -34,9 +34,9 @@
  * Main entry point. Currently it only responds if the spec query
  * parameter is given, otherwise lets the next middleware handle it
  */
-router.get('/', function(req, res, next) {
+router.get('/', (req, res, next) => {
 
-    if(!(req.query || {}).hasOwnProperty('spec')) {
+    if (!{}.hasOwnProperty.call(req.query || {}, 'spec')) {
         next();
     } else {
         res.json(app.conf.spec);
@@ -52,7 +52,7 @@
     return {
         path: '/',
         skip_domain: true,
-        router: router
+        router
     };
 
 };
diff --git a/scripts/gen-init-scripts.rb b/scripts/gen-init-scripts.rb
index 3807172..eac05fd 100755
--- a/scripts/gen-init-scripts.rb
+++ b/scripts/gen-init-scripts.rb
@@ -68,3 +68,4 @@
 data = ScriptData.new indir
 data.set_info rootdir
 data.generate outdir
+

-- 
To view, visit https://gerrit.wikimedia.org/r/324750
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4010bf58d86e241d4c03d6149f9547f00a45ba45
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/services/mobileapps
Gerrit-Branch: master
Gerrit-Owner: Mholloway <mhollo...@wikimedia.org>
Gerrit-Reviewer: BearND <bsitzm...@wikimedia.org>
Gerrit-Reviewer: Dbrant <dbr...@wikimedia.org>
Gerrit-Reviewer: Fjalapeno <cfl...@wikimedia.org>
Gerrit-Reviewer: GWicke <gwi...@wikimedia.org>
Gerrit-Reviewer: Jdlrobson <jrob...@wikimedia.org>
Gerrit-Reviewer: Jhernandez <jhernan...@wikimedia.org>
Gerrit-Reviewer: Mhurd <mh...@wikimedia.org>
Gerrit-Reviewer: Mobrovac <mobro...@wikimedia.org>
Gerrit-Reviewer: Niedzielski <sniedziel...@wikimedia.org>
Gerrit-Reviewer: Ppchelko <ppche...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to