From 700be85487d25b0a6563ae8c3726caac31d7b907 Mon Sep 17 00:00:00 2001 From: damienrunnable Date: Fri, 10 Feb 2017 13:29:02 -0800 Subject: [PATCH 1/6] Revert "Revert "San 5594 whitelist all orgs"" This reverts commit 90e91d3606776185970e18c085dc63f1be03b074. --- lib/models/apis/github.js | 33 +++++++++++++++- unit/models/apis/github.js | 80 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/lib/models/apis/github.js b/lib/models/apis/github.js index ea0ca4f45..293a397fc 100644 --- a/lib/models/apis/github.js +++ b/lib/models/apis/github.js @@ -227,6 +227,35 @@ Github.prototype.getUserById = function (githubId, cb) { }, cb) } +/** + * Fetches all orgs recursively starting at page 1. + * + * @param {object} opts List of options passed in to fetch page + * @param {function} cb callback + * @private + */ +Github.prototype._getAllOrgs = function (opts, cb) { + opts = Object.assign({ per_page: 100 }, opts || {}) + let allOrgs = [] + + const fetchPage = (page) => { + return Promise.fromCallback((cb) => { + return this.user.getOrgs(Object.assign({}, opts, {page}), cb) + }) + .then((orgs) => { + if (orgs) { + allOrgs = allOrgs.concat(orgs) + if (orgs.length === opts.per_page) { + return fetchPage(page + 1) + } + } + return allOrgs + }) + } + + return fetchPage(1).asCallback(cb) +} + Github.prototype.getUserAuthorizedOrgs = function (cb) { logger.log.info('Github.prototype.getUserAuthorizedOrgs') if (!this.token) { @@ -236,8 +265,8 @@ Github.prototype.getUserAuthorizedOrgs = function (cb) { var userKey = keyPrefix + (this.token ? this.tokenHash : 'runnable') var userOrgsKey = new redisTypes.String(userKey + ':user:' + this.token + ':orgs') this._runQueryAgainstCache({ - query: this.user.getOrgs, - debug: 'this.user.getOrgs', + query: this._getAllOrgs.bind(this), + debug: 'this._getAllOrgs', stringKey: userOrgsKey }, cb) } diff --git a/unit/models/apis/github.js b/unit/models/apis/github.js index 44d2c205b..49587f5dc 100644 --- a/unit/models/apis/github.js +++ b/unit/models/apis/github.js @@ -392,4 +392,84 @@ describe('github: ' + moduleName, function () { }) }) }) // end createHooksAndKeys + + describe('_getAllOrgs', () => { + let github + + beforeEach((done) => { + github = new Github({token: 'some-token'}) + sinon.stub(github.user,'getOrgs').yields(null, []) + done() + }) + + it('should return no results', (done) => { + let opts = {} + github.user.getOrgs.yields(null, []); + github._getAllOrgs(opts, function (err, allOrgs) { + sinon.assert.calledOnce(github.user.getOrgs) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 100, page: 1}, sinon.match.func) + expect(allOrgs.length).to.equal(0) + done() + }) + + }) + + it('should return 99 results', (done) => { + let opts = {} + + github.user.getOrgs.yields(null, new Array(99)) + github._getAllOrgs(opts, (err, allOrgs) => { + sinon.assert.calledOnce(github.user.getOrgs) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 100, page: 1}, sinon.match.func) + expect(allOrgs.length).to.equal(99) + done() + }) + }) + + it('should return 100 results', (done) => { + let opts = {} + + github.user.getOrgs.onFirstCall().yields(null, new Array(100)) + github.user.getOrgs.onSecondCall().yields(null, []) + github._getAllOrgs(opts, (err, allOrgs) => { + sinon.assert.calledTwice(github.user.getOrgs) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 100, page: 1}, sinon.match.func) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 100, page: 2}, sinon.match.func) + expect(allOrgs.length).to.equal(100) + done() + }) + }) + + it('should return 201 results', (done) => { + let opts = {} + + github.user.getOrgs.onFirstCall().yields(null, new Array(100)) + github.user.getOrgs.onSecondCall().yields(null, new Array(100)) + github.user.getOrgs.onThirdCall().yields(null, new Array(1)) + github._getAllOrgs(opts, (err, allOrgs) => { + sinon.assert.calledThrice(github.user.getOrgs) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 100, page: 1}, sinon.match.func) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 100, page: 2}, sinon.match.func) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 100, page: 3}, sinon.match.func) + expect(allOrgs.length).to.equal(201) + done() + }) + }) + + it('should return 40 results, over 2 pages, with three calls', (done) => { + let opts = {per_page: 20} + + github.user.getOrgs.onFirstCall().yields(null, new Array(20)) + github.user.getOrgs.onSecondCall().yields(null, new Array(20)) + github.user.getOrgs.onThirdCall().yields(null, []) + github._getAllOrgs(opts, (err, allOrgs) => { + sinon.assert.calledThrice(github.user.getOrgs) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 20, page: 1}, sinon.match.func) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 20, page: 2}, sinon.match.func) + sinon.assert.calledWith(github.user.getOrgs, {per_page: 20, page: 3}, sinon.match.func) + expect(allOrgs.length).to.equal(40) + done() + }) + }) + }) }) From 401f7d088103e7dbae0bcd19385aa2e147c3aa4d Mon Sep 17 00:00:00 2001 From: damienrunnable Date: Fri, 10 Feb 2017 14:57:36 -0800 Subject: [PATCH 2/6] SAN-5594 Testing change to resolve internal server error caused by initial checkin --- lib/models/apis/github.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/models/apis/github.js b/lib/models/apis/github.js index 293a397fc..ebe77645f 100644 --- a/lib/models/apis/github.js +++ b/lib/models/apis/github.js @@ -237,6 +237,8 @@ Github.prototype.getUserById = function (githubId, cb) { Github.prototype._getAllOrgs = function (opts, cb) { opts = Object.assign({ per_page: 100 }, opts || {}) let allOrgs = [] + // TODO: Remove after testing complete + opts.per_page = 3 const fetchPage = (page) => { return Promise.fromCallback((cb) => { @@ -262,13 +264,8 @@ Github.prototype.getUserAuthorizedOrgs = function (cb) { var errorMsg = 'getUserAuthorizedOrgs should only be called with a user token' return cb(Boom.badImplementation(errorMsg)) } - var userKey = keyPrefix + (this.token ? this.tokenHash : 'runnable') - var userOrgsKey = new redisTypes.String(userKey + ':user:' + this.token + ':orgs') - this._runQueryAgainstCache({ - query: this._getAllOrgs.bind(this), - debug: 'this._getAllOrgs', - stringKey: userOrgsKey - }, cb) + + return this._getAllOrgs({}, cb) } /** From 4bedec8c2c8539c4f4f38a22ebe0de4c4005d346 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Mon, 13 Feb 2017 14:51:39 -0800 Subject: [PATCH 3/6] The cache needed the original array form Giterode, which had metadata on it --- lib/models/apis/github.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/models/apis/github.js b/lib/models/apis/github.js index ebe77645f..c5a041bcf 100644 --- a/lib/models/apis/github.js +++ b/lib/models/apis/github.js @@ -236,19 +236,21 @@ Github.prototype.getUserById = function (githubId, cb) { */ Github.prototype._getAllOrgs = function (opts, cb) { opts = Object.assign({ per_page: 100 }, opts || {}) - let allOrgs = [] // TODO: Remove after testing complete opts.per_page = 3 - const fetchPage = (page) => { + const fetchPage = (page, allOrgs) => { return Promise.fromCallback((cb) => { return this.user.getOrgs(Object.assign({}, opts, {page}), cb) }) .then((orgs) => { + if (!allOrgs) { + allOrgs = orgs + } if (orgs) { - allOrgs = allOrgs.concat(orgs) + orgs.forEach(org => allOrgs.push) if (orgs.length === opts.per_page) { - return fetchPage(page + 1) + return fetchPage(page + 1, allOrgs) } } return allOrgs @@ -264,8 +266,13 @@ Github.prototype.getUserAuthorizedOrgs = function (cb) { var errorMsg = 'getUserAuthorizedOrgs should only be called with a user token' return cb(Boom.badImplementation(errorMsg)) } - - return this._getAllOrgs({}, cb) + var userKey = keyPrefix + (this.token ? this.tokenHash : 'runnable') + var userOrgsKey = new redisTypes.String(userKey + ':user:' + this.token + ':orgs') + return this._runQueryAgainstCache({ + query: this._getAllOrgs.bind(this), + debug: 'this._getAllOrgs', + stringKey: userOrgsKey + }, cb) } /** From a8fc347ef3c82f3cd7df4e1638ae3546ed7ed5c5 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Mon, 13 Feb 2017 15:19:42 -0800 Subject: [PATCH 4/6] Oops, I was adding the orgs twice --- lib/models/apis/github.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/models/apis/github.js b/lib/models/apis/github.js index c5a041bcf..32a3cd9c1 100644 --- a/lib/models/apis/github.js +++ b/lib/models/apis/github.js @@ -244,14 +244,16 @@ Github.prototype._getAllOrgs = function (opts, cb) { return this.user.getOrgs(Object.assign({}, opts, {page}), cb) }) .then((orgs) => { + if (!orgs) { + return orgs + } if (!allOrgs) { allOrgs = orgs - } - if (orgs) { + } else { orgs.forEach(org => allOrgs.push) - if (orgs.length === opts.per_page) { - return fetchPage(page + 1, allOrgs) - } + } + if (orgs.length === opts.per_page) { + return fetchPage(page + 1, allOrgs) } return allOrgs }) From 16b5d8ccba5a5543d9163f374e2c15d4c8cb3c3a Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Wed, 22 Feb 2017 15:41:46 -0800 Subject: [PATCH 5/6] Adding unit tests Adding fields to the dependencies model --- lib/models/mongo/instance.js | 143 +++++++++++------ lib/models/mongo/schemas/instance.js | 3 + test/integration/models/mongo/instance.js | 67 ++++++++ unit/models/mongo/instances.js | 178 +++++++++++++++++++--- 4 files changed, 325 insertions(+), 66 deletions(-) diff --git a/lib/models/mongo/instance.js b/lib/models/mongo/instance.js index 69b828d6a..9ab6de51e 100644 --- a/lib/models/mongo/instance.js +++ b/lib/models/mongo/instance.js @@ -724,12 +724,16 @@ InstanceSchema.methods.getElasticHostname = function (ownerUsername) { }) } -InstanceSchema.methods.generateGraphNode = function (ignoreIsolationProperty) { - return { - elasticHostname: this.elasticHostname, +InstanceSchema.methods.generateGraphNode = function (alias) { + let val = { + elasticHostname: alias || this.elasticHostname, instanceId: this._id, name: this.name } + if (alias) { + val.isAlias = true + } + return val } /** @@ -844,6 +848,18 @@ function findMatchingInstanceByHostname (possibleHostnameString, instanceList) { }) } +/** + * Searches through a list of instances to find the first one that matches the context + * @param {String} contextId - ObjectId (as string) of the contextId to match + * @param {[Instance]} instanceList - Instances to search to find one matching the contextId + * @returns {Instance|null} instance with the given hostname + */ +function findMatchingInstanceByContextId (contextId, instanceList) { + return find(instanceList, function (instance) { + return keypather.get(instance, 'contextVersion.context.toString()') === contextId + }) +} + /** * Used for finding instances for new connections. This searches for either masterpods or isolated * containers, and adds the instance's hostname to itself. If the instance is not isolated, and @@ -901,6 +917,52 @@ InstanceSchema.methods.getHostnamesFromEnvsAndFnr = function () { return linkedHostnames } +InstanceSchema.methods.getAliases = function () { + var log = logger.log.child({ + instanceId: keypather.get(this, '_id'), + instanceName: keypather.get(this, 'name'), + method: 'InstanceSchema.methods.getAliases' + }) + log.info('called') + + return [] +} + +/** + * + * @param {Object} instancesOpts + * @param {Instance[]} instancesOpts.dependentInstances + * @param {Instance[]} instancesOpts.isolatedInstances + * @param {Instance[]} instancesOpts.masterInstances + * @param {Boolean=} isAlias + * + * @returns {Function} + * @private + */ +InstanceSchema.statics._createGraphNodesFromDeps = function (instancesOpts, isAlias) { + var dependentInstances = instancesOpts.dependentInstances // existing dependencies + var isolatedInstances = instancesOpts.isolatedInstances // instances in the same isolation group + var masterInstances = instancesOpts.masterInstances + // We need to do different kinds of matching depending on if this is an existing instance + var matchingMethod = isAlias ? findMatchingInstanceByContextId : findMatchingInstanceByHostname + return function (value) { + var val = isAlias ? value.contextId : value + var alias = isAlias ? value.alias : null + + var instance = matchingMethod(val, dependentInstances) + // first check the isolated instances. + if (!instance && isolatedInstances.length) { + instance = matchingMethod(val, isolatedInstances) + } + if (!instance) { + instance = matchingMethod(val, masterInstances) + } + if (instance) { + return instance.generateGraphNode(alias) + } + } +} + /** * Looks at the envs and it's current FnR rules, and finds all of the possible runnable hostnames in * them. Once it has all of those, it matches those hostnames to any of it's fellow isolation @@ -923,43 +985,29 @@ InstanceSchema.methods.setDependenciesFromEnvironment = function (ownerUsername, ownerUsername = ownerUsername.toLowerCase() Promise.props({ - dependencies: self.getDependenciesAsync(), + existingGraphNodes: self.dependencies.map(dep => dep.toJSON()), + dependentInstances: self.getDependenciesAsync(), masterInstances: self.fetchMatchingInstancesForDepChecking(ownerUsername), isolatedInstances: self.fetchMatchingInstancesForDepChecking(ownerUsername, true) }) .then(function (results) { - var deps = results.dependencies // existing dependencies - var isolated = results.isolatedInstances // instances in the same isolation group - var masters = results.masterInstances - + var existingDeps = results.existingGraphNodes // existing dependencies var linkedHostnames = self.getHostnamesFromEnvsAndFnr() .filter(function filterOutSelf (hostname) { return !doesStringContainInstanceHostname(hostname, self) }) + var linkedAliases = self.getAliases() - var envDeps = linkedHostnames.map(function findAllMatchingInstancesByHostname (val) { - var instance = null - // Always add the original deps first so we don't destroy the current connections - var dep = findMatchingInstanceByHostname(val, deps) - if (dep) { - return dep - } - // first check the isolated instances. - if (self.isolated) { - instance = findMatchingInstanceByHostname(val, isolated) - } - if (!instance) { - instance = findMatchingInstanceByHostname(val, masters) - } - if (instance) { - // maybe add this dep if doesn't already exist - return instance - } - }).filter(exists) + var envDeps = linkedHostnames.map(hostname => Instance._createGraphNodesFromDeps(results)(hostname)) + var aliasDeps = linkedAliases.map(alias => Instance._createGraphNodesFromDeps(results, true)(alias)) + + var allDeps = envDeps + .concat(aliasDeps) + .filter(exists) // check existing deps, to determine which to add and remove var subtract = new Subtract(depsEqual) - var remDeps = subtract.sub(deps, envDeps) - var addDeps = subtract.sub(envDeps, deps) + var remDeps = subtract.sub(existingDeps, allDeps) + var addDeps = subtract.sub(allDeps, existingDeps) log.trace({ remDeps: remDeps, @@ -970,7 +1018,7 @@ InstanceSchema.methods.setDependenciesFromEnvironment = function (ownerUsername, return self.addDependency(dep) } function toRemTask (dep) { - return self.removeDependency(dep._id) + return self.removeDependency(dep.instanceId, dep.elasticHostname) } // convert addDeps and remDeps to tasks return Promise.all([ @@ -989,7 +1037,8 @@ function depsEqual (depA, depB) { // we assume deps have the same keys var keypaths = [ 'elasticHostname', - 'name' + 'name', + 'isAlias' ] return keypaths.every(function (keypath) { var valA = keypather.get(depA, keypath + '.toString().toLowerCase()') @@ -1001,15 +1050,15 @@ function depsEqual (depA, depB) { /** * Goes through a given instance's dependencies, looking for a match for the given elasticHostname * - * @param {Instance} instance - Instance with dependencies to search through - * @param {String} elasticHostname - elastic hostname to search + * @param {Instance} instance - Instance with dependencies to search through + * @param {ObjectId} instanceIdToFind - ObjectId of the instance to find * * @returns {GraphNode|null} Either the matching node for the given hostname, or null */ -function getDepFromInstance (instance, elasticHostname) { +function getDepFromInstance (instance, instanceIdToFind) { if (keypather.get(instance, 'dependencies.length')) { var deps = instance.dependencies.filter(function (dep) { - return dep.elasticHostname === elasticHostname + return dep.instanceId.toString() === instanceIdToFind.toString() }) return deps.length ? deps[0] : null } @@ -1018,7 +1067,9 @@ function getDepFromInstance (instance, elasticHostname) { /** * Adds the given instance to THIS instance's dependency list * - * @param {Instance} instance - instance to become a dependent + * @param {Instance|GraphNode} instance - Either the full instance to add, or the already-creaeted + * graph node to add to THIS instance. Best to send the + * graph node * * @returns {Promise} When the dependency has been added * @resolves {Instance} This instance, updated @@ -1027,6 +1078,9 @@ function getDepFromInstance (instance, elasticHostname) { * @throws {Error} Any Mongo error */ InstanceSchema.methods.addDependency = function (instance) { + if (instance.generateGraphNode) { + instance = instance.generateGraphNode() + } var elasticHostname = instance.elasticHostname.toLowerCase() var log = logger.log.child({ instanceId: keypather.get(this, '_id'), @@ -1056,7 +1110,7 @@ InstanceSchema.methods.addDependency = function (instance) { _id: thisInstance._id }, { $push: { - dependencies: instance.generateGraphNode() + dependencies: instance } }) }) @@ -1069,8 +1123,8 @@ InstanceSchema.methods.addDependency = function (instance) { }) } }) - .then(function (instance) { - return getDepFromInstance(instance, elasticHostname) + .then(function (updatedInstance) { + return getDepFromInstance(updatedInstance, instance._id) }) .finally(function () { self.invalidateContainerDNS() @@ -1081,19 +1135,22 @@ InstanceSchema.methods.addDependency = function (instance) { * Remove an instance dependency from THIS instance * * @param {ObjectId} instanceId - instance id to remove as a dependent + * @param hostnameOrAlias * * @return {Promise} When the dependency has been removed * @resolve {Instance} This instance, updated without the dependency * @throws {Boom.notFound} When this instance failed to update * @throws {Error} Mongo Errors */ -InstanceSchema.methods.removeDependency = function (instanceId) { +InstanceSchema.methods.removeDependency = function (instanceId, hostnameOrAlias) { var self = this + var query = { instanceId } + if (hostnameOrAlias) { + query.elasticHostname = hostnameOrAlias + } return Instance.findByIdAndUpdateAsync(this._id, { $pull: { - dependencies: { - instanceId: instanceId - } + dependencies: query } }) .tap(function (instance) { diff --git a/lib/models/mongo/schemas/instance.js b/lib/models/mongo/schemas/instance.js index 4d32cae93..f9d0eb5a5 100644 --- a/lib/models/mongo/schemas/instance.js +++ b/lib/models/mongo/schemas/instance.js @@ -258,6 +258,9 @@ var InstanceSchema = module.exports = new Schema({ }, name: { type: String + }, + isAlias: { + type: Boolean } }], 'default': [] diff --git a/test/integration/models/mongo/instance.js b/test/integration/models/mongo/instance.js index 1ca719aa2..aff7b405b 100644 --- a/test/integration/models/mongo/instance.js +++ b/test/integration/models/mongo/instance.js @@ -967,6 +967,73 @@ describe('Instance Model Integration Tests', function () { }) }) }) + describe('checking envs don\'t hurt alias in connections', function () { + beforeEach(function (done) { + createNewInstance('fb1-adelle', { + name: 'fb1-adelle', + masterPod: false, + branch: 'fb1', + cv: ctx.adelle.contextVersion + })(done) + }) + beforeEach(function (done) { + createNewInstance('fb1-goodbye', { + name: 'fb1-goodbye', + masterPod: false, + branch: 'fb1', + cv: ctx.goodbye.contextVersion + })(done) + }) + beforeEach(function (done) { + // Set the dep to a branch + ctx.hello.addDependency(ctx['fb1-adelle']).asCallback(done) + }) + it('should not remove the existing dep, when not adding any new ones', function (done) { + ctx.hello.setDependenciesFromEnvironment(ownerName, function (err) { + if (err) { + return done(err) + } + ctx.hello.getDependencies(function (err, dependencies) { + expect(dependencies).to.be.array() + expect(dependencies.length).to.equal(1) + expect(dependencies[0]._id).to.equal(ctx['fb1-adelle']._id) + expect(dependencies[0].elasticHostname).to.equal(ctx['fb1-adelle'].elasticHostname) + done(err) + }) + }) + }) + it('should add 1 new dep, and keep the existing one', function (done) { + ctx.hello.env.push([ + 'as=goodbye-staging-' + ownerName + '.runnableapp.com' + ]) + ctx.hello.setDependenciesFromEnvironment(ownerName, function (err) { + if (err) { + return done(err) + } + ctx.hello.getDependencies(function (err, dependencies) { + expect(dependencies).to.be.array() + expect(dependencies.length).to.equal(2) + var ids = dependencies.map(pluck('_id.toString()')) + expect(ids).to.include(ctx.goodbye._id.toString()) + expect(ids).to.include(ctx['fb1-adelle']._id.toString()) + done(err) + }) + }) + }) + it('should remove the only dependency', function (done) { + ctx.hello.env = [] + ctx.hello.setDependenciesFromEnvironment(ownerName, function (err) { + if (err) { + return done(err) + } + ctx.hello.getDependencies(function (err, dependencies) { + expect(dependencies).to.be.array() + expect(dependencies.length).to.equal(0) + done(err) + }) + }) + }) + }) describe('being isolated', function () { beforeEach(function (done) { diff --git a/unit/models/mongo/instances.js b/unit/models/mongo/instances.js index e913e52d7..fbe14cc53 100644 --- a/unit/models/mongo/instances.js +++ b/unit/models/mongo/instances.js @@ -790,7 +790,121 @@ describe('Instance Model Tests', function () { }) }) }) - + describe('Testing changes in aliases', function () { + var masterInstances + var contextId1 = 'asd' + var contextId2 = 'gsfdg' + var alias1 = { + contextId: contextId1, + alias: 'hello' + } + var alias2 = { + contextId: contextId2, + alias: 'hello.world' + } + var alias12 = { + contextId: contextId1, + alias: 'hello.world' + } + beforeEach(function (done) { + masterInstances = [ + mongoFactory.createNewInstance('hello', { + masterPod: true, + hostname: 'hello-staging-' + ownerName + '.runnableapp.com', + contextVersion: { + context: contextId1 + } + }), + mongoFactory.createNewInstance('adelle', { + masterPod: true, + hostname: 'adelle-staging-' + ownerName + '.runnableapp.com', + contextVersion: { + context: contextId2 + } + }) + ] + sinon.stub(Instance, 'find').yieldsAsync(null, masterInstances) + sinon.stub(instance, 'getAliases') + sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) + sinon.stub(instance, 'addDependency').resolves() + sinon.stub(instance, 'removeDependency').resolves() + done() + }) + afterEach(function (done) { + instance.addDependency.restore() + instance.removeDependency.restore() + instance.getAliases.restore() + done() + }) + describe('Aliases', function () { + it('should add a new dep for the alias', function (done) { + instance.getAliases.returns([alias1]) + instance.setDependenciesFromEnvironment(ownerName, function (err) { + if (err) { + return done(err) + } + sinon.assert.calledOnce(instance.invalidateContainerDNS) + sinon.assert.calledOnce(instance.addDependency) + sinon.assert.calledWith(instance.addDependency.getCall(0), + sinon.match.has('name', masterInstances[0].name) + ) + sinon.assert.notCalled(instance.removeDependency) + done() + }) + }) + it('should add a new dep, and keep the existing one', function (done) { + instance.dependencies = [{ + elasticHostname: alias2.alias, + instanceId: masterInstances[1]._id, + name: masterInstances[1].name, + isAlias: true + }] + instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) + instance.getAliases.returns([alias1, alias2]) + instance.setDependenciesFromEnvironment(ownerName, function (err) { + if (err) { + return done(err) + } + sinon.assert.calledOnce(instance.invalidateContainerDNS) + sinon.assert.calledOnce(instance.addDependency) + sinon.assert.calledWith(instance.addDependency.getCall(0), + sinon.match.has('name', masterInstances[0].name) + ) + sinon.assert.notCalled(instance.removeDependency) + done() + }) + }) + it('should add 2 new deps, and remove the existing one', function (done) { + instance.dependencies = [{ + elasticHostname: 'completely different', + instanceId: masterInstances[1]._id, + name: masterInstances[1].name, + isAlias: true + }] + instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) + instance.getAliases.returns([alias1, alias2]) + instance.setDependenciesFromEnvironment(ownerName, function (err) { + if (err) { + return done(err) + } + sinon.assert.calledOnce(instance.invalidateContainerDNS) + sinon.assert.calledTwice(instance.addDependency) + sinon.assert.calledWith(instance.addDependency.getCall(0), + sinon.match.has('name', masterInstances[0].name) + ) + sinon.assert.calledWith(instance.addDependency.getCall(1), + sinon.match.has('name', masterInstances[1].name) + ) + sinon.assert.calledOnce(instance.removeDependency) + sinon.assert.calledWith(instance.removeDependency.getCall(0), + masterInstances[1]._id, + 'completely different' + ) + done() + }) + }) + }) + }) describe('Testing changes in connections', function () { var masterInstances beforeEach(function (done) { @@ -804,6 +918,7 @@ describe('Instance Model Tests', function () { hostname: 'adelle-staging-' + ownerName + '.runnableapp.com' }) ] + sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) sinon.stub(Instance, 'find').yieldsAsync(null, masterInstances) sinon.stub(instance, 'addDependency').resolves() sinon.stub(instance, 'removeDependency').resolves() @@ -816,7 +931,6 @@ describe('Instance Model Tests', function () { }) describe('Envs', function () { it('should add a new dep for each env, when starting with none', function (done) { - sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) instance.env = [ 'as=hello-staging-' + ownerName + '.runnableapp.com', 'df=adelle-staging-' + ownerName + '.runnableapp.com' @@ -828,17 +942,17 @@ describe('Instance Model Tests', function () { sinon.assert.calledOnce(instance.invalidateContainerDNS) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('shortHash', masterInstances[0].shortHash) + sinon.match.has('name', masterInstances[0].name) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('shortHash', masterInstances[1].shortHash) + sinon.match.has('name', masterInstances[1].name) ) sinon.assert.notCalled(instance.removeDependency) done() }) }) it('should not allow it to add itself', function (done) { - sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) + instance.getDependencies.yieldsAsync(null, []) instance.env = [ 'as=' + instance.elasticHostname ] @@ -852,7 +966,8 @@ describe('Instance Model Tests', function () { }) }) it('should add 1 new dep, and keep the existing one', function (done) { - sinon.stub(instance, 'getDependencies').yieldsAsync(null, [masterInstances[1]]) + instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) + instance.dependencies = [masterInstances[1].generateGraphNode()] instance.env = [ 'as=hello-staging-' + ownerName + '.runnableapp.com', 'df=adelle-staging-' + ownerName + '.runnableapp.com' @@ -864,14 +979,18 @@ describe('Instance Model Tests', function () { sinon.assert.calledOnce(instance.invalidateContainerDNS) sinon.assert.calledOnce(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('shortHash', masterInstances[0].shortHash) + sinon.match.has('name', masterInstances[0].name) ) sinon.assert.notCalled(instance.removeDependency) done() }) }) it('should remove one of the existing, but leave the other', function (done) { - sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances) + instance.getDependencies.yieldsAsync(null, masterInstances) + instance.dependencies = [ + masterInstances[0].generateGraphNode(), + masterInstances[1].generateGraphNode() + ] instance.env = [ 'df=adelle-staging-' + ownerName + '.runnableapp.com' // Keep masterInstance[1] ] @@ -889,7 +1008,11 @@ describe('Instance Model Tests', function () { }) }) it('should remove both of the existing', function (done) { - sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances) + instance.getDependencies.yieldsAsync(null, masterInstances) + instance.dependencies = [ + masterInstances[0].generateGraphNode(), + masterInstances[1].generateGraphNode() + ] instance.setDependenciesFromEnvironment(ownerName, function (err) { if (err) { done(err) @@ -907,7 +1030,10 @@ describe('Instance Model Tests', function () { }) }) it('should remove the existing one, and add the new one', function (done) { - sinon.stub(instance, 'getDependencies').yieldsAsync(null, [masterInstances[1]]) + instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) + instance.dependencies = [ + masterInstances[1].generateGraphNode() + ] instance.env = [ 'df=hello-staging-' + ownerName + '.runnableapp.com' // Add masterInstance[0] ] @@ -922,7 +1048,7 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledOnce(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('shortHash', masterInstances[0].shortHash) + sinon.match.has('name', masterInstances[0].name) ) done() }) @@ -944,7 +1070,9 @@ describe('Instance Model Tests', function () { masterPod: true, hostname: 'potatoes-staging-' + ownerName + '.runnableapp.com' })) // 5 - sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances.slice(0, 3)) + var existingDeps = masterInstances.slice(0, 3); + instance.getDependencies.yieldsAsync(null, existingDeps) + instance.dependencies = existingDeps.map(dep => dep.generateGraphNode()) instance.env = [ 'df=hello-staging-' + ownerName + '.runnableapp.com', // keep masterInstance[0] 'asd=chicken-staging-' + ownerName + '.runnableapp.com', // add masterInstance[3] @@ -964,10 +1092,10 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('shortHash', masterInstances[3].shortHash) + sinon.match.has('name', masterInstances[3].name) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('shortHash', masterInstances[5].shortHash) + sinon.match.has('name', masterInstances[5].name) ) done() }) @@ -975,7 +1103,7 @@ describe('Instance Model Tests', function () { }) describe('FnR', function () { it('should add a new dep for each replace rule, when starting with none', function (done) { - sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) + instance.getDependencies.yieldsAsync(null, []) var firstAppCodeVersion = keypather.get(instance, 'contextVersion.appCodeVersions[0]') firstAppCodeVersion.transformRules.replace = [ { @@ -998,10 +1126,10 @@ describe('Instance Model Tests', function () { sinon.assert.calledOnce(instance.invalidateContainerDNS) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('shortHash', masterInstances[0].shortHash) + sinon.match.has('name', masterInstances[0].name) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('shortHash', masterInstances[1].shortHash) + sinon.match.has('name', masterInstances[1].name) ) sinon.assert.notCalled(instance.removeDependency) done() @@ -1024,7 +1152,9 @@ describe('Instance Model Tests', function () { masterPod: true, hostname: 'potatoes-staging-' + ownerName + '.runnableapp.com' })) // 5 - sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances.slice(0, 3)) + const existingDeps = masterInstances.slice(0, 3) + instance.getDependencies.yieldsAsync(null, existingDeps) + instance.dependencies = existingDeps.map(dep => dep.generateGraphNode()) instance.contextVersion.appCodeVersions[0].transformRules.replace = [ { action: 'Replace', @@ -1061,10 +1191,10 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('shortHash', masterInstances[3].shortHash) + sinon.match.has('name', masterInstances[3].name) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('shortHash', masterInstances[5].shortHash) + sinon.match.has('name', masterInstances[5].name) ) done() }) @@ -1088,7 +1218,9 @@ describe('Instance Model Tests', function () { masterPod: true, hostname: 'potatoes-staging-' + ownerName + '.runnableapp.com' })) // 5 - sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances.slice(0, 3)) + const existingDeps = masterInstances.slice(0, 3) + instance.getDependencies.yieldsAsync(null, existingDeps) + instance.dependencies = existingDeps.map(dep => dep.generateGraphNode()) instance.contextVersion.appCodeVersions[0].transformRules.replace = [ { action: 'Replace', @@ -1122,10 +1254,10 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('shortHash', masterInstances[5].shortHash) + sinon.match.has('name', masterInstances[5].name) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('shortHash', masterInstances[3].shortHash) + sinon.match.has('name', masterInstances[3].name) ) done() }) From 98df2c6867909ef16e5044524361fb977b8248a3 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Wed, 22 Feb 2017 17:12:35 -0800 Subject: [PATCH 6/6] oops. This shouldn't have been committed here --- lib/models/mongo/instance.js | 143 ++++++----------- lib/models/mongo/schemas/instance.js | 3 - test/integration/models/mongo/instance.js | 67 -------- unit/models/mongo/instances.js | 178 +++------------------- 4 files changed, 66 insertions(+), 325 deletions(-) diff --git a/lib/models/mongo/instance.js b/lib/models/mongo/instance.js index 9ab6de51e..69b828d6a 100644 --- a/lib/models/mongo/instance.js +++ b/lib/models/mongo/instance.js @@ -724,16 +724,12 @@ InstanceSchema.methods.getElasticHostname = function (ownerUsername) { }) } -InstanceSchema.methods.generateGraphNode = function (alias) { - let val = { - elasticHostname: alias || this.elasticHostname, +InstanceSchema.methods.generateGraphNode = function (ignoreIsolationProperty) { + return { + elasticHostname: this.elasticHostname, instanceId: this._id, name: this.name } - if (alias) { - val.isAlias = true - } - return val } /** @@ -848,18 +844,6 @@ function findMatchingInstanceByHostname (possibleHostnameString, instanceList) { }) } -/** - * Searches through a list of instances to find the first one that matches the context - * @param {String} contextId - ObjectId (as string) of the contextId to match - * @param {[Instance]} instanceList - Instances to search to find one matching the contextId - * @returns {Instance|null} instance with the given hostname - */ -function findMatchingInstanceByContextId (contextId, instanceList) { - return find(instanceList, function (instance) { - return keypather.get(instance, 'contextVersion.context.toString()') === contextId - }) -} - /** * Used for finding instances for new connections. This searches for either masterpods or isolated * containers, and adds the instance's hostname to itself. If the instance is not isolated, and @@ -917,52 +901,6 @@ InstanceSchema.methods.getHostnamesFromEnvsAndFnr = function () { return linkedHostnames } -InstanceSchema.methods.getAliases = function () { - var log = logger.log.child({ - instanceId: keypather.get(this, '_id'), - instanceName: keypather.get(this, 'name'), - method: 'InstanceSchema.methods.getAliases' - }) - log.info('called') - - return [] -} - -/** - * - * @param {Object} instancesOpts - * @param {Instance[]} instancesOpts.dependentInstances - * @param {Instance[]} instancesOpts.isolatedInstances - * @param {Instance[]} instancesOpts.masterInstances - * @param {Boolean=} isAlias - * - * @returns {Function} - * @private - */ -InstanceSchema.statics._createGraphNodesFromDeps = function (instancesOpts, isAlias) { - var dependentInstances = instancesOpts.dependentInstances // existing dependencies - var isolatedInstances = instancesOpts.isolatedInstances // instances in the same isolation group - var masterInstances = instancesOpts.masterInstances - // We need to do different kinds of matching depending on if this is an existing instance - var matchingMethod = isAlias ? findMatchingInstanceByContextId : findMatchingInstanceByHostname - return function (value) { - var val = isAlias ? value.contextId : value - var alias = isAlias ? value.alias : null - - var instance = matchingMethod(val, dependentInstances) - // first check the isolated instances. - if (!instance && isolatedInstances.length) { - instance = matchingMethod(val, isolatedInstances) - } - if (!instance) { - instance = matchingMethod(val, masterInstances) - } - if (instance) { - return instance.generateGraphNode(alias) - } - } -} - /** * Looks at the envs and it's current FnR rules, and finds all of the possible runnable hostnames in * them. Once it has all of those, it matches those hostnames to any of it's fellow isolation @@ -985,29 +923,43 @@ InstanceSchema.methods.setDependenciesFromEnvironment = function (ownerUsername, ownerUsername = ownerUsername.toLowerCase() Promise.props({ - existingGraphNodes: self.dependencies.map(dep => dep.toJSON()), - dependentInstances: self.getDependenciesAsync(), + dependencies: self.getDependenciesAsync(), masterInstances: self.fetchMatchingInstancesForDepChecking(ownerUsername), isolatedInstances: self.fetchMatchingInstancesForDepChecking(ownerUsername, true) }) .then(function (results) { - var existingDeps = results.existingGraphNodes // existing dependencies + var deps = results.dependencies // existing dependencies + var isolated = results.isolatedInstances // instances in the same isolation group + var masters = results.masterInstances + var linkedHostnames = self.getHostnamesFromEnvsAndFnr() .filter(function filterOutSelf (hostname) { return !doesStringContainInstanceHostname(hostname, self) }) - var linkedAliases = self.getAliases() - var envDeps = linkedHostnames.map(hostname => Instance._createGraphNodesFromDeps(results)(hostname)) - var aliasDeps = linkedAliases.map(alias => Instance._createGraphNodesFromDeps(results, true)(alias)) - - var allDeps = envDeps - .concat(aliasDeps) - .filter(exists) + var envDeps = linkedHostnames.map(function findAllMatchingInstancesByHostname (val) { + var instance = null + // Always add the original deps first so we don't destroy the current connections + var dep = findMatchingInstanceByHostname(val, deps) + if (dep) { + return dep + } + // first check the isolated instances. + if (self.isolated) { + instance = findMatchingInstanceByHostname(val, isolated) + } + if (!instance) { + instance = findMatchingInstanceByHostname(val, masters) + } + if (instance) { + // maybe add this dep if doesn't already exist + return instance + } + }).filter(exists) // check existing deps, to determine which to add and remove var subtract = new Subtract(depsEqual) - var remDeps = subtract.sub(existingDeps, allDeps) - var addDeps = subtract.sub(allDeps, existingDeps) + var remDeps = subtract.sub(deps, envDeps) + var addDeps = subtract.sub(envDeps, deps) log.trace({ remDeps: remDeps, @@ -1018,7 +970,7 @@ InstanceSchema.methods.setDependenciesFromEnvironment = function (ownerUsername, return self.addDependency(dep) } function toRemTask (dep) { - return self.removeDependency(dep.instanceId, dep.elasticHostname) + return self.removeDependency(dep._id) } // convert addDeps and remDeps to tasks return Promise.all([ @@ -1037,8 +989,7 @@ function depsEqual (depA, depB) { // we assume deps have the same keys var keypaths = [ 'elasticHostname', - 'name', - 'isAlias' + 'name' ] return keypaths.every(function (keypath) { var valA = keypather.get(depA, keypath + '.toString().toLowerCase()') @@ -1050,15 +1001,15 @@ function depsEqual (depA, depB) { /** * Goes through a given instance's dependencies, looking for a match for the given elasticHostname * - * @param {Instance} instance - Instance with dependencies to search through - * @param {ObjectId} instanceIdToFind - ObjectId of the instance to find + * @param {Instance} instance - Instance with dependencies to search through + * @param {String} elasticHostname - elastic hostname to search * * @returns {GraphNode|null} Either the matching node for the given hostname, or null */ -function getDepFromInstance (instance, instanceIdToFind) { +function getDepFromInstance (instance, elasticHostname) { if (keypather.get(instance, 'dependencies.length')) { var deps = instance.dependencies.filter(function (dep) { - return dep.instanceId.toString() === instanceIdToFind.toString() + return dep.elasticHostname === elasticHostname }) return deps.length ? deps[0] : null } @@ -1067,9 +1018,7 @@ function getDepFromInstance (instance, instanceIdToFind) { /** * Adds the given instance to THIS instance's dependency list * - * @param {Instance|GraphNode} instance - Either the full instance to add, or the already-creaeted - * graph node to add to THIS instance. Best to send the - * graph node + * @param {Instance} instance - instance to become a dependent * * @returns {Promise} When the dependency has been added * @resolves {Instance} This instance, updated @@ -1078,9 +1027,6 @@ function getDepFromInstance (instance, instanceIdToFind) { * @throws {Error} Any Mongo error */ InstanceSchema.methods.addDependency = function (instance) { - if (instance.generateGraphNode) { - instance = instance.generateGraphNode() - } var elasticHostname = instance.elasticHostname.toLowerCase() var log = logger.log.child({ instanceId: keypather.get(this, '_id'), @@ -1110,7 +1056,7 @@ InstanceSchema.methods.addDependency = function (instance) { _id: thisInstance._id }, { $push: { - dependencies: instance + dependencies: instance.generateGraphNode() } }) }) @@ -1123,8 +1069,8 @@ InstanceSchema.methods.addDependency = function (instance) { }) } }) - .then(function (updatedInstance) { - return getDepFromInstance(updatedInstance, instance._id) + .then(function (instance) { + return getDepFromInstance(instance, elasticHostname) }) .finally(function () { self.invalidateContainerDNS() @@ -1135,22 +1081,19 @@ InstanceSchema.methods.addDependency = function (instance) { * Remove an instance dependency from THIS instance * * @param {ObjectId} instanceId - instance id to remove as a dependent - * @param hostnameOrAlias * * @return {Promise} When the dependency has been removed * @resolve {Instance} This instance, updated without the dependency * @throws {Boom.notFound} When this instance failed to update * @throws {Error} Mongo Errors */ -InstanceSchema.methods.removeDependency = function (instanceId, hostnameOrAlias) { +InstanceSchema.methods.removeDependency = function (instanceId) { var self = this - var query = { instanceId } - if (hostnameOrAlias) { - query.elasticHostname = hostnameOrAlias - } return Instance.findByIdAndUpdateAsync(this._id, { $pull: { - dependencies: query + dependencies: { + instanceId: instanceId + } } }) .tap(function (instance) { diff --git a/lib/models/mongo/schemas/instance.js b/lib/models/mongo/schemas/instance.js index f9d0eb5a5..4d32cae93 100644 --- a/lib/models/mongo/schemas/instance.js +++ b/lib/models/mongo/schemas/instance.js @@ -258,9 +258,6 @@ var InstanceSchema = module.exports = new Schema({ }, name: { type: String - }, - isAlias: { - type: Boolean } }], 'default': [] diff --git a/test/integration/models/mongo/instance.js b/test/integration/models/mongo/instance.js index aff7b405b..1ca719aa2 100644 --- a/test/integration/models/mongo/instance.js +++ b/test/integration/models/mongo/instance.js @@ -967,73 +967,6 @@ describe('Instance Model Integration Tests', function () { }) }) }) - describe('checking envs don\'t hurt alias in connections', function () { - beforeEach(function (done) { - createNewInstance('fb1-adelle', { - name: 'fb1-adelle', - masterPod: false, - branch: 'fb1', - cv: ctx.adelle.contextVersion - })(done) - }) - beforeEach(function (done) { - createNewInstance('fb1-goodbye', { - name: 'fb1-goodbye', - masterPod: false, - branch: 'fb1', - cv: ctx.goodbye.contextVersion - })(done) - }) - beforeEach(function (done) { - // Set the dep to a branch - ctx.hello.addDependency(ctx['fb1-adelle']).asCallback(done) - }) - it('should not remove the existing dep, when not adding any new ones', function (done) { - ctx.hello.setDependenciesFromEnvironment(ownerName, function (err) { - if (err) { - return done(err) - } - ctx.hello.getDependencies(function (err, dependencies) { - expect(dependencies).to.be.array() - expect(dependencies.length).to.equal(1) - expect(dependencies[0]._id).to.equal(ctx['fb1-adelle']._id) - expect(dependencies[0].elasticHostname).to.equal(ctx['fb1-adelle'].elasticHostname) - done(err) - }) - }) - }) - it('should add 1 new dep, and keep the existing one', function (done) { - ctx.hello.env.push([ - 'as=goodbye-staging-' + ownerName + '.runnableapp.com' - ]) - ctx.hello.setDependenciesFromEnvironment(ownerName, function (err) { - if (err) { - return done(err) - } - ctx.hello.getDependencies(function (err, dependencies) { - expect(dependencies).to.be.array() - expect(dependencies.length).to.equal(2) - var ids = dependencies.map(pluck('_id.toString()')) - expect(ids).to.include(ctx.goodbye._id.toString()) - expect(ids).to.include(ctx['fb1-adelle']._id.toString()) - done(err) - }) - }) - }) - it('should remove the only dependency', function (done) { - ctx.hello.env = [] - ctx.hello.setDependenciesFromEnvironment(ownerName, function (err) { - if (err) { - return done(err) - } - ctx.hello.getDependencies(function (err, dependencies) { - expect(dependencies).to.be.array() - expect(dependencies.length).to.equal(0) - done(err) - }) - }) - }) - }) describe('being isolated', function () { beforeEach(function (done) { diff --git a/unit/models/mongo/instances.js b/unit/models/mongo/instances.js index fbe14cc53..e913e52d7 100644 --- a/unit/models/mongo/instances.js +++ b/unit/models/mongo/instances.js @@ -790,121 +790,7 @@ describe('Instance Model Tests', function () { }) }) }) - describe('Testing changes in aliases', function () { - var masterInstances - var contextId1 = 'asd' - var contextId2 = 'gsfdg' - var alias1 = { - contextId: contextId1, - alias: 'hello' - } - var alias2 = { - contextId: contextId2, - alias: 'hello.world' - } - var alias12 = { - contextId: contextId1, - alias: 'hello.world' - } - beforeEach(function (done) { - masterInstances = [ - mongoFactory.createNewInstance('hello', { - masterPod: true, - hostname: 'hello-staging-' + ownerName + '.runnableapp.com', - contextVersion: { - context: contextId1 - } - }), - mongoFactory.createNewInstance('adelle', { - masterPod: true, - hostname: 'adelle-staging-' + ownerName + '.runnableapp.com', - contextVersion: { - context: contextId2 - } - }) - ] - sinon.stub(Instance, 'find').yieldsAsync(null, masterInstances) - sinon.stub(instance, 'getAliases') - sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) - sinon.stub(instance, 'addDependency').resolves() - sinon.stub(instance, 'removeDependency').resolves() - done() - }) - afterEach(function (done) { - instance.addDependency.restore() - instance.removeDependency.restore() - instance.getAliases.restore() - done() - }) - describe('Aliases', function () { - it('should add a new dep for the alias', function (done) { - instance.getAliases.returns([alias1]) - instance.setDependenciesFromEnvironment(ownerName, function (err) { - if (err) { - return done(err) - } - sinon.assert.calledOnce(instance.invalidateContainerDNS) - sinon.assert.calledOnce(instance.addDependency) - sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[0].name) - ) - sinon.assert.notCalled(instance.removeDependency) - done() - }) - }) - it('should add a new dep, and keep the existing one', function (done) { - instance.dependencies = [{ - elasticHostname: alias2.alias, - instanceId: masterInstances[1]._id, - name: masterInstances[1].name, - isAlias: true - }] - instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) - instance.getAliases.returns([alias1, alias2]) - instance.setDependenciesFromEnvironment(ownerName, function (err) { - if (err) { - return done(err) - } - sinon.assert.calledOnce(instance.invalidateContainerDNS) - sinon.assert.calledOnce(instance.addDependency) - sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[0].name) - ) - sinon.assert.notCalled(instance.removeDependency) - done() - }) - }) - it('should add 2 new deps, and remove the existing one', function (done) { - instance.dependencies = [{ - elasticHostname: 'completely different', - instanceId: masterInstances[1]._id, - name: masterInstances[1].name, - isAlias: true - }] - instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) - instance.getAliases.returns([alias1, alias2]) - instance.setDependenciesFromEnvironment(ownerName, function (err) { - if (err) { - return done(err) - } - sinon.assert.calledOnce(instance.invalidateContainerDNS) - sinon.assert.calledTwice(instance.addDependency) - sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[0].name) - ) - sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('name', masterInstances[1].name) - ) - sinon.assert.calledOnce(instance.removeDependency) - sinon.assert.calledWith(instance.removeDependency.getCall(0), - masterInstances[1]._id, - 'completely different' - ) - done() - }) - }) - }) - }) + describe('Testing changes in connections', function () { var masterInstances beforeEach(function (done) { @@ -918,7 +804,6 @@ describe('Instance Model Tests', function () { hostname: 'adelle-staging-' + ownerName + '.runnableapp.com' }) ] - sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) sinon.stub(Instance, 'find').yieldsAsync(null, masterInstances) sinon.stub(instance, 'addDependency').resolves() sinon.stub(instance, 'removeDependency').resolves() @@ -931,6 +816,7 @@ describe('Instance Model Tests', function () { }) describe('Envs', function () { it('should add a new dep for each env, when starting with none', function (done) { + sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) instance.env = [ 'as=hello-staging-' + ownerName + '.runnableapp.com', 'df=adelle-staging-' + ownerName + '.runnableapp.com' @@ -942,17 +828,17 @@ describe('Instance Model Tests', function () { sinon.assert.calledOnce(instance.invalidateContainerDNS) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[0].name) + sinon.match.has('shortHash', masterInstances[0].shortHash) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('name', masterInstances[1].name) + sinon.match.has('shortHash', masterInstances[1].shortHash) ) sinon.assert.notCalled(instance.removeDependency) done() }) }) it('should not allow it to add itself', function (done) { - instance.getDependencies.yieldsAsync(null, []) + sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) instance.env = [ 'as=' + instance.elasticHostname ] @@ -966,8 +852,7 @@ describe('Instance Model Tests', function () { }) }) it('should add 1 new dep, and keep the existing one', function (done) { - instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) - instance.dependencies = [masterInstances[1].generateGraphNode()] + sinon.stub(instance, 'getDependencies').yieldsAsync(null, [masterInstances[1]]) instance.env = [ 'as=hello-staging-' + ownerName + '.runnableapp.com', 'df=adelle-staging-' + ownerName + '.runnableapp.com' @@ -979,18 +864,14 @@ describe('Instance Model Tests', function () { sinon.assert.calledOnce(instance.invalidateContainerDNS) sinon.assert.calledOnce(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[0].name) + sinon.match.has('shortHash', masterInstances[0].shortHash) ) sinon.assert.notCalled(instance.removeDependency) done() }) }) it('should remove one of the existing, but leave the other', function (done) { - instance.getDependencies.yieldsAsync(null, masterInstances) - instance.dependencies = [ - masterInstances[0].generateGraphNode(), - masterInstances[1].generateGraphNode() - ] + sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances) instance.env = [ 'df=adelle-staging-' + ownerName + '.runnableapp.com' // Keep masterInstance[1] ] @@ -1008,11 +889,7 @@ describe('Instance Model Tests', function () { }) }) it('should remove both of the existing', function (done) { - instance.getDependencies.yieldsAsync(null, masterInstances) - instance.dependencies = [ - masterInstances[0].generateGraphNode(), - masterInstances[1].generateGraphNode() - ] + sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances) instance.setDependenciesFromEnvironment(ownerName, function (err) { if (err) { done(err) @@ -1030,10 +907,7 @@ describe('Instance Model Tests', function () { }) }) it('should remove the existing one, and add the new one', function (done) { - instance.getDependencies.yieldsAsync(null, [masterInstances[1]]) - instance.dependencies = [ - masterInstances[1].generateGraphNode() - ] + sinon.stub(instance, 'getDependencies').yieldsAsync(null, [masterInstances[1]]) instance.env = [ 'df=hello-staging-' + ownerName + '.runnableapp.com' // Add masterInstance[0] ] @@ -1048,7 +922,7 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledOnce(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[0].name) + sinon.match.has('shortHash', masterInstances[0].shortHash) ) done() }) @@ -1070,9 +944,7 @@ describe('Instance Model Tests', function () { masterPod: true, hostname: 'potatoes-staging-' + ownerName + '.runnableapp.com' })) // 5 - var existingDeps = masterInstances.slice(0, 3); - instance.getDependencies.yieldsAsync(null, existingDeps) - instance.dependencies = existingDeps.map(dep => dep.generateGraphNode()) + sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances.slice(0, 3)) instance.env = [ 'df=hello-staging-' + ownerName + '.runnableapp.com', // keep masterInstance[0] 'asd=chicken-staging-' + ownerName + '.runnableapp.com', // add masterInstance[3] @@ -1092,10 +964,10 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[3].name) + sinon.match.has('shortHash', masterInstances[3].shortHash) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('name', masterInstances[5].name) + sinon.match.has('shortHash', masterInstances[5].shortHash) ) done() }) @@ -1103,7 +975,7 @@ describe('Instance Model Tests', function () { }) describe('FnR', function () { it('should add a new dep for each replace rule, when starting with none', function (done) { - instance.getDependencies.yieldsAsync(null, []) + sinon.stub(instance, 'getDependencies').yieldsAsync(null, []) var firstAppCodeVersion = keypather.get(instance, 'contextVersion.appCodeVersions[0]') firstAppCodeVersion.transformRules.replace = [ { @@ -1126,10 +998,10 @@ describe('Instance Model Tests', function () { sinon.assert.calledOnce(instance.invalidateContainerDNS) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[0].name) + sinon.match.has('shortHash', masterInstances[0].shortHash) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('name', masterInstances[1].name) + sinon.match.has('shortHash', masterInstances[1].shortHash) ) sinon.assert.notCalled(instance.removeDependency) done() @@ -1152,9 +1024,7 @@ describe('Instance Model Tests', function () { masterPod: true, hostname: 'potatoes-staging-' + ownerName + '.runnableapp.com' })) // 5 - const existingDeps = masterInstances.slice(0, 3) - instance.getDependencies.yieldsAsync(null, existingDeps) - instance.dependencies = existingDeps.map(dep => dep.generateGraphNode()) + sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances.slice(0, 3)) instance.contextVersion.appCodeVersions[0].transformRules.replace = [ { action: 'Replace', @@ -1191,10 +1061,10 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[3].name) + sinon.match.has('shortHash', masterInstances[3].shortHash) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('name', masterInstances[5].name) + sinon.match.has('shortHash', masterInstances[5].shortHash) ) done() }) @@ -1218,9 +1088,7 @@ describe('Instance Model Tests', function () { masterPod: true, hostname: 'potatoes-staging-' + ownerName + '.runnableapp.com' })) // 5 - const existingDeps = masterInstances.slice(0, 3) - instance.getDependencies.yieldsAsync(null, existingDeps) - instance.dependencies = existingDeps.map(dep => dep.generateGraphNode()) + sinon.stub(instance, 'getDependencies').yieldsAsync(null, masterInstances.slice(0, 3)) instance.contextVersion.appCodeVersions[0].transformRules.replace = [ { action: 'Replace', @@ -1254,10 +1122,10 @@ describe('Instance Model Tests', function () { ) sinon.assert.calledTwice(instance.addDependency) sinon.assert.calledWith(instance.addDependency.getCall(0), - sinon.match.has('name', masterInstances[5].name) + sinon.match.has('shortHash', masterInstances[5].shortHash) ) sinon.assert.calledWith(instance.addDependency.getCall(1), - sinon.match.has('name', masterInstances[3].name) + sinon.match.has('shortHash', masterInstances[3].shortHash) ) done() })