From b3b675d108d02f32b24ad69b33f1dacdd5e85c1d Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 21 Sep 2021 12:44:30 -0400 Subject: [PATCH] Merge NewClient and JoinWorkspace into Connect --- client.go | 105 +++++++++++++++++++++++++----------------------- client_test.go | 46 ++------------------- connection.go | 2 +- session_test.go | 9 +---- 4 files changed, 61 insertions(+), 101 deletions(-) diff --git a/client.go b/client.go index ba9d2f5e7..3f9345ce4 100644 --- a/client.go +++ b/client.go @@ -1,3 +1,13 @@ +// Package liveshare is a Go client library for the Visual Studio Live Share +// service, which provides collaborative, distibuted editing and debugging. +// See https://docs.microsoft.com/en-us/visualstudio/liveshare for an overview. +// +// It provides the ability for a Go program to connect to a Live Share +// workspace (Connect), to expose a TCP port on a remote host +// (UpdateSharedVisibility), to start an SSH server listening on an +// exposed port (StartSSHServer), and to forward connections between +// the remote port and a local listening TCP port (ForwardToListener) +// or a local Go reader/writer (Forward). package liveshare import ( @@ -9,66 +19,79 @@ import ( "golang.org/x/crypto/ssh" ) -// A Client capable of joining a Live Share workspace. -type Client struct { +// A client capable of joining a Live Share workspace. +type client struct { connection Connection tlsConfig *tls.Config } -// A ClientOption is a function that modifies a client -type ClientOption func(*Client) error +// An Option updates the initial configuration state of a Live Share connection. +type Option func(*client) error -// NewClient accepts a range of options, applies them and returns a client -func NewClient(opts ...ClientOption) (*Client, error) { - client := new(Client) - - for _, o := range opts { - if err := o(client); err != nil { - return nil, err - } - } - - return client, nil -} - -// WithConnection is a ClientOption that accepts a Connection -func WithConnection(connection Connection) ClientOption { - return func(c *Client) error { +// WithConnection is a Option that accepts a Connection. +// +// TODO(adonovan): WithConnection is not optional, so it should not be +// not an Option. We should make Connection a mandatory parameter of +// Connect, at which point, why not just merge +// client+Option+Connection, rename it to Options, do away with the +// function mechanism, and express TLS config (etc) as public fields +// of Options with sensible zero values, like websocket.Dialer, etc? +func WithConnection(connection Connection) Option { + return func(cli *client) error { if err := connection.validate(); err != nil { return err } - c.connection = connection + cli.connection = connection return nil } } -func WithTLSConfig(tlsConfig *tls.Config) ClientOption { - return func(c *Client) error { - c.tlsConfig = tlsConfig +// WithTLSConfig returns a Connect option that sets the TLS configuration. +func WithTLSConfig(tlsConfig *tls.Config) Option { + return func(cli *client) error { + cli.tlsConfig = tlsConfig return nil } } -// JoinWorkspace connects the client to the server's Live Share -// workspace and returns a session representing their connection. -func (c *Client) JoinWorkspace(ctx context.Context) (*Session, error) { - span, ctx := opentracing.StartSpanFromContext(ctx, "Client.JoinWorkspace") +// Connect connects to a Live Share workspace specified by the +// options, and returns a session representing the connection. +// The caller must call the session's Close method to end the session. +func Connect(ctx context.Context, opts ...Option) (*Session, error) { + cli := new(client) + for _, opt := range opts { + if err := opt(cli); err != nil { + return nil, fmt.Errorf("error applying Live Share connect option: %w", err) + } + } + + span, ctx := opentracing.StartSpanFromContext(ctx, "Connect") defer span.Finish() - clientSocket := newSocket(c.connection, c.tlsConfig) - if err := clientSocket.connect(ctx); err != nil { + sock := newSocket(cli.connection, cli.tlsConfig) + if err := sock.connect(ctx); err != nil { return nil, fmt.Errorf("error connecting websocket: %w", err) } - ssh := newSSHSession(c.connection.SessionToken, clientSocket) + ssh := newSSHSession(cli.connection.SessionToken, sock) if err := ssh.connect(ctx); err != nil { return nil, fmt.Errorf("error connecting to ssh session: %w", err) } rpc := newRPCClient(ssh) rpc.connect(ctx) - if _, err := c.joinWorkspace(ctx, rpc); err != nil { + + args := joinWorkspaceArgs{ + ID: cli.connection.SessionID, + ConnectionMode: "local", + JoiningUserSessionToken: cli.connection.SessionToken, + ClientCapabilities: clientCapabilities{ + IsNonInteractive: false, + }, + } + var result joinWorkspaceResult + if err := rpc.do(ctx, "workspace.joinWorkspace", &args, &result); err != nil { return nil, fmt.Errorf("error joining Live Share workspace: %w", err) } @@ -96,24 +119,6 @@ type channelID struct { name, condition string } -func (c *Client) joinWorkspace(ctx context.Context, rpc *rpcClient) (*joinWorkspaceResult, error) { - args := joinWorkspaceArgs{ - ID: c.connection.SessionID, - ConnectionMode: "local", - JoiningUserSessionToken: c.connection.SessionToken, - ClientCapabilities: clientCapabilities{ - IsNonInteractive: false, - }, - } - - var result joinWorkspaceResult - if err := rpc.do(ctx, "workspace.joinWorkspace", &args, &result); err != nil { - return nil, fmt.Errorf("error making workspace.joinWorkspace call: %w", err) - } - - return &result, nil -} - func (s *Session) openStreamingChannel(ctx context.Context, id channelID) (ssh.Channel, error) { type getStreamArgs struct { StreamName string `json:"streamName"` diff --git a/client_test.go b/client_test.go index c1e61f6e8..369c53b28 100644 --- a/client_test.go +++ b/client_test.go @@ -13,37 +13,7 @@ import ( "github.com/sourcegraph/jsonrpc2" ) -func TestNewClient(t *testing.T) { - client, err := NewClient() - if err != nil { - t.Errorf("error creating new client: %v", err) - } - if client == nil { - t.Error("client is nil") - } -} - -func TestNewClientValidConnection(t *testing.T) { - connection := Connection{"1", "2", "3", "4"} - - client, err := NewClient(WithConnection(connection)) - if err != nil { - t.Errorf("error creating new client: %v", err) - } - if client == nil { - t.Error("client is nil") - } -} - -func TestNewClientWithInvalidConnection(t *testing.T) { - connection := Connection{} - - if _, err := NewClient(WithConnection(connection)); err == nil { - t.Error("err is nil") - } -} - -func TestJoinSession(t *testing.T) { +func TestConnect(t *testing.T) { connection := Connection{ SessionID: "session-id", SessionToken: "session-token", @@ -83,21 +53,11 @@ func TestJoinSession(t *testing.T) { ctx := context.Background() tlsConfig := WithTLSConfig(&tls.Config{InsecureSkipVerify: true}) - client, err := NewClient(WithConnection(connection), tlsConfig) - if err != nil { - t.Errorf("error creating new client: %v", err) - } done := make(chan error) go func() { - session, err := client.JoinWorkspace(ctx) - if err != nil { - done <- fmt.Errorf("error joining workspace: %v", err) - return - } - _ = session - - done <- nil + _, err := Connect(ctx, WithConnection(connection), tlsConfig) // ignore session + done <- err }() select { diff --git a/connection.go b/connection.go index c1a4632c8..f402e4bb9 100644 --- a/connection.go +++ b/connection.go @@ -6,7 +6,7 @@ import ( "strings" ) -// A Connection represents a set of values necessary to join a liveshare connection +// A Connection represents a set of values necessary to join a liveshare connection. type Connection struct { SessionID string SessionToken string diff --git a/session_test.go b/session_test.go index 54aab16c8..3be90cb0e 100644 --- a/session_test.go +++ b/session_test.go @@ -32,14 +32,9 @@ func makeMockSession(opts ...livesharetest.ServerOption) (*livesharetest.Server, ) connection.RelayEndpoint = "sb" + strings.TrimPrefix(testServer.URL(), "https") tlsConfig := WithTLSConfig(&tls.Config{InsecureSkipVerify: true}) - client, err := NewClient(WithConnection(connection), tlsConfig) + session, err := Connect(context.Background(), WithConnection(connection), tlsConfig) if err != nil { - return nil, nil, fmt.Errorf("error creating new client: %v", err) - } - ctx := context.Background() - session, err := client.JoinWorkspace(ctx) - if err != nil { - return nil, nil, fmt.Errorf("error joining workspace: %v", err) + return nil, nil, fmt.Errorf("error connecting to Live Share: %v", err) } return testServer, session, nil }