Uh oh!
There was an error while loading. Please reload this page.
San 6078 - #2029
Conversation
* master: (134 commits) Replacing the npm tokens 12.5.1 update model Added catch to prevent keymaker from preventing build process 12.5.0 Revert "Revert "San 6200 add ssh keys"" Remove session user from call Revert "San 6200 add ssh keys" Add a migrate-and-start command Use saveAsync() fix seedVersions for callback remove unused func param add migration stuff into readme fix migrate up again 12.4.3 add migrate-up to start 12.4.2 Added newlines and added comments Still messed up which functions went where Changing invalid mount type. r to ro ...
| return portList | ||
| } | ||
| ImageService.getImageDataFromJob = function (job) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, maybe we don't even need to put it in the service. Can we keep it in to the worker?
There was a problem hiding this comment.
I don't mind keeping this separate since the worker is already bloated.
podviaznikov
left a comment
There was a problem hiding this comment.
Looks good. Few minor questions
| ImageService.getImageDataFromJob = function (job) { | ||
| const log = ImageService.logger.child({ | ||
| method: 'getImageDataFromJob' |
| return portList | ||
| } | ||
| ImageService.getImageDataFromJob = function (job) { |
There was a problem hiding this comment.
Also, maybe we don't even need to put it in the service. Can we keep it in to the worker?
| } | ||
| } | ||
| if (imageData) { |
There was a problem hiding this comment.
is there a case where we don't get image data? we should always have it here.
| @@ -0,0 +1,56 @@ | |||
| /** | |||
| * @module lib/models/services/image-service | |||
There was a problem hiding this comment.
nit: rename to docker image service
| */ | ||
| 'use strict' | ||
| require('loadenv')('models/services/infracode-version-service') |
| return portList | ||
| } | ||
| ImageService.getImageDataFromJob = function (job) { |
There was a problem hiding this comment.
I don't mind keeping this separate since the worker is already bloated.
| }) | ||
| log.info('called') | ||
| let rawImageData = keypather.get(job, 'inspectImageData.Config') |
There was a problem hiding this comment.
this should always exist at this point.
| exposedPorts: joi.array(joi.object({}).unknown()).default([]), | ||
| cmd: joi.array(joi.string()).default([]), | ||
| entrypoint: joi.array(joi.string()).default([]) | ||
| }).default({ |
There was a problem hiding this comment.
hmmm I am not sure about defaulting here. If you then remove all the logic you have checking for null cases.
| }) | ||
| }) | ||
| it('should save a successful build with image data', (done) => { |
There was a problem hiding this comment.
also need unit test for empty / null ports, cmd entryPoint keys.
| @@ -337,11 +337,11 @@ BuildService._buildBuild = function (build, buildInfo, sessionUser) { | |||
| * @param {String} buildId | |||
| @@ -721,7 +721,7 @@ ContextVersionSchema.statics.updateAndGetFailedBuild = (buildId, errorMessage) = | |||
| * @param {string} buildId - build id associated with context version | |||
| for (let key of Object.keys(ports)) { | ||
| splitPort = key.split('/') | ||
| if (splitPort.length === 2) { |
There was a problem hiding this comment.
this check is not needed. we will always have 2 here
| let rawImageData = keypather.get(job, 'inspectImageData.Config') | ||
| return { | ||
| port: this._parsePorts(keypather.get(rawImageData, 'ExposedPorts')), | ||
| cmd: keypather.get(rawImageData, 'Cmd') || [], |
There was a problem hiding this comment.
remove || [] since you default them in the joi.. BUT also double check defaulting in Joi works, not sure we pass the joi data the job here.
| ContextVersion.updateAndGetSuccessfulBuild(buildId, imageData).asCallback(() => { | ||
| sinon.assert.calledOnce(ContextVersion.updateByAsync) | ||
| sinon.assert.calledWith(ContextVersion.updateByAsync, |
There was a problem hiding this comment.
if possible use sinon.assert.calledWithExactly - which is more strict
There was a problem hiding this comment.
Was able to switch it below but this one causes issues.
| }) | ||
| sinon.assert.calledOnce(ContextVersion.findByAsync) | ||
| sinon.assert.calledWith(ContextVersion.findByAsync, 'build._id', buildId) |
There was a problem hiding this comment.
if possible use sinon.assert.calledWithExactly - which is more strict
| ContextVersion.updateAndGetSuccessfulBuild(buildId, imageData).asCallback(() => { | ||
| sinon.assert.calledOnce(ContextVersion.updateByAsync) | ||
| sinon.assert.calledWith(ContextVersion.updateByAsync, |
There was a problem hiding this comment.
if possible use sinon.assert.calledWithExactly - which is more strict
There was a problem hiding this comment.
Was able to switch it below but this one causes issues.
| }) | ||
| sinon.assert.calledOnce(ContextVersion.findByAsync) | ||
| sinon.assert.calledWith(ContextVersion.findByAsync, 'build._id', buildId) |
There was a problem hiding this comment.
if possible use sinon.assert.calledWithExactly - which is more strict
Saving the image data on the context version.