-
Notifications
You must be signed in to change notification settings - Fork 0
Az.Ssh Preview 2 #3
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
base: main
Are you sure you want to change the base?
Conversation
| sshProcess.StartInfo.RedirectStandardError = true; | ||
| sshProcess.Start(); | ||
|
|
||
| bool sshSuccess = WaitSSHTunnelConnection(sshProcess); |
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.
Should there be a timeout for this connection wait? Can this wait be interrupted with Ctl+C?
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.
Yes, it can be interrupted with a Ctrl+c, and in that case the function returns false and the cmdlet exits. I'm not totally sure if having a timeout is necessary since this function exits once the ssh process exits, and ssh has its own timeout. But I'll look into it and make sure.
|
|
||
| if (SshLogsPrinted() || IsDebugMode()) | ||
| { | ||
| var stderr = sshProcess.StandardError; |
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.
Is stderr still valid after the process exits? Can stderr.Readline() throw?
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.
Yes, stderr is still valid once the process exits. You can still read any messages that were sent to stderr in between the last time you read stderr and the moment the process exits.
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.
I don't think stderr.Readline() can throw as long as stderr is valid. I can add a check to make sure stderr is not null.
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.
I believe stderr is a StreamReader, in which case it can throw a IOException exception. Not sure if you want to catch it or let it propagate.
…ype. Co-authored-by: Paul Higinbotham <paulhi@microsoft.com>
Co-authored-by: Beisi Zhou <zhoubeisi@gmail.com>
Description
Checklist
CONTRIBUTING.mdand reviewed the following information:generationbranch.ChangeLog.mdfile(s) appropriatelyChangeLog.mdfile can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md## Upcoming Releaseheader in the past tense. Add changelog in description section if PR goes intogenerationbranch.ChangeLog.mdif no new release is required, such as fixing test case only.