Basic Peer #7

bfbachmann · 2017-05-06T18:53:27Z

What Changed:

Added cumuluspeer with MakeBasicHost functionality
Updated main.go to take host port number, IP address and target peer to connect to as command line arguments
Added tests for cumuluspeer
Added documentation for cumuluspeer to Wiki

Remaining TODOs

Update main and BasicStreamHandler to actually communicate useful information between peer and host

Create cumuluspeer BasicStreamHandler and ExtractPeerInfo Add logger Main now parses command line arguments specifying multiaddress of peer to connect to

jordanschalm

Good job for your first Go code!

jordanschalm · 2017-05-06T20:58:15Z

cumuluspeer/peer.go

@@ -0,0 +1,145 @@
+package cumuluspeer


Just call this peer. We know it's cumulus because inside the cumulus folder. Short and succinct package names

jordanschalm · 2017-05-06T21:00:35Z

cumuluspeer/peer.go

+const (
+	DefaultPort 	= 8765 // Cumulus peers communicate over this TCP port
+	CumulusProtocol	= "/cumulate/0.0.1" // Cumulus communication protocol
+	DefaultIP		= "127.0.0.1" // Default Host IP address if none given


Run gofmt here.

jordanschalm · 2017-05-06T21:00:52Z

cumuluspeer/peer.go

+
+const (
+	DefaultPort 	= 8765 // Cumulus peers communicate over this TCP port
+	CumulusProtocol	= "/cumulate/0.0.1" // Cumulus communication protocol


Why cumulate instead of cumulus?

jordanschalm · 2017-05-06T21:02:38Z

cumuluspeer/peer.go

+// Create a Cumulus host.
+// This may throw an error if we fail to create a key pair, a pid, or a new
+// multiaddress.
+func MakeBasicHost(ip string, port int) (host.Host, pstore.Peerstore, error) {


You should wrap host.Host with your own struct peer.Peer and return that here. Also use the pattern NewPeer rather than MakePeer

jordanschalm · 2017-05-06T21:03:24Z

cumuluspeer/peer.go

+// Communicate with peers.
+// TODO: Update this to do something useful. For now it just reads from the
+// stream and writes back what it read.
+func doCumulate(s net.Stream) {


This should be a method on the Peer struct
func (p *Peer) Receive(s net.String) {...}

jordanschalm · 2017-05-06T21:11:18Z

cumuluspeer/peer_test.go

@@ -0,0 +1,38 @@
+package cumuluspeer_test


all files in the same folder have the same package name

jordanschalm · 2017-05-06T21:11:26Z

cumuluspeer/peer_test.go

+import (
+    "testing"
+
+    cumuluspeer "github.com/ubclaunchpad/cumulus/cumuluspeer"


So we don't need this import

jordanschalm · 2017-05-06T21:11:36Z

cumuluspeer/peer_test.go

+)
+
+// Tests if we can make a basic host on a valid TCP port
+func TestMakeBasicHostValidPort(t *testing.T) {


jordanschalm · 2017-05-06T21:14:36Z

main.go

+    if *targetPeer == "" {
+        // No target was specified, wait for incoming connections
+        log.Info("No target provided. Listening for incoming connections...")
+        select {} // Hang until someone connects to us


This will hang this goroutine forever, is that what we want?

jordanschalm · 2017-05-06T21:15:58Z

main.go

+    // when joining the Cumulus Network.
+    // port is the port to communicate over (defaults to peer.DefaultPort).
+    // ip is the public IP address of the this host.
+    targetPeer := flag.String("t", "", "target peer to connect to")


If we know the IP and the port of the multiaddr can we infer the rest? A multiaddr as a CLI flag is an awkward ui

chadlagore · 2017-05-06T21:24:52Z

cumuluspeer/peer.go

+	doCumulate(s)
+}
+
+// Communicate with peers.


Re: https://golang.org/doc/effective_go.html#commentary. Function comments start with function name, ie // doCumulate communicates with peers.

The first sentence should be a one-sentence summary that starts with the name being declared. If every doc comment begins with the name of the item it describes, the output of godoc can usefully be run through grep.

coveralls · 2017-05-07T18:54:42Z

Coverage decreased (-32.7%) to 67.347% when pulling 4a89e2c on 1-cumulus-peer into 8fb32b0 on dev.

into 1-cumulus-peer

jordanschalm · 2017-05-08T04:54:35Z

peer/peer.go

+	// Open a stream with the peer
+	stream, err := p.NewStream(context.Background(), peerid,
+		CumulusProtocol)
+	if err != nil {


Just return stream, err if you aren't handling error

jordanschalm · 2017-05-09T03:41:46Z

peer/peer.go

+// NewPeer creates a Cumulus host with the given IP addr and TCP port.
+// This may throw an error if we fail to create a key pair, a pid, or a new
+// multiaddress.
+func NewPeer(ip string, port int) (*Peer, error) {


If you're making a package with one primary export (ie Peer) then just call the constructor New. Then you're calling peer.New(...) -- makes more sense semantically

coveralls · 2017-05-11T06:03:55Z

Coverage decreased (-34.7%) to 65.306% when pulling 8b255ad on 1-cumulus-peer into 8fb32b0 on dev.

coveralls · 2017-05-13T21:10:41Z

Coverage decreased (-33.6%) to 66.434% when pulling 39543d8 on 1-cumulus-peer into 8fb32b0 on dev.

bfbachmann added 9 commits April 30, 2017 18:42

Add simple peer and swarm creation

2336b05

Update cumuluspeer and add logger

1c4278d

Create cumuluspeer BasicStreamHandler and ExtractPeerInfo Add logger Main now parses command line arguments specifying multiaddress of peer to connect to

Switch logger to Logrus, add port selection through CLI

784cd06

Merge remote-tracking branch 'origin/dev' into 1-cumulus-peer

c7b6185

Update travis, add MakeBasicHostTest

6832cc3

Update travis.yml

fd96a01

Update travis.yml again (maybe this time?)

124f0c9

Yet another permutation of travis settings

d9c9e51

Update peer tests and add IP address flag

9807889

bfbachmann requested a review from jordanschalm May 6, 2017 19:04

bfbachmann assigned chadlagore May 6, 2017

bfbachmann requested a review from chadlagore May 6, 2017 19:05

bfbachmann unassigned chadlagore May 6, 2017

jordanschalm suggested changes May 6, 2017

View reviewed changes

chadlagore reviewed May 6, 2017

View reviewed changes

bfbachmann and others added 3 commits May 6, 2017 18:27

Update travis, refactor peer, add tests

4bac689

Add peer.Connect(peer), add peer test

a013db6

test goveralls

4a89e2c

jordanschalm and others added 5 commits May 7, 2017 11:56

Remove master build

db71ad5

Fix net/identi error in peer

6bfe882

Merge branch '1-cumulus-peer' of https://github.com/ubclaunchpad/cumulus

0b634f2

into 1-cumulus-peer

Attempt to fix key mismatch error

beb45fa

Remove goveralls because I'm sure it's the problem

4fb47c0

jordanschalm reviewed May 8, 2017

View reviewed changes

jordanschalm reviewed May 9, 2017

View reviewed changes

Restore travis, update TestReceiveInvalidMessage

4240e1a

jordanschalm mentioned this pull request May 11, 2017

Vendor dependencies with Glide #12

Closed

Weakened failing test slightly

8b255ad

Avoid SIGSEGV when invalid addrs are given

4b43c00

bfbachmann changed the title ~~1 cumulus peer~~ Basic Peer May 13, 2017

Change NewPeer() to New()

39543d8

jordanschalm merged commit 138b4a4 into dev May 13, 2017

jordanschalm deleted the 1-cumulus-peer branch May 13, 2017 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic Peer #7

Basic Peer #7

bfbachmann commented May 6, 2017 •

edited

jordanschalm left a comment

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

jordanschalm May 6, 2017

chadlagore May 6, 2017 •

edited

coveralls commented May 7, 2017

jordanschalm May 8, 2017

jordanschalm May 9, 2017

coveralls commented May 11, 2017 •

edited

coveralls commented May 13, 2017

Basic Peer #7

Basic Peer #7

Conversation

bfbachmann commented May 6, 2017 • edited

What Changed:

Remaining TODOs

jordanschalm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadlagore May 6, 2017 • edited

Choose a reason for hiding this comment

coveralls commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 11, 2017 • edited

coveralls commented May 13, 2017

bfbachmann commented May 6, 2017 •

edited

chadlagore May 6, 2017 •

edited

coveralls commented May 11, 2017 •

edited