-
Notifications
You must be signed in to change notification settings - Fork 1
San 6078 #2029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
San 6078 #2029
Changes from 10 commits
0fb3101
b0e9c38
7081162
9004c98
5374e39
048f077
b937e89
2208562
85796d4
e519fb7
933f315
8ef0303
11a3f75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -721,7 +721,7 @@ ContextVersionSchema.statics.updateAndGetFailedBuild = (buildId, errorMessage) = | |
| * @param {string} buildId - build id associated with context version | ||
| * @return {Promise} | ||
| */ | ||
| ContextVersionSchema.statics.updateAndGetSuccessfulBuild = (buildId) => { | ||
| ContextVersionSchema.statics.updateAndGetSuccessfulBuild = (buildId, imageData) => { | ||
| var log = logger.child({ | ||
| buildId: buildId, | ||
| method: 'updateAndGetSuccessfulBuild' | ||
|
|
@@ -736,6 +736,10 @@ ContextVersionSchema.statics.updateAndGetSuccessfulBuild = (buildId) => { | |
| } | ||
| } | ||
|
|
||
| if (imageData) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -337,11 +337,11 @@ BuildService._buildBuild = function (build, buildInfo, sessionUser) { | |
| * @param {String} buildId | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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')) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /** | ||
| * @module lib/models/services/image-service | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: [] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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' | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -103,7 +86,7 @@ class Worker { | |
| imageTag: this.dockerImageTag | ||
| }) | ||
|
|
||
| return BuildService.updateSuccessfulBuild(this.contextVersionBuildId) | ||
| return BuildService.updateSuccessfulBuild(this.contextVersionBuildId, this.inspectImageData) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -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({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(() => { | ||
|
|
@@ -329,6 +329,36 @@ describe('Context Version Unit Test', function () { | |
| done() | ||
| }) | ||
| }) | ||
|
|
||
| it('should save a successful build with image data', (done) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if possible use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update jsdoc