Skip to content
6 changes: 5 additions & 1 deletion lib/models/mongo/context-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,7 @@ ContextVersionSchema.statics.updateAndGetFailedBuild = (buildId, errorMessage) =
* @param {string} buildId - build id associated with context version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: update jsdoc

* @return {Promise}
*/
ContextVersionSchema.statics.updateAndGetSuccessfulBuild = (buildId) => {
ContextVersionSchema.statics.updateAndGetSuccessfulBuild = (buildId, imageData) => {
var log = logger.child({
buildId: buildId,
method: 'updateAndGetSuccessfulBuild'
Expand All @@ -736,6 +736,10 @@ ContextVersionSchema.statics.updateAndGetSuccessfulBuild = (buildId) => {
}
}

if (imageData) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a case where we don't get image data? we should always have it here.

update.$set.imageData = imageData
}

return ContextVersion._updateByBuildIdAndEmit(buildId, update)
}

Expand Down
28 changes: 28 additions & 0 deletions lib/models/mongo/schemas/context-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,34 @@ var ContextVersionSchema = module.exports = new Schema({
},
log: Schema.Types.Mixed,
failed: Boolean
},
imageData: {
type: {
ports: {
type: [{
protocol: {
type: String
},
port: {
type: String
}
}],
'default': []
},
cmd: [{
type: String,
'default': []
}],
entryPoint: [{
type: String,
'default': []
}]
},
'default': {
ports: [],
cmd: [],
entryPoint: []
}
}
})

