From e609ef10a2bf1535979be8fd8bbf1da32a583fa0 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Wed, 28 Jun 2017 14:45:52 -0700 Subject: [PATCH 1/4] Only throw the error that the files are the same if the instance._id matches the one on the AIC --- lib/models/services/cluster-config-service.js | 12 ++++++-- .../models/services/cluster-config-service.js | 29 ++++++++++++++----- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/lib/models/services/cluster-config-service.js b/lib/models/services/cluster-config-service.js index e1c702f0b..7b081f151 100644 --- a/lib/models/services/cluster-config-service.js +++ b/lib/models/services/cluster-config-service.js @@ -1568,8 +1568,12 @@ module.exports = class ClusterConfigService { instance, githubPushInfo }) log.info('called') - return ClusterConfigService.fetchConfigByInstanceId(instance._id) - .then(clusterConfig => { + return Promise.props({ + aic: AutoIsolationService.fetchAutoIsolationForInstance(instance._id), + clusterConfig: ClusterConfigService.fetchConfigByInstanceId(instance._id) + }) + .then(results => { + const clusterConfig = results.clusterConfig 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) @@ -1590,7 +1594,9 @@ module.exports = class ClusterConfigService { currentPaths }, 'new compose files') const diffedShas = difference(newShas, currentShas) - if (diffedShas.length === 0) { + const aic = results.aic + // We want branches to act like they've changed, so they make their own ICC and AIC + if (diffedShas.length === 0 && instance._id === aic.instance) { throw new InputClusterConfig.NotChangedError({ newShas, currentShas diff --git a/unit/models/services/cluster-config-service.js b/unit/models/services/cluster-config-service.js index 608bb1d49..a4a2b42c2 100644 --- a/unit/models/services/cluster-config-service.js +++ b/unit/models/services/cluster-config-service.js @@ -1725,15 +1725,26 @@ describe('Cluster Config Service Unit Tests', function () { bpUserId, repo: repoFullName } + const aic = { + instance: instanceId + } + const parentAic = { + instance: 'asdasdasdassa' + } + const instance = { + _id: instanceId + } beforeEach(function (done) { sinon.stub(ClusterConfigService, 'fetchConfigByInstanceId').resolves(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() + AutoIsolationService.fetchAutoIsolationForInstance.restore() UserService.getByBpId.restore() ClusterConfigService.fetchFilesFromGithub.restore() done() @@ -1742,7 +1753,7 @@ describe('Cluster Config Service Unit Tests', function () { it('should return error if fetchConfigByInstanceId failed', function (done) { const error = new Error('Some error') ClusterConfigService.fetchConfigByInstanceId.rejects(error) - ClusterConfigService.checkIfComposeFilesChanged(instanceId, githubPushInfo) + ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) .asCallback(function (err) { expect(err).to.exist() expect(err.message).to.equal(error.message) @@ -1752,7 +1763,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(instanceId, githubPushInfo) + ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) .asCallback(function (err) { expect(err).to.exist() expect(err.message).to.equal(error.message) @@ -1762,7 +1773,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(instanceId, githubPushInfo) + ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) .asCallback(function (err) { expect(err).to.exist() expect(err.message).to.equal(error.message) @@ -1771,14 +1782,13 @@ describe('Cluster Config Service Unit Tests', function () { }) }) describe('success', function () { - it('should run successfully', function (done) { + it('should run successfully', function () { ClusterConfigService.fetchFilesFromGithub.resolves(changedClusterConfigFiles) - ClusterConfigService.checkIfComposeFilesChanged(instanceId, githubPushInfo) - .asCallback(done) + return ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) }) it('should return InputClusterConfig.NotChangedError if shas match', function (done) { ClusterConfigService.fetchFilesFromGithub.resolves(clusterConfigFiles) - ClusterConfigService.checkIfComposeFilesChanged(instanceId, githubPushInfo) + ClusterConfigService.checkIfComposeFilesChanged(instance, githubPushInfo) .then(function () { done(new Error('Expecting NotChangedError')) }) @@ -1786,6 +1796,11 @@ describe('Cluster Config Service Unit Tests', function () { done() }) }) + 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) + }) }) }) describe('_createAutoIsolationModelsFromClusterInstances', function () { From cca7df023396d117886a035e8ce4cd21a021f959 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Thu, 29 Jun 2017 15:00:01 -0700 Subject: [PATCH 2/4] Fix issue where created branches wouldn't update their compose --- lib/models/services/cluster-config-service.js | 5 +++-- lib/routes/instances/index.js | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/models/services/cluster-config-service.js b/lib/models/services/cluster-config-service.js index 7b081f151..6e94b837e 100644 --- a/lib/models/services/cluster-config-service.js +++ b/lib/models/services/cluster-config-service.js @@ -1620,8 +1620,9 @@ 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 {Instance} instance - instance to be updated + * @param {Object} githubPushInfo - parsed githook data + * @param {String} githubPushInfo.bpUserId - BigPoppa user id * * @resolves {undefined} * diff --git a/lib/routes/instances/index.js b/lib/routes/instances/index.js index d3875ec3c..6887c002d 100644 --- a/lib/routes/instances/index.js +++ b/lib/routes/instances/index.js @@ -504,7 +504,8 @@ module.exports.forkInstance = function (req) { commit: req.body.sha, user: { id: req.sessionUser.accounts.github.id - } + }, + bpUserId: keypather.get(req.sessionUser, 'bigPoppaUser.id') } log.info({ githubPushInfo: githubPushInfo From 63fc5bfa92bacf887153b1e9f4f33bbc7298d56f Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Thu, 29 Jun 2017 15:02:24 -0700 Subject: [PATCH 3/4] toString() --- lib/models/services/cluster-config-service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/services/cluster-config-service.js b/lib/models/services/cluster-config-service.js index 6e94b837e..38fc5f52b 100644 --- a/lib/models/services/cluster-config-service.js +++ b/lib/models/services/cluster-config-service.js @@ -1596,7 +1596,7 @@ module.exports = class ClusterConfigService { const diffedShas = difference(newShas, currentShas) const aic = results.aic // We want branches to act like they've changed, so they make their own ICC and AIC - if (diffedShas.length === 0 && instance._id === aic.instance) { + if (diffedShas.length === 0 && instance._id.toString() === aic.instance.toString()) { throw new InputClusterConfig.NotChangedError({ newShas, currentShas From 6b01e1f62a0d038c327baa012ff2e3d6b500d519 Mon Sep 17 00:00:00 2001 From: Nathan219 Date: Thu, 29 Jun 2017 16:19:14 -0700 Subject: [PATCH 4/4] Fix tests --- unit/routes/instances.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/unit/routes/instances.js b/unit/routes/instances.js index 4130f77aa..ae8d0b963 100644 --- a/unit/routes/instances.js +++ b/unit/routes/instances.js @@ -39,6 +39,9 @@ describe('/instances', () => { github: { id: githubId } + }, + bigPoppaUser: { + id: 1 } } } @@ -79,7 +82,8 @@ describe('/instances', () => { commit: 'sha12', user: { id: '1234' - } + }, + bpUserId: mockReq.sessionUser.bigPoppaUser.id }, 'manual' ) @@ -111,7 +115,8 @@ describe('/instances', () => { commit: 'sha12', user: { id: '1234' - } + }, + bpUserId: mockReq.sessionUser.bigPoppaUser.id } ) }) @@ -129,7 +134,8 @@ describe('/instances', () => { commit: 'sha12', user: { id: '1234' - } + }, + bpUserId: mockReq.sessionUser.bigPoppaUser.id } ) })