Allow plugin to control where goes the output of commands executed via `CliConnection.CliCommand`
michele-mancioppi opened this issue · 14 comments
TL;DR: Has the option of allowing plugins to specify where does the output of commands they trigger go?
If it is already possible and I missed it from the code, how would it be done?
Background
I am working on a plugin that would benefit greatly from having control over where potentially very large output of another cf ... command goes (e.g., to a file).
(The plugin builds on top of cf ssh and simplifies Java-related tasks like creating and downloading locally heap-dumps and thread-dumps.)
The facts (as far as I can tell)
Plugins are allowed to execute other commands in the scope of cf ... (e.g., cf help ...) using the CliConnection.CliCommand and CliConnection.CliCommandWithoutTerminalOutput functions.
The implementation of CliConnection interface that is passed to the plugin executes the commands over RPC again an RPC Server that is controlled by the "parent" cf ... program execution that launched the plugin.
The RPC server has its own ideas in terms of where should go the output of the invoked commands.
Indeed, although the RPC server seems capable of redirecting its output, as far as I can see the plugin has absolutely no control over it.
A very inelegant solution that I could imagine for supporting this use-case would be adding to the CliConnection.CliCommand and CliConnection.CliCommandWithoutTerminalOutput functions an argument that specifies the target file path.
What would be the right way of implementing this? Is it even anything that makes sense in the concept of plugins for the cli?
We have created an issue in Pivotal Tracker to manage this:
https://www.pivotaltracker.com/story/show/139830483
The labels on this github issue will be updated when the story is started.
@michele-mancioppi I wouldn't recommend adding an additional argument to the CliConnection.CliCommand and CliConnection.CliCommandWithoutTerminalOutput methods because this would introduce a breaking change for anyone who's written plugins using these methods. Perhaps adding a new method specifically for directing the output to a file?
Can you provide a detailed example use case of what kind of output is saved to file and how it's being used/scripted against? This would help us understand what you would expect as the outcome.
Hi @nickwei84,
For example, the heap-dump of several hundreds of megabytes triggered over and streamed via a single cf ssh command, which is itself triggered via CliConnection.CliCommand.
(I'd love nothing better than to show the actual code, but right now it unfortunately not an option; hopefully soon that will change.)
Elaborating on your idea of setting up the actual target before executing the command, that he idea of configuring output to file could work well having a method on CliConnection to create a Command struct (similar to os/exec Command) that prepares the command before actually running it. On that Command struct, one would set things like the target file for the output, etc. The current CliCommand and CliCommandWithoutTerminalOutput methods would be straightforward to refactor on top of the new Command API, which I think would also be much more future-proof and intuitive for people used to the "os/exec" than the current one.
Thanks for your suggestion. There is some thought going into redesigning the plugin architecture, because it's currently difficult to develop against the existing RPC server architecture. However this is a lot of work and currently, engineering is focused on other priorities. We wouldn't be able to dedicate the engineering hours to redesign plugins properly, this this work has not been planned.
What we might be able to do is to add to the existing API to support this functionality. For example, a new method CliCommandWithRedirectOutput that takes an io.Writer as an argument and would write all output there.
Hi @nickwei84, as a short term solution until rearchitect it would work for me :-)
But then, let's make it a bufio.Writer. I think it would perhaps be a better choice, as the output of commands may be sizeable and it is a more robust API for the scenario.
Sorry for the delayed response. I was thinking it may be better to make the arg an io.Writer because this is an interface and you are free to swap out the object type passed to it. You can still pass in a bufio.Writer.
Roughly the new API method could look like this, CliCommandWithFileOutput(writer io.Writer, args ...string) ([]string, error), and any output from stderr or stdout would be written to this writer. What do you think? Our PM can prioritize this work if you are okay with this.
I suppose that allowing in anything satisfying io.Writer will cater better to use cases where input is not expected to be much, but must still go to file. I though bufio.Writer could communicate more the "here be danger" factor of large outputs, as happens with the net/http package :-)
What would the returned []string contain, if the output is supposed to go to the writer? And probably we may need also a writer for the error. Keeping stdout and stderr is very important; so would be their interleaving in some cases, but I cannot think up a good API for that in this case.
Hi Michele,
You are right about not needing to return a []string since we are writing to file. The updated API method would look like: CliCommandWithFileOutput(writer io.Writer, args ...string) error. I was thinking of using the io.Writer because the bufio.Writer struct has a Write method that implements the io.Writer interface. This way users of this API method can choose if they want to use a buffered or unbuffered writer according to their specific use case.
With the current plugin architecture, it might not be possible to return stderr and stdout separately. In the future we plan on improving the plugin architecture, but that work is currently far off.
Hi @nickwei84,
The API design sounds as good as we can make it pending the refactoring of the plugin architecture. I am looking forward to this new API being available.
For the record, the limitation of the current RPC based architecture of not differentiating stdout and stderr is also reported in #1074.
Not scheduled any longer? But whyyy?
@michele-mancioppi I got feedback from my team that the current code doesn't capture stderr at all, and it would be a great amount of work to sort it out. This is not something we'll be able to do by the scheduled release.
We'll first need to look at how we can break this work up into smaller, incremental steps before we can introduce an API to expose it.
I am willing to give up on std-err gladly, if it allows me to redirect std-out.
In any case, the std-err gets mixed up with std-out for commands executed through CliConnection (#1074), so it won't make much of a difference :-)
After a great mini-hackathlon to see if it was doable to pass down the APIs an io.Writer, turns out it requires an exploration to see how to accomplish this best.
Hi @michele-mancioppi thanks for creating this. We are closing this issue due to inactivity, however, if you want to submit a pull request, please feel free to. We do not have any current plans to make significant changes to the plugin architecture at the moment.