-
Notifications
You must be signed in to change notification settings - Fork 129
Test-Connection Changes #172
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
Changes from 2 commits
2633d63
f2d2fbc
67d894f
771e964
f6dc579
6b92b9a
deb5d3f
7a3af51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
--- | ||
RFC: XXXX | ||
Author: Joel Sallow | ||
Status: Draft | ||
Version: 1.0 | ||
Area: Cmdlets | ||
Comments Due: 6/10/19 | ||
Plan to Implement: Yes | ||
--- | ||
|
||
# Proposed Changes to `Test-Connection` Cmdlet | ||
|
||
## Syntax | ||
|
||
``` | ||
# All Sets | ||
Test-Connection [-TargetName] <string[]> [-IPv4] [-IPv6] [-ResolveDestination] [-TimeoutSeconds <int>] [-Quiet] [<CommonParameters>] | ||
|
||
# Set 1 | ||
[-Ping] [-Count <int>] [-Delay <int>] [-Source <string>] [-MaxHops <int>] [-BufferSize <int>] [-DontFragment] | ||
|
||
# Set 2 | ||
[-Ping] [-Continues] [-Delay <int>] [-Source <string>] [-MaxHops <int>] [-BufferSize <int>] [-DontFragment] | ||
|
||
# Set 3 | ||
Test-Connection [-TargetName] <string[]> -Traceroute [-Source <string>] [-MaxHops <int>] | ||
vexx32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Set 4 | ||
Test-Connection [-TargetName] <string[]> -MTUSizeDetect | ||
vexx32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Set 5 | ||
Test-Connection [-TargetName] <string[]> -TCPPort <int> [-Source <string>] | ||
vexx32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
## Changes | ||
|
||
1. All current host/information output is relegated to the `-Verbose` stream. | ||
2. No progress bar per committee decision in [PowerShell#6768](https://github.com/PowerShell/PowerShell/issues/6768). | ||
3. All output occurs as soon as data is available, individual records per ping reply / trace hop. | ||
4. All presented data should be readily available as properties to the user. | ||
|
||
## Output Type Proposed Members | ||
|
||
### PingStatus | ||
|
||
```csharp | ||
class PingStatus | ||
{ | ||
string Source { get; } | ||
IPAddress Address { get; } | ||
string Destination { get; } | ||
int RoundtripTime { get; } | ||
int BufferSize { get; } | ||
PingOptions Options { get; } | ||
} | ||
|
||
// For -MtuSizeDetect switch | ||
class PingMtuDetect : PingStatus | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you consider just having MtuSize being a member of PingStatus but $null if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do this, absolutely. However, that means that the default format view with the default behaviour will always have an empty column. That strikes me as... Odd? Also, given the cmdlet's current behaviour with Definitely open to alternate solutions though! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've reminded me that @powercode has a PR to only show properties of MeasureInfo objects that have values. We can do the same thing here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you link me to that? I wasn't aware that the formatter supported this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PowerShell/PowerShell#7104, this was merged but was reverted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we'd need to put at least those methods into the code base for that to happen. @powercode, would you be able to PR just those formatting methods in so that we can make use of them? I'm not familiar with why it was reverted, but I'm sure we could stand to use a feature like that in more places than just here. 🙂 EDIT: Seeing references to a security review. I guess that means we're waiting on a green light from... @PaulHigin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SteveL-MSFT I looked over these again -- I know you opened an issue for it in the PS repo -- and I realize we're going to hit an issue here; the reason this is fairly easily doable for MeasureInfo is that the format used is a List format, not a Table format. Implementing the same thing in a table format is... problematic at best. |
||
{ | ||
int MtuSize { get; } | ||
} | ||
``` | ||
|
||
### TraceStatus | ||
|
||
```csharp | ||
class TraceStatus | ||
{ | ||
int Hop { get; } | ||
int[] RoundTripTimes { get; } | ||
string Destination { get; } | ||
IPAddress DestinationAddress { get; } | ||
IPAddress HopAddress { get; } | ||
vexx32 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int BufferSize { get; } | ||
PingStatus[] Replies { get; } | ||
} | ||
``` | ||
|
||
## Mockups | ||
|
||
### `Test-Connection www.google.com` | ||
|
||
- Information from the `Replies` property of the output object should be included in the main output object, and primary output mode should be _multiple_ objects, each representing a single ping attempt/reply object. | ||
- Buffer _data_ is generally irrelevant, as it cannot be specified, and only a `BufferSize` property should be exposed. `Replies.Buffer` property should remain `private`. | ||
- `Replies.Options` property should be hidden from default formatting. | ||
- Results output as a table, grouped by Destination address (in the case that multiple destinations are specified). | ||
|
||
**Resulting output:** | ||
```code | ||
Destination: www.google.com | ||
Source Address RoundtripTime BufferSize | ||
SteveL-MSFT marked this conversation as resolved.
Show resolved
Hide resolved
|
||
------ ------- ------------- ---------- | ||
WS-JOEL 172.217.2.132 36 32 | ||
WS-JOEL 172.217.2.132 21 32 | ||
WS-JOEL 172.217.2.132 25 32 | ||
WS-JOEL 172.217.2.132 25 32 | ||
``` | ||
|
||
### `Test-Connection www.google.com -TraceRoute` | ||
|
||
- Each hop should be output as a separate object, each containing the `PingReply` objects as a property accessible by hidden from formatting. | ||
- Main `TraceRouteResult` object should contain either ETS or class properties that calculate summary data from their four PingReplies. | ||
- **Note:** This object type we're using is currently **bugged**, and all `PingReply` objects report `TtlExpired` as their status. Recommend investigating progress of fix for .NET Core 3, or designing custom solution for TraceRoute backing to resolve the issue. | ||
- Output as a table, grouped by `DestinationHost` (Why is this property name different to that of the other object type used for standard pings?) | ||
|
||
**Resulting output:** | ||
```code | ||
Destination: www.google.com | ||
Hop RoundtripTimes DestinationAddress HopAddress BufferSize | ||
--- -------------- ------------------ ---------- ---------- | ||
1 {0, 0, 0} 172.217.2.132 192.168.22.254 | ||
2 {0, 0, 0} 172.217.2.132 75.144.219.238 | ||
3 {0, 0, 0} 172.217.2.132 96.120.37.17 | ||
4 {0, 0, 0} 172.217.2.132 96.110.136.65 | ||
5 {0, 0, 0} 172.217.2.132 69.139.180.170 | ||
6 {0, 0, 0} 172.217.2.132 68.85.127.121 | ||
7 {0, 0, 0} 172.217.2.132 68.86.165.161 | ||
8 {0, 0, 0} 172.217.2.132 68.86.90.205 | ||
9 {0, 0, 0} 172.217.2.132 68.86.82.154 | ||
10 {0, 0, 0} 172.217.2.132 66.208.233.242 | ||
11 {0, 0, 0} 172.217.2.132 0.0.0.0 | ||
12 {0, 0, 0} 172.217.2.132 216.239.59.124 | ||
13 {0, 0, 0} 172.217.2.132 216.239.59.61 | ||
14 {32, 28, 20} 172.217.2.132 172.217.2.132 | ||
``` |
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.
This would be the default parameter set, correct?
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.
Yep!