Skip to content
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

feat(command): add connection direction #5457

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

overbool
Copy link
Contributor

License: MIT
Signed-off-by: Overbool [email protected]

Fixes: #5443

License: MIT
Signed-off-by: Overbool <[email protected]>
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -101,6 +100,9 @@ var swarmPeersCmd = &cmds.Command{
*/

if verbose || latency {
// set direction
ci.Direction = c.Stat().Direction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of overloading the latency flag, let's just add a --direction flag. I'm usually all for reducing the number of flags we have but tying latency to connection direction feels a bit weird.

Peer: pid.Pretty(),
Addr: addr.String(),
Peer: pid.Pretty(),
Direction: inet.Direction(-1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just let this default to 0 ("unknown"). By using -1, we're changing the API a bit: new clients running against old servers will see Direction: 0 (instead of Direction: -1) even if they haven't requested the direction. When they do, they'll print the direction as if the user had requested it. See my comment at https://github.com/ipfs/go-ipfs/pull/5457/files#diff-ab43cdc4b214774869f9dc156ebc0fa0R152

@@ -146,6 +148,11 @@ var swarmPeersCmd = &cmds.Command{
if info.Latency != "" {
fmt.Fprintf(buf, " %s", info.Latency)
}

if info.Direction >= 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just check to see if the user has passed the appropriate flag (either --direction or --verbose). That way we can keep the default direction as 0.

Alternatively, we can just not print anything if the direction is unknown. Either way is fine by me.

@Stebalien Stebalien added status/in-progress In progress need/author-input Needs input from the original author labels Sep 13, 2018
@overbool overbool force-pushed the feat/command/add-connection-direction branch from fff878d to daf6fbd Compare September 14, 2018 00:49
@overbool
Copy link
Contributor Author

@Stebalien Thank you for your advice. could you help me review my pr again?

@Stebalien Stebalien requested a review from keks September 14, 2018 00:56
@Stebalien Stebalien added need/review Needs a review and removed need/author-input Needs input from the original author labels Sep 14, 2018
@Stebalien
Copy link
Member

Given that this is a change to a public API, I'd like to get one more sign off. I'm puling a few others to see if they have any options.

@@ -146,6 +151,11 @@ var swarmPeersCmd = &cmds.Command{
if info.Latency != "" {
fmt.Fprintf(buf, " %s", info.Latency)
}

if info.Direction != inet.DirUnknown {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (feel free to ignore): can we check for the direction or verbose flag here? I prefer not to rely on default (unset) values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: #5457 (comment). We've explicitly defined the zero value of Direction to be DirUnknown so this should always work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTY or would you actually like to print "unknown" if --direction is passed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTY or would you actually like to print "unknown" if --direction is passed?

I'm fine either way ("unknown" or any non-empty string would make the output easier to parse in case we add more information fields in the future), my original comment was more about the fact that we are overloading the DirUnknown value to mean either unknown and/or unset, but's just a nit, I'm fine with the patch as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see your point. The original patch used -1 but I that would change the API (go tends to assume missing == 0 so old go-ipfs daemons would return 0 instead of -1).

I'm just going to go with this for now as I think it's the simplest solution. If it continues to bug you, we can do a separate PR.

(note: users should be parsing ipfs --enc=json swarm peers if they know what's good for them...).

@Stebalien Stebalien merged commit cbc6e69 into ipfs:master Sep 14, 2018
@ghost ghost removed the status/in-progress In progress label Sep 14, 2018
@overbool overbool deleted the feat/command/add-connection-direction branch September 14, 2018 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants