Skip to content
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

Merged
merged 7 commits into from
Mar 27, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion node/toolrunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Copy link
Contributor

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?

if (fileStream) {
fileStream.write(this._getCommandString(options) + os.EOL);
Copy link
Contributor

@ericsciple ericsciple Mar 22, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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());
Copy link
Contributor

Choose a reason for hiding this comment

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

why .toString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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());
Copy link
Contributor

Choose a reason for hiding this comment

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

same question about .toString()

}
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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

copy paste error? shouldn't this write stderr to the file too?

Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is reliable. Do you know whether close means no further stdout/stderr data events are pending to be drained? I believe the node docs have good details about this, but I don't remember the details offhand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand Down