Skip to content

Conversation

@PaulHigin
Copy link
Contributor

@PaulHigin PaulHigin commented Oct 6, 2021

PR Summary

This is a fix for Issue #15759.

PR Context

The SSH transport currently terminates a connection attempt if SSH writes any error to stdErr stream, essentially making any SSH error a terminating error. However, data stdErr is unreliable for this since it can contain warnings, the endpoint banner text, and probably other non-error related information, and this can cause an SSH connection to fail unnecessarily.

Fix is to just remove this code and only write the stdErr data to the terminal. A user can terminate an SSH connection in one of two ways;

  • Typing Ctrl+C in an interactive session
  • Using the -ConnectingTimeout parameter in the New-PSSession, Invoke-Command, Enter-PSSession cmdlets

PR Checklist

@ghost ghost assigned daxian-dbw Oct 6, 2021
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 7, 2021
@iSazonov iSazonov marked this pull request as draft October 7, 2021 05:01
@PaulHigin PaulHigin changed the title [WIP] Fix for SSH remoting when banner is enabled on SSHD endpoint Fix for SSH remoting when banner is enabled on SSHD endpoint Oct 7, 2021
@PaulHigin PaulHigin added this to the 7.2.0-preview.10 milestone Oct 7, 2021
@ghost ghost removed this from the 7.2.0-preview.10 milestone Oct 7, 2021
@ghost
Copy link

ghost commented Oct 7, 2021

Open PRs should not be assigned to milestone, so they are not assigned to the wrong milestone after they are merged. For backport consideration, use a backport label.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 11, 2021
@PaulHigin
Copy link
Contributor Author

@daxian-dbw Is there anything preventing this from being merged into vNext branch?

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 26, 2021
@PaulHigin PaulHigin added this to the 7.3-Consider milestone Oct 26, 2021
@ghost ghost removed this from the 7.3-Consider milestone Oct 26, 2021
@ghost
Copy link

ghost commented Oct 26, 2021

Open PRs should not be assigned to milestone, so they are not assigned to the wrong milestone after they are merged. For backport consideration, use a backport label.

@daxian-dbw
Copy link
Member

@PaulHigin You forgot to move this to "Ready for review", so I didn't know this is ready to be merged.
I will mark it as ready and merge it.

@daxian-dbw daxian-dbw marked this pull request as ready for review October 27, 2021 00:54
@daxian-dbw
Copy link
Member

Oh, there are conflicts. Can you please rebase your changes to resolve the conflicts?

@PaulHigin
Copy link
Contributor Author

I checked the This PR is ready to merge and is not Work in Progress.' in the check list. I didn't see a Ready for Review. Also I disagree with adding a comma to the end of the experimental list.

@daxian-dbw
Copy link
Member

I didn't see a Ready for Review.

Ilya marked your PR as draft when he left his comment:
image

I marked it "ready for review" 2 days ago when saw your comment:
image

"Ready for review" button is at the right corner of the CI part of a draft PR:
image

Also I disagree with adding a comma to the end of the experimental list.

It has been this way for previous changes to this list, for example, PSNativeCommandErrorActionPreferenceFeatureName, PSAnsiRenderingFileInfo, and PSLoadAssemblyFromNativeCode. It adds a little convenience when adding a new item at the end of the list.

@daxian-dbw daxian-dbw merged commit bd4e559 into PowerShell:master Oct 28, 2021
@PaulHigin
Copy link
Contributor Author

Just because it has been that way before doesn't mean I have to agree to it :)

@daxian-dbw
Copy link
Member

Of course :) Just wanted to clarify why I made that minor change.

@ghost
Copy link

ghost commented Dec 16, 2021

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants