Passing arguments more securly via stdin #134
Conversation
- Works for CLC and TF.EXE - Fixed a related bug where the model was being refreshed too soon before everything was setup - Had to add code to put arguments on a single line in the ArgBuilder - Added tests to maintain code coverage
|
@jpricketMSFT, |
| } | ||
| } | ||
|
|
||
| public async Run(args: IArgumentProvider, options: any, isExe: boolean): Promise<IExecutionResult> { |
There was a problem hiding this comment.
It'd be great to have a comment about what's going on here. After decoding it and recalling what you mentioned IntelliJ does, it makes more sense. Something along the lines of "we're using the currently running instance to run the command but immediately start a new one so the next command can use it."
| // spawn a new instance of TF with these options | ||
| await this.startNewTfInstance(options); | ||
| } | ||
| return this._runningInstance; |
There was a problem hiding this comment.
I get what's going on here, but it seems a bit odd to be returning a private member variable. Nothing to do here, I guess.
There was a problem hiding this comment.
Yeah, in IntelliJ we had a hash table based on working directory so this method made a bit more sense. But here the working directory is almost always the same, so I didn't try to create a map. Maybe we can discuss this when you are back in the office. No rush to get this in.
| private async startNewTfInstance(options: any) { | ||
| // Start up a new instance of TF for later use | ||
| this._options = options; | ||
| this._runningInstance = await this.spawn(options); |
There was a problem hiding this comment.
To be consistent, perhaps startNewTfInstance could return the process from this.spawn(...) and the above would return that value.
| } | ||
|
|
||
| private optionsMatch(options1: any, options2: any): boolean { | ||
| return (!options1 && !options2) || (options1.cwd === options2.cwd); |
There was a problem hiding this comment.
Is it enough to just look at 'cwd'? Is 'cwd' the folder that's open in VS Code?
There was a problem hiding this comment.
enough? for now yes, the only thing that makes the command line launched invalid for the current workspace is if the working directory changed for some reason. If a new folder is opened, the tfrunner is disposed and a new one created, so that should work fine.
| // spawn a new instance of TF with these options | ||
| await this.startNewTfInstance(options); | ||
| } | ||
| return this._runningInstance; |
There was a problem hiding this comment.
Yeah, in IntelliJ we had a hash table based on working directory so this method made a bit more sense. But here the working directory is almost always the same, so I didn't try to create a map. Maybe we can discuss this when you are back in the office. No rush to get this in.
| } | ||
|
|
||
| private optionsMatch(options1: any, options2: any): boolean { | ||
| return (!options1 && !options2) || (options1.cwd === options2.cwd); |
There was a problem hiding this comment.
enough? for now yes, the only thing that makes the command line launched invalid for the current workspace is if the working directory changed for some reason. If a new folder is opened, the tfrunner is disposed and a new one created, so that should work fine.
User Story #930304 and #930313