-
Notifications
You must be signed in to change notification settings - Fork 274
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
Add option to pipe to file and another tool #329
Changes from 1 commit
22e9d33
4c75ff7
288a43f
953cf2e
05038e6
d99b570
057056b
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 |
---|---|---|
|
@@ -7,6 +7,7 @@ import child = require('child_process'); | |
import stream = require('stream'); | ||
import im = require('./internal'); | ||
import tcm = require('./taskcommand'); | ||
import fs = require('fs'); | ||
|
||
/** | ||
* Interface for exec options | ||
|
@@ -73,6 +74,7 @@ export class ToolRunner extends events.EventEmitter { | |
private toolPath: string; | ||
private args: string[]; | ||
private pipeOutputToTool: ToolRunner; | ||
private pipeOutputToFile: string; | ||
|
||
private _debug(message) { | ||
this.emit('debug', message); | ||
|
@@ -528,10 +530,12 @@ export class ToolRunner extends events.EventEmitter { | |
/** | ||
* Pipe output of exec() to another tool | ||
* @param tool | ||
* @param file optional filename to additionally stream the output to. | ||
* @returns {ToolRunner} | ||
*/ | ||
public pipeExecOutputToTool(tool: ToolRunner) : ToolRunner { | ||
public pipeExecOutputToTool(tool: ToolRunner, file?: string) : ToolRunner { | ||
this.pipeOutputToTool = tool; | ||
this.pipeOutputToFile = file; | ||
return this; | ||
} | ||
|
||
|
@@ -584,27 +588,48 @@ export class ToolRunner extends events.EventEmitter { | |
this.pipeOutputToTool._getSpawnArgs(options), | ||
this.pipeOutputToTool._getSpawnOptions(options)); | ||
|
||
let fileStream: fs.WriteStream = this.pipeOutputToFile ? fs.createWriteStream(this.pipeOutputToFile) : null; | ||
if (fileStream) { | ||
fileStream.write(this._getCommandString(options) + os.EOL); | ||
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 it weird to write the command string? I would expect the file to only contain the output of the first tool. 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. The command string helps set context for what this output was for. Otherwise user has to use the step number to match the output especially if there are multiple tasks of the same type that are producing these files. I initially did not write it but after I tried e2e it made more sense to write it. 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. we cant do this. it may contain secrets and does not get run through the log scrubber 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. Ah good point! |
||
} | ||
|
||
//pipe stdout of first tool to stdin of second tool | ||
cpFirst.stdout.on('data', (data: Buffer) => { | ||
try { | ||
if (fileStream) { | ||
fileStream.write(data.toString()); | ||
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. why 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. that's right, it should not be needed. I will pull it out and verify. |
||
} | ||
cp.stdin.write(data); | ||
} catch (err) { | ||
this._debug('Failed to pipe output of ' + toolPathFirst + ' to ' + toolPath); | ||
this._debug(toolPath + ' might have exited due to errors prematurely. Verify the arguments passed are valid.'); | ||
} | ||
}); | ||
cpFirst.stderr.on('data', (data: Buffer) => { | ||
if (fileStream) { | ||
fileStream.write(data.toString()); | ||
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. same question about |
||
} | ||
successFirst = !options.failOnStdErr; | ||
if (!options.silent) { | ||
var s = options.failOnStdErr ? options.errStream : options.outStream; | ||
s.write(data); | ||
} | ||
}); | ||
cpFirst.on('error', (err) => { | ||
if (fileStream) { | ||
fileStream.on('finish', () => { | ||
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. copy paste error? shouldn't this write stderr to the file too? 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. oops I misread this. |
||
fileStream.close(); | ||
}); | ||
} | ||
cp.stdin.end(); | ||
defer.reject(new Error(toolPathFirst + ' failed. ' + err.message)); | ||
}); | ||
cpFirst.on('close', (code, signal) => { | ||
if (fileStream) { | ||
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 wonder if this is reliable. Do you know whether 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 should be as per https://nodejs.org/api/child_process.html#child_process_event_close - The 'close' event is emitted when the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams. 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 wrote tests and found that there were some timing issues. I fixed the issues but the code looks ugly. Will sync up before doing any refactoring. |
||
fileStream.on('finish', () => { | ||
fileStream.close(); | ||
}); | ||
} | ||
if (code != 0 && !options.ignoreReturnCode) { | ||
successFirst = false; | ||
returnCodeFirst = code; | ||
|
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.
will this work if the file already exists? It should append in that case I guess?