From 75dda3fd689e218e0af6d51917ad966ea96969e7 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Thu, 6 Jul 2017 13:26:53 -0700 Subject: [PATCH] Fix where the add-branch was fetching the compose file It was wrong, trying to get it from the added branch --- lib/models/services/cluster-config-service.js | 33 +++++++++------- lib/models/services/webhook-service.js | 22 ++++++++--- lib/routes/instances/index.js | 2 +- .../models/services/cluster-config-service.js | 39 ++++++++++++++----- unit/models/services/webhook-service.js | 22 ++++++++++- unit/routes/instances.js | 1 + 6 files changed, 87 insertions(+), 32 deletions(-) diff --git a/lib/models/services/cluster-config-service.js b/lib/models/services/cluster-config-service.js index 70bb6d6f8..4e9786028 100644 --- a/lib/models/services/cluster-config-service.js +++ b/lib/models/services/cluster-config-service.js @@ -1557,18 +1557,19 @@ module.exports = class ClusterConfigService { * Checks if the given instance has a Docker Compose config, and if it does, check if compose files were changed. * If it doesn't, this throws a BaseSchema.NotFoundError * - * @param {Instance} instance - Instance to look up - * @param {Object} githubPushInfo - Github webhook push data - * @param {String} githubPushInfo.repo - Github repositories full name (org/repo) - * @param {String} githubPushInfo.commit - Commit for this push - * @param {String} githubPushInfo.bpUserId - BigPoppa user id + * @param {SessionUser} sessionUser - sessionUser + * @param {Instance} instance - Instance to look up + * @param {Object} githubPushInfo - Github webhook push data + * @param {String} githubPushInfo.repo - Github repositories full name (org/repo) + * @param {String} githubPushInfo.commit - Commit for this push + * @param {String} githubPushInfo.bpUserId - BigPoppa user id * * @resolves {undefined} resolves when the update config job has been created * * @throws InputClusterConfig.NotChangedError - When the config's sha's match, so this won't be done * @throws InputClusterConfig.NotFoundError - When no config is found to match the given instance */ - static checkIfComposeFilesChanged (instance, githubPushInfo) { + static checkIfComposeFilesChanged (sessionUser, instance, githubPushInfo) { const log = ClusterConfigService.log.child({ method: 'checkIfComposeFilesChanged', instance, githubPushInfo @@ -1576,19 +1577,19 @@ module.exports = class ClusterConfigService { log.info('called') return Promise.props({ aic: AutoIsolationService.fetchAutoIsolationForInstance(instance._id), - clusterConfig: ClusterConfigService.fetchConfigByInstanceId(instance._id) + clusterConfig: ClusterConfigService.fetchComposeInfoByInstanceId(sessionUser, instance._id, githubPushInfo) }) .then(results => { - const clusterConfig = results.clusterConfig + const clusterConfig = results.clusterConfig.clusterOpts const currentPaths = clusterConfig.files.map(pluck('path')) // We found a cluster, so fetch the current one, and see if it changed return UserService.getByBpId(githubPushInfo.bpUserId) .then(bpUser => { return ClusterConfigService.fetchFilesFromGithub( bpUser, - githubPushInfo.repo, + clusterConfig.repo, currentPaths, - githubPushInfo.commit + clusterConfig.commit ) }) .then(newComposeFiles => { @@ -1626,22 +1627,24 @@ module.exports = class ClusterConfigService { /** * If you need to do an updateCluster job, use this method. It checks if the compose file has changed * before creating the job. - * @param {Instance} instance - instance to be updated - * @param {Object} githubPushInfo - parsed githook data - * @param {String} githubPushInfo.bpUserId - BigPoppa user id + * + * @param {SessionUser} sessionUser - SessionUser + * @param {Instance} instance - instance to be updated + * @param {Object} githubPushInfo - parsed githook data + * @param {String} githubPushInfo.bpUserId - BigPoppa user id * * @resolves {undefined} * * @throws InputClusterConfig.NotChangedError - When the config's sha's match, so this won't be done * @throws InputClusterConfig.NotFoundError - When no config is found to match the given instance */ - static checkFileChangeAndCreateUpdateJob (instance, githubPushInfo) { + static checkFileChangeAndCreateUpdateJob (sessionUser, instance, githubPushInfo) { const log = ClusterConfigService.log.child({ method: 'checkFileChangeAndCreateUpdateJob', instance, githubPushInfo }) log.info('called') - return ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) + return ClusterConfigService.checkIfComposeFilesChanged(sessionUser, instance, githubPushInfo) .then(() => rabbitMQ.updateCluster({ instanceId: instance._id.toString(), pushInfo: githubPushInfo diff --git a/lib/models/services/webhook-service.js b/lib/models/services/webhook-service.js index 78932d222..b7e2b2776 100644 --- a/lib/models/services/webhook-service.js +++ b/lib/models/services/webhook-service.js @@ -79,7 +79,10 @@ WebhookService.updateComposeOrAutoDeploy = function (instance, githubPushInfo) { method: 'updateComposeOrAutoDeploy' }) log.info('called') - return ClusterConfigService.checkFileChangeAndCreateUpdateJob(instance, githubPushInfo) + const committerUsername = keypather.get(githubPushInfo, 'commitPusher') + + return UserService.getCompleteUserByGithubUsername(committerUsername) + .then((user) => ClusterConfigService.checkFileChangeAndCreateUpdateJob(user, instance, githubPushInfo)) .catch((err) => { log.warn({ err }, 'Cluster could not be checked or updated') return BuildService.createAndBuildContextVersion(instance, githubPushInfo, 'autodeploy') @@ -150,13 +153,22 @@ WebhookService.autoFork = function (autoDeployedInstances, githubPushInfo) { method: 'autoFork' }) log.info('called') - - return Instance.findMasterPodsToAutoFork(githubPushInfo.repo, githubPushInfo.branch, autoDeployedInstances) + const committerUsername = keypather.get(githubPushInfo, 'commitPusher') + return Promise.props({ + sessionUser: UserService.getCompleteUserByGithubUsername(committerUsername), + instances: Instance.findMasterPodsToAutoFork( + githubPushInfo.repo, + githubPushInfo.branch, + autoDeployedInstances + ) + }) .catch(function (err) { log.error({ err }, 'error attempting to fetch autoForking candidates') throw err }) - .then(function (instances) { + .then(function (results) { + const instances = results.instances + const sessionUser = results.sessionUser if (!instances.length) { log.info('found no instances to fork') return null @@ -166,7 +178,7 @@ WebhookService.autoFork = function (autoDeployedInstances, githubPushInfo) { return IsolationService.autoIsolate(newInstances, githubPushInfo) }) .each(newInstance => - ClusterConfigService.checkFileChangeAndCreateUpdateJob(newInstance, githubPushInfo) + ClusterConfigService.checkFileChangeAndCreateUpdateJob(sessionUser, newInstance, githubPushInfo) .catch(err => { log.warn({ err }, 'Cluster could not be checked or updated') }) diff --git a/lib/routes/instances/index.js b/lib/routes/instances/index.js index 6887c002d..77c5e1488 100644 --- a/lib/routes/instances/index.js +++ b/lib/routes/instances/index.js @@ -526,7 +526,7 @@ module.exports.forkInstance = function (req) { return IsolationService.autoIsolate([newInstance], githubPushInfo) }) .tap(newInstance => - ClusterConfigService.checkFileChangeAndCreateUpdateJob(newInstance, githubPushInfo) + ClusterConfigService.checkFileChangeAndCreateUpdateJob(req.sessionUser, newInstance, githubPushInfo) .catch(err => { log.warn({err}, 'Cluster could not be checked or updated') }) diff --git a/unit/models/services/cluster-config-service.js b/unit/models/services/cluster-config-service.js index 6bf7adbf5..c92532dcd 100644 --- a/unit/models/services/cluster-config-service.js +++ b/unit/models/services/cluster-config-service.js @@ -1840,16 +1840,35 @@ describe('Cluster Config Service Unit Tests', function () { const instance = { _id: instanceId } - + const testSessionUser = { + _id: 'id', + accounts: { + github: { + id: testUserGithubId, + accessToken: 'some-token' + }, + login: 'login', + username: 'best' + }, + bigPoppaUser: { + id: testUserBpId, + organizations: [{ + name: testOrgName, + lowerName: testOrgName.toLowerCase(), + id: testOrgBpId, + githubId: testOrgGithubId + }] + } + } beforeEach(function (done) { - sinon.stub(ClusterConfigService, 'fetchConfigByInstanceId').resolves(clusterConfig) + sinon.stub(ClusterConfigService, 'fetchComposeInfoByInstanceId').resolves({ clusterOpts: clusterConfig }) sinon.stub(AutoIsolationService, 'fetchAutoIsolationForInstance').resolves(aic) sinon.stub(UserService, 'getByBpId').resolves(userModel) sinon.stub(ClusterConfigService, 'fetchFilesFromGithub').resolves(changedClusterConfigFiles) done() }) afterEach(function (done) { - ClusterConfigService.fetchConfigByInstanceId.restore() + ClusterConfigService.fetchComposeInfoByInstanceId.restore() AutoIsolationService.fetchAutoIsolationForInstance.restore() UserService.getByBpId.restore() ClusterConfigService.fetchFilesFromGithub.restore() @@ -1858,8 +1877,8 @@ describe('Cluster Config Service Unit Tests', function () { describe('errors', function () { it('should return error if fetchConfigByInstanceId failed', function (done) { const error = new Error('Some error') - ClusterConfigService.fetchConfigByInstanceId.rejects(error) - ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) + ClusterConfigService.fetchComposeInfoByInstanceId.rejects(error) + ClusterConfigService.checkIfComposeFilesChanged(testSessionUser, instance, githubPushInfo) .asCallback(function (err) { expect(err).to.exist() expect(err.message).to.equal(error.message) @@ -1869,7 +1888,7 @@ describe('Cluster Config Service Unit Tests', function () { it('should return error if UserService.getByBpId failed', function (done) { const error = new Error('Some error') UserService.getByBpId.rejects(error) - ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) + ClusterConfigService.checkIfComposeFilesChanged(testSessionUser, instance, githubPushInfo) .asCallback(function (err) { expect(err).to.exist() expect(err.message).to.equal(error.message) @@ -1879,7 +1898,7 @@ describe('Cluster Config Service Unit Tests', function () { it('should return error if ClusterConfigService.fetchFilesFromGithub failed', function (done) { const error = new Error('Some error') ClusterConfigService.fetchFilesFromGithub.rejects(error) - ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) + ClusterConfigService.checkIfComposeFilesChanged(testSessionUser, instance, githubPushInfo) .asCallback(function (err) { expect(err).to.exist() expect(err.message).to.equal(error.message) @@ -1890,11 +1909,11 @@ describe('Cluster Config Service Unit Tests', function () { describe('success', function () { it('should run successfully', function () { ClusterConfigService.fetchFilesFromGithub.resolves(changedClusterConfigFiles) - return ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) + return ClusterConfigService.checkIfComposeFilesChanged(testSessionUser, instance, githubPushInfo) }) it('should return InputClusterConfig.NotChangedError if shas match', function (done) { ClusterConfigService.fetchFilesFromGithub.resolves(clusterConfigFiles) - ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) + ClusterConfigService.checkIfComposeFilesChanged(testSessionUser, instance, githubPushInfo) .then(function () { done(new Error('Expecting NotChangedError')) }) @@ -1905,7 +1924,7 @@ describe('Cluster Config Service Unit Tests', function () { it('should not return InputClusterConfig.NotChangedError if aic is parent\'s', function () { ClusterConfigService.fetchFilesFromGithub.resolves(changedClusterConfigFiles) AutoIsolationService.fetchAutoIsolationForInstance.resolves(parentAic) - return ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) + return ClusterConfigService.checkIfComposeFilesChanged(testSessionUser, instance, githubPushInfo) }) }) }) diff --git a/unit/models/services/webhook-service.js b/unit/models/services/webhook-service.js index 06208581b..8dfe45761 100644 --- a/unit/models/services/webhook-service.js +++ b/unit/models/services/webhook-service.js @@ -184,18 +184,27 @@ describe('Webhook Service Unit Tests', function () { repo: 'theRepo', branch: 'theBranch' } + const user = { + _id: 'some-id', + allowed: true, + bigPoppaUser: { + id: 'bp-id' + } + } var instance beforeEach(function (done) { instance = { _id: 'sdasdsaddgfasdfgasdfasdf' } sinon.stub(BuildService, 'createAndBuildContextVersion') + sinon.stub(UserService, 'getCompleteUserByGithubUsername').resolves(user) sinon.stub(ClusterConfigService, 'checkIfComposeFilesChanged').rejects(new Error()) sinon.stub(rabbitMQ, 'updateCluster').resolves() done() }) afterEach(function (done) { BuildService.createAndBuildContextVersion.restore() + UserService.getCompleteUserByGithubUsername.restore() ClusterConfigService.checkIfComposeFilesChanged.restore() rabbitMQ.updateCluster.restore() done() @@ -255,6 +264,13 @@ describe('Webhook Service Unit Tests', function () { repo: 'theRepo', branch: 'theBranch' } + const user = { + _id: 'some-id', + allowed: true, + bigPoppaUser: { + id: 'bp-id' + } + } var instances var contextIds beforeEach(function (done) { @@ -266,15 +282,17 @@ describe('Webhook Service Unit Tests', function () { contextIds = ['sadsadasdsad', 'sdgfddfsgdfsgsdfgdsfg'] sinon.stub(Instance, 'findMasterPodsToAutoFork') sinon.stub(ClusterConfigService, 'checkIfComposeFilesChanged').resolves() + sinon.stub(UserService, 'getCompleteUserByGithubUsername').resolves(user) sinon.stub(InstanceForkService, 'autoFork') sinon.stub(IsolationService, 'autoIsolate') - sinon.stub(rabbitMQ, 'updateCluster').resolves + sinon.stub(rabbitMQ, 'updateCluster').resolves() done() }) afterEach(function (done) { Instance.findMasterPodsToAutoFork.restore() ClusterConfigService.checkIfComposeFilesChanged.restore() InstanceForkService.autoFork.restore() + UserService.getCompleteUserByGithubUsername.restore() IsolationService.autoIsolate.restore() rabbitMQ.updateCluster.restore() done() @@ -387,11 +405,13 @@ describe('Webhook Service Unit Tests', function () { sinon.assert.calledTwice(ClusterConfigService.checkIfComposeFilesChanged) sinon.assert.calledWithExactly( ClusterConfigService.checkIfComposeFilesChanged, + user, forkedInstances[0], githubPushInfo ) sinon.assert.calledWithExactly( ClusterConfigService.checkIfComposeFilesChanged, + user, forkedInstances[1], githubPushInfo ) diff --git a/unit/routes/instances.js b/unit/routes/instances.js index ae8d0b963..b76501062 100644 --- a/unit/routes/instances.js +++ b/unit/routes/instances.js @@ -127,6 +127,7 @@ describe('/instances', () => { .then(function () { sinon.assert.calledOnce(ClusterConfigService.checkFileChangeAndCreateUpdateJob) sinon.assert.calledWith(ClusterConfigService.checkFileChangeAndCreateUpdateJob, + mockReq.sessionUser, mockForkedInstance, { repo: 'repoName',