From 4c9bcd8ed8c65c160ba58542db0338f6437f91ea Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 08:56:14 +0800 Subject: [PATCH 01/44] Add runnable-api-client as dependency --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index 693cb85..66f1ca7 100644 --- a/package.json +++ b/package.json @@ -18,6 +18,7 @@ "grunt-contrib-sass": "^0.9.2", "grunt-jade-plugin": "^0.6.0", "jade": "~1.9.2", + "runnable": "git+ssh://git@github.com:CodeNow/runnable-api-client#v4.7.1", "serve-favicon": "~2.2.1" } } From 9e50eb7948a2f6691e3ea0a28236c35cce456e12 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 12:39:37 +0800 Subject: [PATCH 02/44] Add npm dependencies and logic for super-user client --- app.js | 38 +++++++++++++++++++++++++++++++++----- package.json | 3 +++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/app.js b/app.js index e701c49..04fa80c 100644 --- a/app.js +++ b/app.js @@ -3,9 +3,11 @@ var express = require('express'); var path = require('path'); var bodyParser = require('body-parser'); +var Runnable = require('runnable'); -var version = require('./package.json').version; var app = express(); +var log = require('logger')(__filename).log; +var version = require('./package.json').version; // view engine setup app.set('views', path.join(__dirname, 'views')); @@ -15,6 +17,35 @@ var locals = { version: version }; +// this is used for hello runnable user so we only have to login once +var superUser = new Runnable(process.env.API_HOST, { + requestDefaults: { + headers: { + 'user-agent': 'detention-root' + }, + } +}); + +/** + * + */ +api.loginSuperUser = function (cb) { + var logData = { + tx: true + }; + log.info(logData, 'api.loginSuperUser'); + superUser.githubLogin(process.env.HELLO_RUNNABLE_GITHUB_TOKEN, function (err) { + if (err) { + log.error(put({ + err: err + }, logData), 'loginSuperUser error'); + } else { + log.trace(logData, 'loginSuperUser success'); + } + cb(err); + }); +}; + // uncomment after placing your favicon in /public app.use(bodyParser.json()); app.use(bodyParser.urlencoded({ extended: false })); @@ -29,11 +60,8 @@ app.route('/*').get(function (req, res, next) { var page = req.query.type; [ 'status', - 'branchName', 'redirectUrl', - 'containerUrl', - 'ownerName', - 'instanceName' + 'shortHash' ].forEach(function (option) { options[option] = req.query[option]; }); diff --git a/package.json b/package.json index 66f1ca7..77f26ef 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,9 @@ "start": "node ./bin/www" }, "dependencies": { + "101": "^1.2.0", "body-parser": "~1.12.4", + "bunyan": "^1.5.1", "debug": "~2.2.0", "express": "~4.12.4", "grunt": "^0.4.5", @@ -18,6 +20,7 @@ "grunt-contrib-sass": "^0.9.2", "grunt-jade-plugin": "^0.6.0", "jade": "~1.9.2", + "keypather": "^1.10.1", "runnable": "git+ssh://git@github.com:CodeNow/runnable-api-client#v4.7.1", "serve-favicon": "~2.2.1" } From 74837e60401053447ff3b7e9b3f54e0a94f87929 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 12:39:45 +0800 Subject: [PATCH 03/44] Add logging utility to project --- logger.js | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 logger.js diff --git a/logger.js b/logger.js new file mode 100644 index 0000000..b3c3d68 --- /dev/null +++ b/logger.js @@ -0,0 +1,45 @@ +/** + * @module lib/logger + */ +'use strict'; +require('./loadenv.js'); + +var bunyan = require('bunyan'); +var envIs = require('101/env-is'); +var keypather = require('keypather')(); +var put = require('101/put'); + +var serializers = put(bunyan.stdSerializers, { + tx: function () { + var runnableData = keypather.get(process.domain, 'runnableData'); + if (!runnableData) { + runnableData = {}; + } + var date = new Date(); + if (runnableData.txTimestamp) { + // Save delta of time from previous log to this log + runnableData.txMSDelta = date.valueOf() - runnableData.txTimestamp.valueOf(); + } + runnableData.txTimestamp = date; + if (runnableData.reqStart) { + // Milliseconds from request start + runnableData.txMSFromReqStart = runnableData.txTimestamp.valueOf() - + runnableData.reqStart.valueOf(); + } + return runnableData; + } +}); + +module.exports = bunyan.createLogger({ + name: 'detention', + streams: [{ + level: process.env.LOG_LEVEL_STDOUT, + stream: process.stdout + }], + serializers: serializers, + // DO NOT use src in prod, slow + src: !envIs('production'), + environment: process.env.NODE_ENV +}); + +module.exports._serializers = serializers; From 2bc920d7902a8c3a50bdd0a42c51933ddebdee7c Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 12:46:54 +0800 Subject: [PATCH 04/44] Add npm dependencies for testing --- package.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/package.json b/package.json index 77f26ef..172edea 100644 --- a/package.json +++ b/package.json @@ -23,5 +23,9 @@ "keypather": "^1.10.1", "runnable": "git+ssh://git@github.com:CodeNow/runnable-api-client#v4.7.1", "serve-favicon": "~2.2.1" + }, + "devDependencies": { + "code": "^2.0.1", + "lab": "^7.3.0" } } From af96a8379d1dfe98ccab5f226a73857c98b6cfa5 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 12:51:24 +0800 Subject: [PATCH 05/44] Seed test files and package script --- package.json | 3 ++- test/app.js | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 test/app.js diff --git a/package.json b/package.json index 172edea..6f1736d 100644 --- a/package.json +++ b/package.json @@ -4,7 +4,8 @@ "private": true, "scripts": { "grunt": "./node_modules/.bin/grunt", - "start": "node ./bin/www" + "start": "node ./bin/www", + "test": "lab -c -v" }, "dependencies": { "101": "^1.2.0", diff --git a/test/app.js b/test/app.js new file mode 100644 index 0000000..ccc0036 --- /dev/null +++ b/test/app.js @@ -0,0 +1,16 @@ +/** + * @module test/app + */ +'use strict'; + +var Lab = require('lab'); +var Code = require('code'); + +var lab = exports.lab = Lab.script(); + +var describe = lab.describe; +var expect = Code.expect; +var it = lab.test; + +describe('app.js', function () { +}); From f2555a7811f8d3618d39c0d43a1b2fed9cc43443 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 12:54:24 +0800 Subject: [PATCH 06/44] Add sinon to project dependencies --- package.json | 3 ++- test/app.js | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 6f1736d..1b1ae2b 100644 --- a/package.json +++ b/package.json @@ -27,6 +27,7 @@ }, "devDependencies": { "code": "^2.0.1", - "lab": "^7.3.0" + "lab": "^7.3.0", + "sinon": "^1.17.2" } } diff --git a/test/app.js b/test/app.js index ccc0036..49837c4 100644 --- a/test/app.js +++ b/test/app.js @@ -13,4 +13,6 @@ var expect = Code.expect; var it = lab.test; describe('app.js', function () { + describe('app.loginSuperUser', function () { + }); }); From 5e5c9c19036d954535a4bcdaf4857773db6fc548 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 14:50:38 +0800 Subject: [PATCH 07/44] Add super user login precondition to server listen --- app.js | 14 ++++++++++++-- bin/www | 4 +++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/app.js b/app.js index 04fa80c..8c04a6c 100644 --- a/app.js +++ b/app.js @@ -1,3 +1,6 @@ +/** + * @module app + */ 'use strict'; var express = require('express'); @@ -27,7 +30,8 @@ var superUser = new Runnable(process.env.API_HOST, { }); /** - * + * Authenticate with API as super user + * Must invoke before server begins listening */ api.loginSuperUser = function (cb) { var logData = { @@ -46,12 +50,18 @@ api.loginSuperUser = function (cb) { }); }; +/** + * Fetch Instance resource from API + */ +api._fetchInstance = function (req, res, next) { +}; + // uncomment after placing your favicon in /public app.use(bodyParser.json()); app.use(bodyParser.urlencoded({ extended: false })); app.use(express.static(path.join(__dirname, 'public'))); -app.route('/*').get(function (req, res, next) { +app.route('/*').get(api._fetchInstance, function (req, res, next) { var options = { localVersion: version, absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' diff --git a/bin/www b/bin/www index 965f15e..c56d5f3 100755 --- a/bin/www +++ b/bin/www @@ -25,7 +25,9 @@ var server = http.createServer(app); * Listen on provided port, on all network interfaces. */ -server.listen(port); +app.loginSuperUser(function () { + server.listen(port); +}); server.on('error', onError); server.on('listening', onListening); From e8c3e28507424a15f766820e2afcecc44e7d9b3b Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 15:07:39 +0800 Subject: [PATCH 08/44] Fetch instance from API middleware --- app.js | 16 ++++++++++++++++ test/app.js | 3 ++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index 8c04a6c..29c6218 100644 --- a/app.js +++ b/app.js @@ -54,6 +54,22 @@ api.loginSuperUser = function (cb) { * Fetch Instance resource from API */ api._fetchInstance = function (req, res, next) { + log.info({ + shortHash: req.query.shortHash + }, 'api._fetchInstance'); + if (!req.query.shortHash) { + // only valid occurance if login error + return next(); + } + req.instance = superUser.fetchInstance(req.query.shortHash, function (err) { + if (err) { + log.error({ + err: err + }, '_fetchInstance superUser.fetchInstance error'); + } + log.trace('_fetchInstance superUser.fetchInstance success'); + next(); + }); }; // uncomment after placing your favicon in /public diff --git a/test/app.js b/test/app.js index 49837c4..cabe1d3 100644 --- a/test/app.js +++ b/test/app.js @@ -3,8 +3,9 @@ */ 'use strict'; -var Lab = require('lab'); var Code = require('code'); +var Lab = require('lab'); +var sinon = require('sinon'); var lab = exports.lab = Lab.script(); From ab191aa57d81b4c6e7bb18db2b89a373592cc496 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 15:24:44 +0800 Subject: [PATCH 09/44] Log module return child log instance --- app.js | 13 +++++++------ logger.js | 36 ++++++++++++++---------------------- 2 files changed, 21 insertions(+), 28 deletions(-) diff --git a/app.js b/app.js index 29c6218..dcd0408 100644 --- a/app.js +++ b/app.js @@ -3,13 +3,14 @@ */ 'use strict'; +var Runnable = require('runnable'); +var bodyParser = require('body-parser'); var express = require('express'); var path = require('path'); -var bodyParser = require('body-parser'); -var Runnable = require('runnable'); +var put = require('101/put'); var app = express(); -var log = require('logger')(__filename).log; +var log = require('./logger')(__filename); var version = require('./package.json').version; // view engine setup @@ -33,7 +34,7 @@ var superUser = new Runnable(process.env.API_HOST, { * Authenticate with API as super user * Must invoke before server begins listening */ -api.loginSuperUser = function (cb) { +app.loginSuperUser = function (cb) { var logData = { tx: true }; @@ -53,7 +54,7 @@ api.loginSuperUser = function (cb) { /** * Fetch Instance resource from API */ -api._fetchInstance = function (req, res, next) { +app._fetchInstance = function (req, res, next) { log.info({ shortHash: req.query.shortHash }, 'api._fetchInstance'); @@ -77,7 +78,7 @@ app.use(bodyParser.json()); app.use(bodyParser.urlencoded({ extended: false })); app.use(express.static(path.join(__dirname, 'public'))); -app.route('/*').get(api._fetchInstance, function (req, res, next) { +app.route('/*').get(app._fetchInstance, function (req, res, next) { var options = { localVersion: version, absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' diff --git a/logger.js b/logger.js index b3c3d68..3857fcc 100644 --- a/logger.js +++ b/logger.js @@ -2,38 +2,20 @@ * @module lib/logger */ 'use strict'; -require('./loadenv.js'); var bunyan = require('bunyan'); var envIs = require('101/env-is'); -var keypather = require('keypather')(); +var path = require('path'); var put = require('101/put'); var serializers = put(bunyan.stdSerializers, { - tx: function () { - var runnableData = keypather.get(process.domain, 'runnableData'); - if (!runnableData) { - runnableData = {}; - } - var date = new Date(); - if (runnableData.txTimestamp) { - // Save delta of time from previous log to this log - runnableData.txMSDelta = date.valueOf() - runnableData.txTimestamp.valueOf(); - } - runnableData.txTimestamp = date; - if (runnableData.reqStart) { - // Milliseconds from request start - runnableData.txMSFromReqStart = runnableData.txTimestamp.valueOf() - - runnableData.reqStart.valueOf(); - } - return runnableData; - } + // put serializers here }); -module.exports = bunyan.createLogger({ +var logger = bunyan.createLogger({ name: 'detention', streams: [{ - level: process.env.LOG_LEVEL_STDOUT, + level: process.env.LOG_LEVEL_STDOUT || 'trace', stream: process.stdout }], serializers: serializers, @@ -42,4 +24,14 @@ module.exports = bunyan.createLogger({ environment: process.env.NODE_ENV }); +/** + * Return a new child bunyan instance + * @param {String} namespace + */ +module.exports = function (namespace) { + return logger.child({ + module: path.relative(process.cwd(), namespace) + }); +} + module.exports._serializers = serializers; From db767af1e4c65f5d0963d1700de424b7171f86e1 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 15:32:27 +0800 Subject: [PATCH 10/44] Add trace log --- app.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app.js b/app.js index dcd0408..ec8d002 100644 --- a/app.js +++ b/app.js @@ -60,6 +60,7 @@ app._fetchInstance = function (req, res, next) { }, 'api._fetchInstance'); if (!req.query.shortHash) { // only valid occurance if login error + log.trace('_fetchInstance !shortHash'); return next(); } req.instance = superUser.fetchInstance(req.query.shortHash, function (err) { From ab64255fd2a6116a68a557adf7c2ca79d87e374e Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 15:35:45 +0800 Subject: [PATCH 11/44] Add start check for required ENV keys --- bin/www | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/bin/www b/bin/www index c56d5f3..acdf495 100755 --- a/bin/www +++ b/bin/www @@ -4,10 +4,22 @@ * Module dependencies. */ -var app = require('../app'); var debug = require('debug')('detention:server'); +var hasKeypaths = require('101/has-keypaths'); var http = require('http'); +var app = require('../app'); + +var requiredEnvKeys = [ + 'HELLO_RUNNABLE_GITHUB_TOKEN', + 'API_HOST' +]; +if (!hasKeypaths(process.env, requiredEnvKeys)) { + console.log('Missing required ENV keys'); + console.log(requiredEnvKeys); + process.exit(1); +} + /** * Get port from environment and store in Express. */ From b14069035be7d694d143a6f4d1c59be6b88b546b Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 17:07:55 +0800 Subject: [PATCH 12/44] Add error page generation logic for instance status --- app.js | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 11 deletions(-) diff --git a/app.js b/app.js index ec8d002..d0695f7 100644 --- a/app.js +++ b/app.js @@ -6,6 +6,7 @@ var Runnable = require('runnable'); var bodyParser = require('body-parser'); var express = require('express'); +var keypather = require('keypather')(); var path = require('path'); var put = require('101/put'); @@ -79,20 +80,86 @@ app.use(bodyParser.json()); app.use(bodyParser.urlencoded({ extended: false })); app.use(express.static(path.join(__dirname, 'public'))); -app.route('/*').get(app._fetchInstance, function (req, res, next) { +app.route('/*').get(app._fetchInstance, function processInstance (req, res, next) { + log.info({ + query: req.query + }, 'processInstance'); var options = { localVersion: version, absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' }; - if (req.query.type) { - var page = req.query.type; - [ - 'status', - 'redirectUrl', - 'shortHash' - ].forEach(function (option) { - options[option] = req.query[option]; - }); + + [ + 'redirectUrl', + 'shortHash' + ].forEach(function (option) { + options[option] = req.query[option]; + }); + + if (req.query.type === 'signin') { + log.trace('processInstance type signin') + return res.render('pages/signin', options); + } else if (req.query.type === 'not_running') { + log.trace('processInstance type !signin') + + if (!req.instance) { + log.error('instance not found'); + return next(new Error('instance not found')); + } + + options.branchName = keypather.get(req.instance, 'attrs.contextVersion.branch'); + // Temp missing pending resolution of SAN-3018 + // https://runnable.atlassian.net/browse/SAN-3018 + options.instanceName = keypather.get(req.instance, 'attrs.contextVersion.branch'); + options.ownerName = keypather.get(req.instance, 'attrs.owner.username'); + + // container state error pages. + // - Not running (building, starting, crashed) + // - Running, but unresponsive + var status = req.instance.status(); + log.trace({ + status: status, + options: options + }, 'processInstance instance status'); + + options.status = status; + switch(status) { + case 'stopped': + case 'crashed': + case 'stopping': + options.headerText = 'is ' + status; + res.render('pages/dead', options); + break; + case 'running': + // The instance could have started after Navi fetched it and proxied to detention. + // Might not be the best idea to trigger a refresh, could easily result in user-unfriendly + // infinite redirect loops. Better to display an error page prompting user to refresh? + options.headerText = 'is running'; + res.render('pages/dead', options); + break; + case 'buildFailed': + options.headerText = 'build failed'; + res.render('pages/dead', options); + break; + case 'building': + options.headerText = 'is building'; + res.render('pages/dead', options); + break; + case 'neverStarted': + case 'starting': + options.headerText = 'is starting'; + res.render('pages/dead', options); + break; + case 'unknown': + options.headerText = 'unknown'; + res.render('pages/dead', options); + break; + } + } else { + // ports, unresponsive + } + +/* if (req.query.ports) { var value = req.query.ports; if (!Array.isArray(value)) { @@ -109,10 +176,11 @@ app.route('/*').get(app._fetchInstance, function (req, res, next) { options.status = 'is ' + options.status; } } - res.render('pages/' + page, options); } else { res.render('pages/invalid', options); } +*/ + }); // catch 404 and forward to error handler From c815c1d868c053d2ac6956b9d85bd0b63865775c Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 26 Nov 2015 18:05:22 +0800 Subject: [PATCH 13/44] Fix jshint error --- app.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app.js b/app.js index d0695f7..2f75f0a 100644 --- a/app.js +++ b/app.js @@ -97,10 +97,10 @@ app.route('/*').get(app._fetchInstance, function processInstance (req, res, next }); if (req.query.type === 'signin') { - log.trace('processInstance type signin') + log.trace('processInstance type signin'); return res.render('pages/signin', options); } else if (req.query.type === 'not_running') { - log.trace('processInstance type !signin') + log.trace('processInstance type !signin'); if (!req.instance) { log.error('instance not found'); From 5e78b7954cd8370dfd855f75bacf920736c0aa57 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 11:14:34 +0800 Subject: [PATCH 14/44] Add error-cat dep to project and use with invalid request params --- app.js | 5 +++-- package.json | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app.js b/app.js index 2f75f0a..ade3ef6 100644 --- a/app.js +++ b/app.js @@ -3,6 +3,7 @@ */ 'use strict'; +var ErrorCat = require('error-cat'); var Runnable = require('runnable'); var bodyParser = require('body-parser'); var express = require('express'); @@ -62,7 +63,8 @@ app._fetchInstance = function (req, res, next) { if (!req.query.shortHash) { // only valid occurance if login error log.trace('_fetchInstance !shortHash'); - return next(); + // TODO?: switch to createAndReport + return next(ErrorCat.create(500, 'instance shortHash required')); } req.instance = superUser.fetchInstance(req.query.shortHash, function (err) { if (err) { @@ -200,5 +202,4 @@ app.use(function(err, req, res, next) { }); }); - module.exports = app; diff --git a/package.json b/package.json index 1b1ae2b..f24c9a0 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,8 @@ "jade": "~1.9.2", "keypather": "^1.10.1", "runnable": "git+ssh://git@github.com:CodeNow/runnable-api-client#v4.7.1", - "serve-favicon": "~2.2.1" + "serve-favicon": "~2.2.1", + "error-cat": "~1.4.0" }, "devDependencies": { "code": "^2.0.1", From 0b64376c49cbf5e85abe43de2b9e3496d95d7438 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 11:30:54 +0800 Subject: [PATCH 15/44] Invoke next with error if instance not found --- app.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app.js b/app.js index ade3ef6..72c69b7 100644 --- a/app.js +++ b/app.js @@ -71,6 +71,8 @@ app._fetchInstance = function (req, res, next) { log.error({ err: err }, '_fetchInstance superUser.fetchInstance error'); + // TODO?: switch to createAndReport + return next(ErrorCat.create(404, 'instance not found')); } log.trace('_fetchInstance superUser.fetchInstance success'); next(); From 8b78d96e94f4e9ff9cce772f65a2ed5c0a9b3fc7 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 11:42:04 +0800 Subject: [PATCH 16/44] Use expressjs view engine local vars --- app.js | 16 ++++++---------- views/index.jade | 6 +++--- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/app.js b/app.js index 72c69b7..4fb578c 100644 --- a/app.js +++ b/app.js @@ -19,9 +19,10 @@ var version = require('./package.json').version; app.set('views', path.join(__dirname, 'views')); app.set('view engine', 'jade'); -var locals = { - version: version -}; +app.locals({ + localVersionversion: version, + absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' +}); // this is used for hello runnable user so we only have to login once var superUser = new Runnable(process.env.API_HOST, { @@ -88,10 +89,7 @@ app.route('/*').get(app._fetchInstance, function processInstance (req, res, next log.info({ query: req.query }, 'processInstance'); - var options = { - localVersion: version, - absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' - }; + var options = {}; [ 'redirectUrl', @@ -199,9 +197,7 @@ app.use(function(req, res, next) { // production error handler // no stacktraces leaked to user app.use(function(err, req, res, next) { - res.render('pages/invalid', { - localVersion: version - }); + res.render('pages/invalid', {}); }); module.exports = app; diff --git a/views/index.jade b/views/index.jade index 370e5fb..bb095ec 100644 --- a/views/index.jade +++ b/views/index.jade @@ -13,13 +13,13 @@ html( title Uh oh //- css link( - href = "//#{locals.absoluteUrl}/stylesheets/error.css?v=#{locals.localVersion}" + href = "//#{absoluteUrl}/stylesheets/error.css?v=#{localVersion}" media = "screen" rel = "stylesheet" ) //- favicon link( - href = "//#{locals.absoluteUrl}/images/favicon.png" + href = "//#{absoluteUrl}/images/favicon.png" rel = "shortcut icon" ) meta( @@ -49,4 +49,4 @@ html( body.error-wrapper .error-content - block content \ No newline at end of file + block content From 2d42b5420b7bb39669fb7f62935f8be836e32387 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 14:19:30 +0800 Subject: [PATCH 17/44] Use assign to extend app.locals --- app.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index 4fb578c..effb629 100644 --- a/app.js +++ b/app.js @@ -5,6 +5,7 @@ var ErrorCat = require('error-cat'); var Runnable = require('runnable'); +var assign = require('101/assign'); var bodyParser = require('body-parser'); var express = require('express'); var keypather = require('keypather')(); @@ -19,7 +20,8 @@ var version = require('./package.json').version; app.set('views', path.join(__dirname, 'views')); app.set('view engine', 'jade'); -app.locals({ + +assign(app.locals, { localVersionversion: version, absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' }); From 8972fe4b7cdf12107ec3012d28ff6ff8e06a1316 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 15:08:35 +0800 Subject: [PATCH 18/44] Reorganize functions into private module properties for unit tests --- app.js | 58 ++++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/app.js b/app.js index effb629..75b86a8 100644 --- a/app.js +++ b/app.js @@ -16,11 +16,18 @@ var app = express(); var log = require('./logger')(__filename); var version = require('./package.json').version; +// valid Detention request types (val for req validation) +var validDetentionTypes = [ + 'not_running', + 'ports', + 'signin', + 'unresponsive' +]; + // view engine setup app.set('views', path.join(__dirname, 'views')); app.set('view engine', 'jade'); - assign(app.locals, { localVersionversion: version, absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' @@ -56,6 +63,24 @@ app.loginSuperUser = function (cb) { }); }; +/** + * Validate query parameters + */ +app._validateRequest = function (req, res, next) { + log.info('app._validateRequest'); + if (!req.query.shortHash && req.query.type !== 'signin') { + log.trace('_validateRequest !shortHash'); + // TODO?: switch to createAndReport + return next(ErrorCat.create(500, 'instance shortHash required')); + } + if (~validDetentionTypes.indexOf(req.query.type)) { + // only valid occurance if login error + log.trace('_validateRequest !type'); + // TODO?: switch to createAndReport + return next(ErrorCat.create(500, 'invalid request type')); + } +}; + /** * Fetch Instance resource from API */ @@ -63,11 +88,9 @@ app._fetchInstance = function (req, res, next) { log.info({ shortHash: req.query.shortHash }, 'api._fetchInstance'); - if (!req.query.shortHash) { - // only valid occurance if login error - log.trace('_fetchInstance !shortHash'); - // TODO?: switch to createAndReport - return next(ErrorCat.create(500, 'instance shortHash required')); + if (req.query.type === 'signin') { + log.trace('_fetchInstance signin bypass'); + return next(); } req.instance = superUser.fetchInstance(req.query.shortHash, function (err) { if (err) { @@ -81,13 +104,12 @@ app._fetchInstance = function (req, res, next) { next(); }); }; - // uncomment after placing your favicon in /public -app.use(bodyParser.json()); -app.use(bodyParser.urlencoded({ extended: false })); -app.use(express.static(path.join(__dirname, 'public'))); -app.route('/*').get(app._fetchInstance, function processInstance (req, res, next) { +/** + * Resolve instance status and render + return relevant error message html page + */ +app._processNaviError = function (req, res, next) { log.info({ query: req.query }, 'processInstance'); @@ -185,7 +207,17 @@ app.route('/*').get(app._fetchInstance, function processInstance (req, res, next } */ -}); +}; + +app.use(bodyParser.json()); +app.use(bodyParser.urlencoded({ extended: false })); +app.use(express.static(path.join(__dirname, 'public'))); + +app.route('/*').get( + app._validateRequest, + app._fetchInstance, + app._processNaviError +); // catch 404 and forward to error handler app.use(function(req, res, next) { @@ -194,8 +226,6 @@ app.use(function(req, res, next) { next(err); }); -// error handlers - // production error handler // no stacktraces leaked to user app.use(function(err, req, res, next) { From 25b8f103d4cffd9837c02ffa499fd207d8687d3e Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 15:10:15 +0800 Subject: [PATCH 19/44] Fix missing invoke of next --- app.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index 75b86a8..dd9f867 100644 --- a/app.js +++ b/app.js @@ -73,12 +73,14 @@ app._validateRequest = function (req, res, next) { // TODO?: switch to createAndReport return next(ErrorCat.create(500, 'instance shortHash required')); } - if (~validDetentionTypes.indexOf(req.query.type)) { + if (!~validDetentionTypes.indexOf(req.query.type)) { // only valid occurance if login error log.trace('_validateRequest !type'); // TODO?: switch to createAndReport return next(ErrorCat.create(500, 'invalid request type')); } + log.trace('_validateRequest success'); + next(); }; /** From fc953de6c1aa0c5c66e7f31d7ac3186c876d4ada Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 15:11:34 +0800 Subject: [PATCH 20/44] Update log message prefixes --- app.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app.js b/app.js index dd9f867..eba4324 100644 --- a/app.js +++ b/app.js @@ -114,7 +114,7 @@ app._fetchInstance = function (req, res, next) { app._processNaviError = function (req, res, next) { log.info({ query: req.query - }, 'processInstance'); + }, 'processNaviError'); var options = {}; [ @@ -125,10 +125,10 @@ app._processNaviError = function (req, res, next) { }); if (req.query.type === 'signin') { - log.trace('processInstance type signin'); + log.trace('processNaviError type signin'); return res.render('pages/signin', options); } else if (req.query.type === 'not_running') { - log.trace('processInstance type !signin'); + log.trace('processNaviError type !signin'); if (!req.instance) { log.error('instance not found'); @@ -148,7 +148,7 @@ app._processNaviError = function (req, res, next) { log.trace({ status: status, options: options - }, 'processInstance instance status'); + }, 'processNaviError instance status'); options.status = status; switch(status) { From b1ed80b86f185030d7de8ebaf282749fb633c27c Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 15:26:58 +0800 Subject: [PATCH 21/44] Add if statement block for unresponsive query --- app.js | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/app.js b/app.js index eba4324..ad8747a 100644 --- a/app.js +++ b/app.js @@ -183,32 +183,19 @@ app._processNaviError = function (req, res, next) { res.render('pages/dead', options); break; } - } else { - // ports, unresponsive + } else if (req.query.type === 'ports'){ + /* + * Currently not implemented, might be bundled into 'unresponsive' + * + * Userland hipache will only route to navi if a request is made to an elastic url on a port + * that's explicitly set on the instance (we set hipache redis entries when ports are exposed) + * otherwise userland-hipache will return an error page due to a lack of a redis entry. + * + * Anand if you read this Monday morning lets chat about it at 3pm + */ + } else if (req.query.type === 'unresponsive'){ + res.render('pages/dead', options); } - -/* - if (req.query.ports) { - var value = req.query.ports; - if (!Array.isArray(value)) { - value = [value]; - } - options.ports = value; - } - options.headerText = options.status; - if (options.status) { - if (options.status === 'buildFailed') { - options.headerText = 'build failed.'; - options.status = 'failed to build'; - } else { - options.status = 'is ' + options.status; - } - } - } else { - res.render('pages/invalid', options); - } -*/ - }; app.use(bodyParser.json()); From a8f009808d53d87004af06f49fd91e1d0d921fbf Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 15:35:03 +0800 Subject: [PATCH 22/44] Update README --- README.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 92745cc..286c019 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,10 @@ -# detention -Handles our 404 +Detention +========= + +### Navi request general error message response producing service. + +Navi will proxy to this service in the event of several types of error scenarios. Detention fetches +the status of an instance from API and produces an error HTML response page. script to push it ``` From b1b83ddb71d126b28c1ba78e625111e9e2125f34 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 15:39:07 +0800 Subject: [PATCH 23/44] Update src comments --- app.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index ad8747a..4bf33c5 100644 --- a/app.js +++ b/app.js @@ -191,10 +191,13 @@ app._processNaviError = function (req, res, next) { * that's explicitly set on the instance (we set hipache redis entries when ports are exposed) * otherwise userland-hipache will return an error page due to a lack of a redis entry. * + * Probably could fix by patching Hipache or perhaps reading the manual to see if there's a + * some kind of forward-for-all-ports functionality + * * Anand if you read this Monday morning lets chat about it at 3pm */ } else if (req.query.type === 'unresponsive'){ - res.render('pages/dead', options); + res.render('pages/unresponsive', options); } }; From 68764a47ddcca586a83e828320b0ec3771cbb5e7 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 15:48:45 +0800 Subject: [PATCH 24/44] Add trace logs and add list of port numbers to view data --- app.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/app.js b/app.js index 4bf33c5..a7e4efa 100644 --- a/app.js +++ b/app.js @@ -124,22 +124,24 @@ app._processNaviError = function (req, res, next) { options[option] = req.query[option]; }); - if (req.query.type === 'signin') { - log.trace('processNaviError type signin'); - return res.render('pages/signin', options); - } else if (req.query.type === 'not_running') { - log.trace('processNaviError type !signin'); - - if (!req.instance) { - log.error('instance not found'); - return next(new Error('instance not found')); - } - + if (req.instance) { options.branchName = keypather.get(req.instance, 'attrs.contextVersion.branch'); // Temp missing pending resolution of SAN-3018 // https://runnable.atlassian.net/browse/SAN-3018 options.instanceName = keypather.get(req.instance, 'attrs.contextVersion.branch'); options.ownerName = keypather.get(req.instance, 'attrs.owner.username'); + var ports = keypather.get(req.instance, 'attrs.container.ports'); + options.ports = Object.keys(ports).map(function (portKey) { + // Ex: '3000/tcp' --> '3000' + return portKey.replace(/\/tcp$/, ''); + }); + } + + if (req.query.type === 'signin') { + log.trace('processNaviError type signin'); + return res.render('pages/signin', options); + } else if (req.query.type === 'not_running') { + log.trace('processNaviError type not_running'); // container state error pages. // - Not running (building, starting, crashed) @@ -184,6 +186,7 @@ app._processNaviError = function (req, res, next) { break; } } else if (req.query.type === 'ports'){ + log.trace('processNaviError type ports'); /* * Currently not implemented, might be bundled into 'unresponsive' * @@ -197,6 +200,7 @@ app._processNaviError = function (req, res, next) { * Anand if you read this Monday morning lets chat about it at 3pm */ } else if (req.query.type === 'unresponsive'){ + log.trace('processNaviError type unresponsive'); res.render('pages/unresponsive', options); } }; From 9e39139724ae4eb8d7d6ef559183c5d6331f99e2 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Mon, 30 Nov 2015 17:04:59 +0800 Subject: [PATCH 25/44] Change API_HOST to API_HOSTNAME for greater consistency --- app.js | 2 +- bin/www | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app.js b/app.js index a7e4efa..721a3c2 100644 --- a/app.js +++ b/app.js @@ -34,7 +34,7 @@ assign(app.locals, { }); // this is used for hello runnable user so we only have to login once -var superUser = new Runnable(process.env.API_HOST, { +var superUser = new Runnable(process.env.API_HOSTNAME, { requestDefaults: { headers: { 'user-agent': 'detention-root' diff --git a/bin/www b/bin/www index acdf495..42041e7 100755 --- a/bin/www +++ b/bin/www @@ -12,7 +12,7 @@ var app = require('../app'); var requiredEnvKeys = [ 'HELLO_RUNNABLE_GITHUB_TOKEN', - 'API_HOST' + 'API_HOSTNAME' ]; if (!hasKeypaths(process.env, requiredEnvKeys)) { console.log('Missing required ENV keys'); From b45b1e1019d920e2874e4136aa18dd7e873b3fd5 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 15:46:10 +0800 Subject: [PATCH 26/44] Remove else after return --- app.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index 721a3c2..9b9cfb5 100644 --- a/app.js +++ b/app.js @@ -140,7 +140,8 @@ app._processNaviError = function (req, res, next) { if (req.query.type === 'signin') { log.trace('processNaviError type signin'); return res.render('pages/signin', options); - } else if (req.query.type === 'not_running') { + } + if (req.query.type === 'not_running') { log.trace('processNaviError type not_running'); // container state error pages. From 4e91d820576c6d9d4d199d0e13b46d666de2dcc6 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 15:47:46 +0800 Subject: [PATCH 27/44] Replace console logs with bunyan --- bin/www | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bin/www b/bin/www index 42041e7..86203c5 100755 --- a/bin/www +++ b/bin/www @@ -9,14 +9,17 @@ var hasKeypaths = require('101/has-keypaths'); var http = require('http'); var app = require('../app'); +var log = require('../logger')(__filename); var requiredEnvKeys = [ 'HELLO_RUNNABLE_GITHUB_TOKEN', 'API_HOSTNAME' ]; if (!hasKeypaths(process.env, requiredEnvKeys)) { - console.log('Missing required ENV keys'); - console.log(requiredEnvKeys); + log.error({ + requiredEnvKeys: requiredEnvKeys, + env: process.env + }, 'Missing required ENV keys'); process.exit(1); } From b9a3035e240a4e994d0f3c594229c0c9bfcf1220 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 15:51:12 +0800 Subject: [PATCH 28/44] Send error message to rollbar --- bin/www | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/www b/bin/www index 86203c5..ddcd530 100755 --- a/bin/www +++ b/bin/www @@ -4,6 +4,7 @@ * Module dependencies. */ +var ErrorCat = require('error-cat'); var debug = require('debug')('detention:server'); var hasKeypaths = require('101/has-keypaths'); var http = require('http'); @@ -20,6 +21,8 @@ if (!hasKeypaths(process.env, requiredEnvKeys)) { requiredEnvKeys: requiredEnvKeys, env: process.env }, 'Missing required ENV keys'); + // send to rollbar + ErrorCat.report(new Error('Detention missing required ENV values')); process.exit(1); } From e7e34bf040e26cc65ebc67682b8def8c946316d2 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 15:53:21 +0800 Subject: [PATCH 29/44] Switch if/else to be if/return --- app.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app.js b/app.js index 9b9cfb5..b1f29a6 100644 --- a/app.js +++ b/app.js @@ -186,7 +186,9 @@ app._processNaviError = function (req, res, next) { res.render('pages/dead', options); break; } - } else if (req.query.type === 'ports'){ + return; + } + if (req.query.type === 'ports'){ log.trace('processNaviError type ports'); /* * Currently not implemented, might be bundled into 'unresponsive' @@ -200,9 +202,12 @@ app._processNaviError = function (req, res, next) { * * Anand if you read this Monday morning lets chat about it at 3pm */ - } else if (req.query.type === 'unresponsive'){ + return; + } + if (req.query.type === 'unresponsive'){ log.trace('processNaviError type unresponsive'); res.render('pages/unresponsive', options); + return; } }; From 730e20d19bb6785ce6e02fe4b09badc04497a041 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 16:07:57 +0800 Subject: [PATCH 30/44] Add unit tests app.loginSuperUser --- app.js | 4 ++-- test/app.js | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/app.js b/app.js index b1f29a6..6586ea6 100644 --- a/app.js +++ b/app.js @@ -13,7 +13,7 @@ var path = require('path'); var put = require('101/put'); var app = express(); -var log = require('./logger')(__filename); +var log = app.log = require('./logger')(__filename); var version = require('./package.json').version; // valid Detention request types (val for req validation) @@ -34,7 +34,7 @@ assign(app.locals, { }); // this is used for hello runnable user so we only have to login once -var superUser = new Runnable(process.env.API_HOSTNAME, { +var superUser = app.superUser = new Runnable(process.env.API_HOSTNAME, { requestDefaults: { headers: { 'user-agent': 'detention-root' diff --git a/test/app.js b/test/app.js index cabe1d3..6079332 100644 --- a/test/app.js +++ b/test/app.js @@ -9,11 +9,52 @@ var sinon = require('sinon'); var lab = exports.lab = Lab.script(); +var afterEach = lab.afterEach; +var beforeEach = lab.beforeEach; var describe = lab.describe; var expect = Code.expect; var it = lab.test; +var app = require('../app'); + describe('app.js', function () { describe('app.loginSuperUser', function () { + beforeEach(function (done) { + sinon.stub(app.superUser, 'githubLogin').yieldsAsync(); + sinon.stub(app.log, 'error'); + sinon.stub(app.log, 'trace'); + done(); + }); + + afterEach(function (done) { + app.superUser.githubLogin.restore(); + app.log.error.restore(); + app.log.trace.restore(); + done(); + }); + + it('should authenticate with API as hello-runnable', function (done) { + app.loginSuperUser(function (err) { + expect(err).to.be.undefined(); + sinon.assert.callCount(app.superUser.githubLogin, 1); + sinon.assert.calledWith(app.superUser.githubLogin, process.env.HELLO_RUNNABLE_GITHUB_TOKEN); + sinon.assert.calledWith(app.log.trace, + sinon.match.any, 'loginSuperUser success'); + done(); + }) + }); + + it('should log if authentication errors', function (done) { + var error = new Error('api error'); + app.superUser.githubLogin.yieldsAsync(error); + app.loginSuperUser(function (err) { + expect(err).to.equal(error); + sinon.assert.callCount(app.superUser.githubLogin, 1); + sinon.assert.calledWith(app.superUser.githubLogin, process.env.HELLO_RUNNABLE_GITHUB_TOKEN); + sinon.assert.calledWith(app.log.error, + sinon.match.any, 'loginSuperUser error'); + done(); + }) + }); }); }); From d08f22b4b19be8cef3744e4be0a5ab162cf89611 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 16:18:46 +0800 Subject: [PATCH 31/44] Add unit tests app._validateRequest' --- test/app.js | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/test/app.js b/test/app.js index 6079332..5500a28 100644 --- a/test/app.js +++ b/test/app.js @@ -57,4 +57,44 @@ describe('app.js', function () { }) }); }); + + describe('app._validateRequest', function () { + it('should next with error if non-signin error with no shortHash', function (done) { + var req = { + query: { + type: 'not_running' // !signin + } + }; + app._validateRequest(req, {}, function (err) { + expect(err.message).to.equal('instance shortHash required'); + done(); + }); + }); + + it('should next with error if invalid query.type value', function (done) { + var req = { + query: { + shortHash: '5555', + type: '*^^$^#&$@' + } + }; + app._validateRequest(req, {}, function (err) { + expect(err.message).to.equal('invalid request type'); + done(); + }); + }); + + it('should next without error for valid request', function (done) { + var req = { + query: { + shortHash: '5555', + type: 'not_running' + } + }; + app._validateRequest(req, {}, function (err) { + expect(err).to.be.undefined(); + done(); + }); + }); + }); }); From a4ea96a61c277f5a68538a872baf8faa171b48a4 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 16:23:02 +0800 Subject: [PATCH 32/44] Ignore leaks in tests --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index f24c9a0..52d3c46 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "scripts": { "grunt": "./node_modules/.bin/grunt", "start": "node ./bin/www", - "test": "lab -c -v" + "test": "lab -c -v -t 0 --leaks" }, "dependencies": { "101": "^1.2.0", From 7b459829dfed04982181f210f55847fad7645d48 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 16:56:22 +0800 Subject: [PATCH 33/44] Add unit tests app._fetchInstance --- test/app.js | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/app.js b/test/app.js index 5500a28..b03951b 100644 --- a/test/app.js +++ b/test/app.js @@ -97,4 +97,64 @@ describe('app.js', function () { }); }); }); + + describe('app._fetchInstance', function () { + beforeEach(function (done) { + sinon.stub(app.superUser, 'fetchInstance'); + done(); + }); + + afterEach(function (done) { + app.superUser.fetchInstance.restore(); + done(); + }); + + it('should next without fetching if query is signin', function (done) { + var req = { + query: { + type: 'signin' + } + }; + app._fetchInstance(req, {}, function (err) { + expect(err).to.be.undefined(); + sinon.assert.notCalled(app.superUser.fetchInstance); + done(); + }); + }); + + it('should next with error if fetchInstance yields an error', function (done) { + var instance = {}; + app.superUser.fetchInstance.returns(instance).yieldsAsync(new Error('error')); + var req = { + query: { + shortHash: 'axcde', + type: 'not_running' + } + }; + app._fetchInstance(req, {}, function (err) { + expect(err.message).to.equal('instance not found'); + sinon.assert.callCount(app.superUser.fetchInstance, 1); + sinon.assert.calledWith(app.superUser.fetchInstance, 'axcde'); + done(); + }); + }); + + it('should fetch instance and assign to req.instance, then next', function (done) { + var instance = {}; + app.superUser.fetchInstance.returns(instance).yieldsAsync(); + var req = { + query: { + shortHash: 'axcde', + type: 'not_running' + } + }; + app._fetchInstance(req, {}, function (err) { + expect(err).to.be.undefined(); + sinon.assert.callCount(app.superUser.fetchInstance, 1); + sinon.assert.calledWith(app.superUser.fetchInstance, 'axcde'); + expect(req.instance).to.equal(instance); + done(); + }); + }); + }); }); From eb33f5363f124129231b25fcf918924844e9ac64 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 17:24:40 +0800 Subject: [PATCH 34/44] Add unit tests app._processNaviError --- app.js | 2 +- test/app.js | 245 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 246 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index 6586ea6..4c0f508 100644 --- a/app.js +++ b/app.js @@ -128,7 +128,7 @@ app._processNaviError = function (req, res, next) { options.branchName = keypather.get(req.instance, 'attrs.contextVersion.branch'); // Temp missing pending resolution of SAN-3018 // https://runnable.atlassian.net/browse/SAN-3018 - options.instanceName = keypather.get(req.instance, 'attrs.contextVersion.branch'); + options.instanceName = keypather.get(req.instance, 'attrs.lowerName'); options.ownerName = keypather.get(req.instance, 'attrs.owner.username'); var ports = keypather.get(req.instance, 'attrs.container.ports'); options.ports = Object.keys(ports).map(function (portKey) { diff --git a/test/app.js b/test/app.js index b03951b..ac7d519 100644 --- a/test/app.js +++ b/test/app.js @@ -5,6 +5,7 @@ var Code = require('code'); var Lab = require('lab'); +var noop = require('101/noop'); var sinon = require('sinon'); var lab = exports.lab = Lab.script(); @@ -157,4 +158,248 @@ describe('app.js', function () { }); }); }); + + describe('app._processNaviError', function (done) { + var instance; + beforeEach(function (done) { + instance = { + attrs: { + lowerName: 'api', + owner: { + username: 'casey' + }, + contextVersion: { + branch: 'master' + }, + container: { + ports: { + '80/tcp': {} + } + } + } + }; + done(); + }); + + afterEach(function (done) { + done(); + }); + + it('should render signin page if error type is signin', function (done) { + var req = { + instance: instance, + query: { + type: 'signin', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/signin'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'] + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + it('should render not_running state stopped', function (done) { + instance.status = function () { + return 'stopped'; + }; + var req = { + instance: instance, + query: { + type: 'not_running', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/dead'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'], + headerText: 'is stopped' + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + it('should render not_running state running', function (done) { + instance.status = function () { + return 'running'; + }; + var req = { + instance: instance, + query: { + type: 'not_running', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/dead'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'], + headerText: 'is running' + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + it('should render not_running state buildFailed', function (done) { + instance.status = function () { + return 'buildFailed'; + }; + var req = { + instance: instance, + query: { + type: 'not_running', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/dead'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'], + headerText: 'build failed' + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + it('should render not_running state building', function (done) { + instance.status = function () { + return 'building'; + }; + var req = { + instance: instance, + query: { + type: 'not_running', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/dead'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'], + headerText: 'is building' + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + it('should render not_running state starting', function (done) { + instance.status = function () { + return 'neverStarted'; + }; + var req = { + instance: instance, + query: { + type: 'not_running', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/dead'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'], + headerText: 'is starting' + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + it('should render not_running state starting', function (done) { + instance.status = function () { + return 'neverStarted'; + }; + var req = { + instance: instance, + query: { + type: 'not_running', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/dead'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'], + headerText: 'is starting' + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + it('should render unresponsive error if type is unresponsive', function (done) { + var req = { + instance: instance, + query: { + type: 'unresponsive', + shortHash: 'axcde' + } + }; + var res = { + render: function (page, opts) { + expect(page).to.equal('pages/unresponsive'); + expect(opts).to.deep.contain({ + shortHash: 'axcde', + branchName: 'master', + instanceName: 'api', + ownerName: 'casey', + ports: ['80'] + }); + done(); + } + }; + app._processNaviError(req, res, noop); + }); + + }); }); From 55a5e2569f0bb9b0289ff1feb944a582f3ffe271 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Wed, 2 Dec 2015 17:27:35 +0800 Subject: [PATCH 35/44] Add circle.yml test file --- circle.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 circle.yml diff --git a/circle.yml b/circle.yml new file mode 100644 index 0000000..ba431f8 --- /dev/null +++ b/circle.yml @@ -0,0 +1,15 @@ +machine: + environment: + NODE_ENV: test + LOG_LEVEL_STDOUT: error +dependencies: + override: + - nvm install 4.2.0 + - nvm alias default 4.2.0 + - npm install -g npm@2.8.3 + - npm install +test: + pre: + - ulimit -n 10240 + override: + - npm run test From a3283df119ebd0c1a889c779e8a2106b32d8cc21 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 3 Dec 2015 07:46:48 +0800 Subject: [PATCH 36/44] Remove defaulting assign logic --- app.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app.js b/app.js index 4c0f508..6e89419 100644 --- a/app.js +++ b/app.js @@ -30,7 +30,7 @@ app.set('view engine', 'jade'); assign(app.locals, { localVersionversion: version, - absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' + absoluteUrl: process.env.ABSOLUTE_URL }); // this is used for hello runnable user so we only have to login once From 9591b25cd93c3c0266cc69428ce6c0bbbf4f0ffc Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 3 Dec 2015 07:56:38 +0800 Subject: [PATCH 37/44] Add rollbar report wait process exit --- bin/www | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/www b/bin/www index ddcd530..aeabb9f 100755 --- a/bin/www +++ b/bin/www @@ -22,8 +22,9 @@ if (!hasKeypaths(process.env, requiredEnvKeys)) { env: process.env }, 'Missing required ENV keys'); // send to rollbar - ErrorCat.report(new Error('Detention missing required ENV values')); - process.exit(1); + ErrorCat.report(new Error('Detention missing required ENV values'), null, function () { + process.exit(1); + }); } /** From 83a6ad7967253972d36ffc08de5b112d068b2585 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 3 Dec 2015 07:58:20 +0800 Subject: [PATCH 38/44] Remove serializers put extension --- logger.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/logger.js b/logger.js index 3857fcc..eba3433 100644 --- a/logger.js +++ b/logger.js @@ -8,17 +8,13 @@ var envIs = require('101/env-is'); var path = require('path'); var put = require('101/put'); -var serializers = put(bunyan.stdSerializers, { - // put serializers here -}); - var logger = bunyan.createLogger({ name: 'detention', streams: [{ level: process.env.LOG_LEVEL_STDOUT || 'trace', stream: process.stdout }], - serializers: serializers, + serializers: bunyan.stdSerializers, // DO NOT use src in prod, slow src: !envIs('production'), environment: process.env.NODE_ENV From ae7769f7d8db21ce044ede8cc06b003d8d7f2333 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 3 Dec 2015 08:10:43 +0800 Subject: [PATCH 39/44] Fix undefined reference --- logger.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/logger.js b/logger.js index eba3433..cf558cd 100644 --- a/logger.js +++ b/logger.js @@ -29,5 +29,3 @@ module.exports = function (namespace) { module: path.relative(process.cwd(), namespace) }); } - -module.exports._serializers = serializers; From 181f42ee2859b8065b0124ffce1bdcd195515486 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 3 Dec 2015 16:31:56 +0800 Subject: [PATCH 40/44] Fix typo in app module --- app.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app.js b/app.js index 6e89419..09ef965 100644 --- a/app.js +++ b/app.js @@ -29,7 +29,7 @@ app.set('views', path.join(__dirname, 'views')); app.set('view engine', 'jade'); assign(app.locals, { - localVersionversion: version, + localVersion: version, absoluteUrl: process.env.ABSOLUTE_URL }); From cf117d14ed4db06d7cb2aa5c087750c0a3a85346 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Thu, 3 Dec 2015 16:33:24 +0800 Subject: [PATCH 41/44] Add port exception prevent logic --- app.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app.js b/app.js index 09ef965..b99fb82 100644 --- a/app.js +++ b/app.js @@ -131,10 +131,16 @@ app._processNaviError = function (req, res, next) { options.instanceName = keypather.get(req.instance, 'attrs.lowerName'); options.ownerName = keypather.get(req.instance, 'attrs.owner.username'); var ports = keypather.get(req.instance, 'attrs.container.ports'); - options.ports = Object.keys(ports).map(function (portKey) { - // Ex: '3000/tcp' --> '3000' - return portKey.replace(/\/tcp$/, ''); - }); + if (ports) { + options.ports = Object.keys(ports).map(function (portKey) { + // Ex: '3000/tcp' --> '3000' + return portKey.replace(/\/tcp$/, ''); + }); + } else { + log.warn({ + instance: req.instance + }, '_processNaviError instance !ports'); + } } if (req.query.type === 'signin') { From 4c6656c48dcc538ffe2c46861302f4033df98e38 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Fri, 4 Dec 2015 08:52:36 +0800 Subject: [PATCH 42/44] Refactor code to use instance.getBranchName() --- app.js | 2 +- test/app.js | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app.js b/app.js index b99fb82..d88ff34 100644 --- a/app.js +++ b/app.js @@ -125,7 +125,7 @@ app._processNaviError = function (req, res, next) { }); if (req.instance) { - options.branchName = keypather.get(req.instance, 'attrs.contextVersion.branch'); + options.branchName = req.instance.getBranchName(); // Temp missing pending resolution of SAN-3018 // https://runnable.atlassian.net/browse/SAN-3018 options.instanceName = keypather.get(req.instance, 'attrs.lowerName'); diff --git a/test/app.js b/test/app.js index ac7d519..edabc80 100644 --- a/test/app.js +++ b/test/app.js @@ -163,6 +163,7 @@ describe('app.js', function () { var instance; beforeEach(function (done) { instance = { + getBranchName: sinon.stub().returns('master'), attrs: { lowerName: 'api', owner: { @@ -203,6 +204,7 @@ describe('app.js', function () { ownerName: 'casey', ports: ['80'] }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; @@ -231,6 +233,7 @@ describe('app.js', function () { ports: ['80'], headerText: 'is stopped' }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; @@ -259,6 +262,7 @@ describe('app.js', function () { ports: ['80'], headerText: 'is running' }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; @@ -287,6 +291,7 @@ describe('app.js', function () { ports: ['80'], headerText: 'build failed' }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; @@ -315,6 +320,7 @@ describe('app.js', function () { ports: ['80'], headerText: 'is building' }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; @@ -343,6 +349,7 @@ describe('app.js', function () { ports: ['80'], headerText: 'is starting' }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; @@ -371,6 +378,7 @@ describe('app.js', function () { ports: ['80'], headerText: 'is starting' }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; @@ -395,6 +403,7 @@ describe('app.js', function () { ownerName: 'casey', ports: ['80'] }); + sinon.assert.callCount(req.instance.getBranchName, 1); done(); } }; From 1606b54c39e6c8b1ab04ebbb5f056a20e28203f8 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Sat, 5 Dec 2015 09:09:07 +0800 Subject: [PATCH 43/44] Change format of title in README --- README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index 286c019..79f1de7 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,4 @@ -Detention -========= +# Detention ### Navi request general error message response producing service. From 326844a88febd7332fb40236b74d9698abdb71b5 Mon Sep 17 00:00:00 2001 From: Casey Flynn Date: Sat, 5 Dec 2015 09:14:31 +0800 Subject: [PATCH 44/44] Temporarily hide links until bug resolved --- views/pages/dead.jade | 3 ++- views/pages/ports.jade | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/views/pages/dead.jade b/views/pages/dead.jade index 7b7cd75..cad5544 100644 --- a/views/pages/dead.jade +++ b/views/pages/dead.jade @@ -8,6 +8,7 @@ block content //- link goes to the relevant container in the containers view, - with build logs open if container is building - with CMD logs open if container is stopped or crashed +// a.link( href = "//runnable.io/#{ownerName}/#{instanceName}" - ) View logs \ No newline at end of file + ) View logs diff --git a/views/pages/ports.jade b/views/pages/ports.jade index ba36043..af0f80b 100644 --- a/views/pages/ports.jade +++ b/views/pages/ports.jade @@ -14,8 +14,7 @@ block content a.link( href = ":#{port}" ) #{port} - - +// a.link( href = "//runnable.io/#{ownerName}/configure/#{instanceName}" - ) Expose a port in Runnable \ No newline at end of file + ) Expose a port in Runnable