Expand Down
4 changes: 2 additions & 2 deletions lib/models/services/build-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,11 @@ BuildService._buildBuild = function (build, buildInfo, sessionUser) {
* @param {String} buildId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit update jsdoc

* @return {Promise}
*/
BuildService.updateSuccessfulBuild = (buildId) => {
BuildService.updateSuccessfulBuild = (buildId, imageData) => {
var log = logger.child({ method: 'updateSuccessfulBuild' })
log.info({ buildId }, 'updateSuccessfulBuild called')

return ContextVersion.updateAndGetSuccessfulBuild(buildId)
return ContextVersion.updateAndGetSuccessfulBuild(buildId, imageData)
.tap((SuccessfullContextVersions) => {
var contextVersionIds = SuccessfullContextVersions.map(pluck('_id'))

Expand Down
56 changes: 56 additions & 0 deletions lib/models/services/image-service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* @module lib/models/services/image-service
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to docker image service

*/
'use strict'

require('loadenv')('models/services/infracode-version-service')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update name


const keypather = require('keypather')()
const logger = require('logger')
const ImageService = module.exports = {}

ImageService.logger = logger.child({
module: 'ImageService'
})

ImageService.parsePorts = function (ports) {
let portList = []
let splitPort

if (ports) {
for (let key of Object.keys(ports)) {
splitPort = key.split('/')

if (splitPort.length === 2) {
portList.push({
protocol: splitPort[1],
port: splitPort[0]
})
}
}
}

return portList
}

ImageService.getImageDataFromJob = function (job) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this method name can be changed. You are not getting image data from any job, you are getting from a particular one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, maybe we don't even need to put it in the service. Can we keep it in to the worker?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind keeping this separate since the worker is already bloated.

const log = ImageService.logger.child({
method: 'getImageDataFromJob'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add job to log data

})
log.info('called')

let rawImageData = keypather.get(job, 'inspectImageData.Config')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should always exist at this point.

if (rawImageData) {
return {
port: ImageService.parsePorts(keypather.get(rawImageData, 'ExposedPorts')),
cmd: keypather.get(rawImageData, 'Cmd') || [],
entryPoint: keypather.get(rawImageData, 'Entrypoint') || []
}
}

return {
ports: [],
cmd: [],
entryPoint: []
}
}
45 changes: 19 additions & 26 deletions lib/workers/build.container.died.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,44 +19,27 @@ const Promise = require('bluebird')
const error = require('error')
const InstanceService = require('models/services/instance-service')
const BuildService = require('models/services/build-service')
const ImageService = require('models/services/image-service')

const Isolation = require('models/mongo/isolation')
const logger = require('logger')
const rabbitMQ = require('models/rabbitmq')

const ContainerImageBuilderDied = {}

module.exports = ContainerImageBuilderDied

ContainerImageBuilderDied.jobSchema = joi.object({
from: joi.string().required(),
host: joi.string().uri({ scheme: 'http' }).required(),
id: joi.string().required(),
time: joi.number().required(),
inspectData: joi.object({
Config: joi.object({
Labels: joi.object({
'contextVersion.build._id': joi.string().required(),
'ownerUsername': joi.string().required(),
'sessionUserGithubId': joi.number().required()
}).unknown().required()
}).unknown().required()
}).unknown().required()
}).unknown().required()

class Worker {
constructor (job) {
this.log = logger.child({
job: job,
module: 'ContainerImageBuilderDied'
})

this.host = job.host
this.id = job.id
this.inspectImageData = ImageService.getImageDataFromJob(job)
this.inspectData = job.inspectData
this.contextVersionBuildId = job.inspectData.Config.Labels['contextVersion.build._id']
this.ownerUsername = job.inspectData.Config.Labels.ownerUsername
this.sessionUserGithubId = job.inspectData.Config.Labels.sessionUserGithubId
this.dockerImageTag = job.inspectData.Config.Labels.dockerTag

this.log = logger.child({
job: job,
module: 'ContainerImageBuilderDied'
})
}

/**
Expand Down Expand Up @@ -103,7 +86,7 @@ class Worker {
imageTag: this.dockerImageTag
})

return BuildService.updateSuccessfulBuild(this.contextVersionBuildId)
return BuildService.updateSuccessfulBuild(this.contextVersionBuildId, this.inspectImageData)
}

/**
Expand Down Expand Up @@ -323,6 +306,16 @@ module.exports = {
from: joi.string().required(),
host: joi.string().uri({ scheme: 'http' }).required(),
id: joi.string().required(),
time: joi.number().required(),
inspectImageData: joi.object({
exposedPorts: joi.array(joi.object({}).unknown()).default([]),
cmd: joi.array(joi.string()).default([]),
entrypoint: joi.array(joi.string()).default([])
}).default({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm I am not sure about defaulting here. If you then remove all the logic you have checking for null cases.

exposedPorts: [],
cmd: [],
entrypoint: []
}).unknown().required(),
inspectData: joi.object({
Config: joi.object({
Labels: joi.object({
Expand Down
32 changes: 31 additions & 1 deletion unit/models/mongo/context-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ describe('Context Version Unit Test', function () {
done()
})

it('should save a successful build', (done) => {
it('should save a successful build with no image data', (done) => {
const buildId = 12341

ContextVersion.updateAndGetSuccessfulBuild(buildId).asCallback(() => {
Expand All @@ -329,6 +329,36 @@ describe('Context Version Unit Test', function () {
done()
})
})

it('should save a successful build with image data', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also need unit test for empty / null ports, cmd entryPoint keys.

const buildId = 12341
const imageData = {
ports: [{
protocol: 'Omega',
port: '13'
}],
cmd: ['cmd 1'],
entryPoint: ['entry 1']
}

ContextVersion.updateAndGetSuccessfulBuild(buildId, imageData).asCallback(() => {
sinon.assert.calledOnce(ContextVersion.updateByAsync)
sinon.assert.calledWith(ContextVersion.updateByAsync,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if possible use sinon.assert.calledWithExactly - which is more strict

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was able to switch it below but this one causes issues.

'build._id',
buildId, {
$set: {
'build.failed': false,
'build.completed': sinon.match.number,
'state': ContextVersion.states.buildSucceeded,
'imageData': imageData
}
})

sinon.assert.calledOnce(ContextVersion.findByAsync)
sinon.assert.calledWith(ContextVersion.findByAsync, 'build._id', buildId)
done()
})
})
}) // end updateAndGetSuccessfulBuild

describe('findOneCreating', function () {
Expand Down