Support sending files to build services #23
Conversation
mjcuva
changed the title
There was a problem hiding this comment.
Can you fix the code climate issues?
Has this been tested with particularly big files? Doing streamToBuffer() might be quite memory intensive.
This PR really scares me due to the sheer amount of conversion code in here. It seems like we're constantly having to convert things between JSON/Streams/Buffers. I'm sure it's all necessary, but it's scary and seems like there's lots that could go wrong with it.
| }; | ||
| // If no data is passed in, default to {} | ||
| if (typeof data === 'function') { | ||
| callback = data; |
There was a problem hiding this comment.
It looks as though we've lost some coverage here... did this used to be covered?
There was a problem hiding this comment.
I'm pretty sure it never was
| const parsedError = JSON.parse(err); | ||
| console.log(parsedError); | ||
| } else if (Buffer.isBuffer(response)) { | ||
| process.stdout.write(response); |
There was a problem hiding this comment.
Any way to test this? Could monkey path over process.stdout.write for the test? Is this any different that doing console.log(response)?
There was a problem hiding this comment.
console.log formats the output, so for writing binary data it doesn't work. process.stdout.write just dumps the output as is
| return invoke(team.key, service, action, data, true).then((response) => { | ||
| console.log(response.body); | ||
| if (Buffer.isBuffer(response.file)) { | ||
| process.stdout.write(response.file); |
There was a problem hiding this comment.
Same as above. Maybe this piece can be abstracted out, might make it easier to test.
test/utils/file-utils.test.js
Outdated
| }); | ||
|
|
||
| it('should return false if not valid url', () => { | ||
| assert.equal(false, fileUtils.isUrl('/path/to/image')); |
There was a problem hiding this comment.
This should be the other way around assert.equal(fileUtils.isUrl('/path/to/image'), false);
|
|
||
| exports.file = (p) => { | ||
| if (exports.isUrl(p)) { | ||
| return request.get({ url: p, encoding: null }); |
There was a problem hiding this comment.
Anyway to write a test for this and mock out the response?
| exports.streamToBuffer = (s) => { | ||
| return new Promise((resolve) => { | ||
| const out = []; | ||
| s.on('data', (data) => { |
There was a problem hiding this comment.
I think we need to add an error case to this:
s.on('error', reject)
utils/file-utils.js
Outdated
| }; | ||
|
|
||
| // Converts buffers in response to file type | ||
| exports.parseFileResponse = (response) => { |
There was a problem hiding this comment.
Not sure what to do to simplify this. Maybe pull that parse reviver function outside of this function into a named one? Wondering if you came across this to do the buffer part? https://github.com/jprichardson/buffer-json Ours looks like it does more than that though....
|
|
||
| module.exports.getSecret = secrets => secret => secrets.find(s => s.key === secret).value; | ||
|
|
||
| module.exports.fixBuffers = (d) => { |
There was a problem hiding this comment.
When a file gets passed to lambda, it gets stringified into that weird buffer string format, so we have to convert it back to being a buffer
| const data = {}; | ||
| const passedData = exports.fixMinimistSpaces(args); | ||
| for (const arg of passedData) { | ||
| const i = arg.indexOf('='); |
There was a problem hiding this comment.
Can you pull this whole chunk into a separate part to simplify the complexity?
| let parsedResponse = response.body; | ||
| try { | ||
| parsedResponse = JSON.parse(response.body, (k, v) => { | ||
| return v && v.type === 'Buffer' ? Buffer.from(v.data) : v; |
There was a problem hiding this comment.
There was a problem hiding this comment.
They do slightly different things, but I think I should combine them
2 participants
Added support for sending files to build services.
Todo
api localapi link