Merge branch 'main' of github.com:github/ghcs into jg/ssh-cmd-flags

josebalius · josebalius · commit 514448dde8f2 · 2021-09-15T15:45:53.000-04:00
diff --git a/cmd/ghcs/code.go b/cmd/ghcs/code.go
@@ -6,7 +6,7 @@ import (
 	"net/url"
 	"os"
 
-	"github.com/github/ghcs/api"
+	"github.com/github/ghcs/internal/api"
 	"github.com/skratchdot/open-golang/open"
 	"github.com/spf13/cobra"
 )
diff --git a/cmd/ghcs/common.go b/cmd/ghcs/common.go
@@ -11,7 +11,7 @@ import (
 
 	"github.com/AlecAivazis/survey/v2"
 	"github.com/AlecAivazis/survey/v2/terminal"
-	"github.com/github/ghcs/api"
+	"github.com/github/ghcs/internal/api"
 	"golang.org/x/term"
 )
 
diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go
@@ -9,8 +9,8 @@ import (
 
 	"github.com/AlecAivazis/survey/v2"
 	"github.com/fatih/camelcase"
-	"github.com/github/ghcs/api"
 	"github.com/github/ghcs/cmd/ghcs/output"
+	"github.com/github/ghcs/internal/api"
 	"github.com/github/ghcs/internal/codespaces"
 	"github.com/spf13/cobra"
 )
@@ -105,12 +105,16 @@ func create(opts *createOptions) error {
 	return nil
 }
 
+// showStatus polls the codespace for a list of post create states and their status. It will keep polling
+// until all states have finished. Once all states have finished, we poll once more to check if any new
+// states have been introduced and stop polling otherwise.
 func showStatus(ctx context.Context, log *output.Logger, apiClient *api.API, user *api.User, codespace *api.Codespace) error {
 	var lastState codespaces.PostCreateState
 	var breakNextState bool
 
 	finishedStates := make(map[string]bool)
 	ctx, stopPolling := context.WithCancel(ctx)
+	defer stopPolling()
 
 	poller := func(states []codespaces.PostCreateState) {
 		var inProgress bool
@@ -153,7 +157,12 @@ func showStatus(ctx context.Context, log *output.Logger, apiClient *api.API, use
 		}
 	}
 
-	if err := codespaces.PollPostCreateStates(ctx, log, apiClient, user, codespace, poller); err != nil {
+	err := codespaces.PollPostCreateStates(ctx, log, apiClient, user, codespace, poller)
+	if err != nil {
+		if errors.Is(err, context.Canceled) && breakNextState {
+			return nil // we cancelled the context to stop polling, we can ignore the error
+		}
+
 		return fmt.Errorf("failed to poll state changes from codespace: %v", err)
 	}
 
diff --git a/cmd/ghcs/delete.go b/cmd/ghcs/delete.go
@@ -7,8 +7,8 @@ import (
 	"os"
 	"strings"
 
-	"github.com/github/ghcs/api"
 	"github.com/github/ghcs/cmd/ghcs/output"
+	"github.com/github/ghcs/internal/api"
 	"github.com/spf13/cobra"
 )
 
diff --git a/cmd/ghcs/list.go b/cmd/ghcs/list.go
@@ -5,8 +5,8 @@ import (
 	"fmt"
 	"os"
 
-	"github.com/github/ghcs/api"
 	"github.com/github/ghcs/cmd/ghcs/output"
+	"github.com/github/ghcs/internal/api"
 	"github.com/spf13/cobra"
 )
 
diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go
@@ -6,8 +6,8 @@ import (
 	"net"
 	"os"
 
-	"github.com/github/ghcs/api"
 	"github.com/github/ghcs/cmd/ghcs/output"
+	"github.com/github/ghcs/internal/api"
 	"github.com/github/ghcs/internal/codespaces"
 	"github.com/github/go-liveshare"
 	"github.com/spf13/cobra"
@@ -70,7 +70,8 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow
 	defer listen.Close()
 	localPort := listen.Addr().(*net.TCPAddr).Port
 
-	remoteSSHServerPort, sshUser, err := codespaces.StartSSHServer(ctx, session, log)
+	log.Println("Fetching SSH Details...")
+	remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx)
 	if err != nil {
 		return fmt.Errorf("error getting ssh server details: %v", err)
 	}
diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go
@@ -11,8 +11,8 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/github/ghcs/api"
 	"github.com/github/ghcs/cmd/ghcs/output"
+	"github.com/github/ghcs/internal/api"
 	"github.com/github/ghcs/internal/codespaces"
 	"github.com/github/go-liveshare"
 	"github.com/muhammadmuzzammil1998/jsonc"
diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go
@@ -6,8 +6,8 @@ import (
 	"net"
 	"os"
 
-	"github.com/github/ghcs/api"
 	"github.com/github/ghcs/cmd/ghcs/output"
+	"github.com/github/ghcs/internal/api"
 	"github.com/github/ghcs/internal/codespaces"
 	"github.com/github/go-liveshare"
 	"github.com/spf13/cobra"
@@ -59,7 +59,8 @@ func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string
 		return fmt.Errorf("error connecting to Live Share: %v", err)
 	}
 
-	remoteSSHServerPort, sshUser, err := codespaces.StartSSHServer(ctx, session, log)
+	log.Println("Fetching SSH Details...")
+	remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx)
 	if err != nil {
 		return fmt.Errorf("error getting ssh server details: %v", err)
 	}
diff --git a/internal/api/api.go b/internal/api/api.go
@@ -1,4 +1,3 @@
-// TODO(adonovan): rename to package codespaces, and codespaces.Client.
 package api
 
 // For descriptions of service interfaces, see:
@@ -7,6 +6,24 @@ package api
 // - https://github.com/github/github/blob/master/app/api/codespaces.rb (for vscs_internal)
 // TODO(adonovan): replace the last link with a public doc URL when available.
 
+// TODO(adonovan): a possible reorganization would be to split this
+// file into three internal packages, one per backend service, and to
+// rename api.API to github.Client:
+//
+// - github.GetUser(github.Client)
+// - github.GetRepository(Client)
+// - github.ReadFile(Client, nwo, branch, path) // was GetCodespaceRepositoryContents
+// - codespaces.Create(Client, user, repo, sku, branch, location)
+// - codespaces.Delete(Client, user, token, name)
+// - codespaces.Get(Client, token, owner, name)
+// - codespaces.GetMachineTypes(Client, user, repo, branch, location)
+// - codespaces.GetToken(Client, login, name)
+// - codespaces.List(Client, user)
+// - codespaces.Start(Client, token, codespace)
+// - visualstudio.GetRegionLocation(http.Client) // no dependency on github
+//
+// This would make the meaning of each operation clearer.
+
 import (
 	"bytes"
 	"context"
diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go
@@ -6,7 +6,7 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/github/ghcs/api"
+	"github.com/github/ghcs/internal/api"
 	"github.com/github/go-liveshare"
 )
 
diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go
@@ -2,40 +2,12 @@ package codespaces
 
 import (
 	"context"
-	"errors"
-	"fmt"
 	"os"
 	"os/exec"
 	"strconv"
 	"strings"
-
-	"github.com/github/go-liveshare"
 )
 
-// StartSSHServer installs (if necessary) and starts the SSH in the codespace.
-// It returns the remote port where it is running, the user to log in with, or an error if something failed.
-func StartSSHServer(ctx context.Context, session *liveshare.Session, log logger) (serverPort int, user string, err error) {
-	log.Println("Fetching SSH details...")
-
-	sshServer := session.SSHServer()
-
-	sshServerStartResult, err := sshServer.StartRemoteServer(ctx)
-	if err != nil {
-		return 0, "", fmt.Errorf("error starting live share: %v", err)
-	}
-
-	if !sshServerStartResult.Result {
-		return 0, "", errors.New(sshServerStartResult.Message)
-	}
-
-	portInt, err := strconv.Atoi(sshServerStartResult.ServerPort)
-	if err != nil {
-		return 0, "", fmt.Errorf("error parsing port: %v", err)
-	}
-
-	return portInt, sshServerStartResult.User, nil
-}
-
 // Shell runs an interactive secure shell over an existing
 // port-forwarding session. It runs until the shell is terminated
 // (including by cancellation of the context).
@@ -60,9 +32,14 @@ func NewRemoteCommand(ctx context.Context, tunnelPort int, destination, command
 // an interactive shell) over ssh.
 func newSSHCommand(ctx context.Context, port int, dst, command string) (*exec.Cmd, []string) {
 	connArgs := []string{"-p", strconv.Itoa(port), "-o", "NoHostAuthenticationForLocalhost=yes"}
-	// TODO(adonovan): eliminate X11 and X11Trust flags where unneeded.
-	cmdArgs := append([]string{dst, "-X", "-Y", "-C"}, connArgs...) // X11, X11Trust, Compression
 
+	cmdArgs := []string{dst, "-C"} // Always use Compression
+	if command == "" {
+		// if we are in a shell send X11 and X11Trust
+		cmdArgs = append(cmdArgs, "-X", "-Y")
+	}
+
+	cmdArgs = append(cmdArgs, connArgs...)
 	if command != "" {
 		cmdArgs = append(cmdArgs, command)
 	}
diff --git a/internal/codespaces/states.go b/internal/codespaces/states.go
@@ -9,7 +9,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/github/ghcs/api"
+	"github.com/github/ghcs/internal/api"
 	"github.com/github/go-liveshare"
 )
 
@@ -39,12 +39,12 @@ type PostCreateState struct {
 func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, user *api.User, codespace *api.Codespace, poller func([]PostCreateState)) error {
 	token, err := apiClient.GetCodespaceToken(ctx, user.Login, codespace.Name)
 	if err != nil {
-		return fmt.Errorf("getting codespace token: %v", err)
+		return fmt.Errorf("getting codespace token: %w", err)
 	}
 
 	session, err := ConnectToLiveshare(ctx, log, apiClient, user.Login, token, codespace)
 	if err != nil {
-		return fmt.Errorf("connect to Live Share: %v", err)
+		return fmt.Errorf("connect to Live Share: %w", err)
 	}
 
 	// Ensure local port is listening before client (getPostCreateOutput) connects.
@@ -54,9 +54,10 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, u
 	}
 	localPort := listen.Addr().(*net.TCPAddr).Port
 
-	remoteSSHServerPort, sshUser, err := StartSSHServer(ctx, session, log)
+	log.Println("Fetching SSH Details...")
+	remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx)
 	if err != nil {
-		return fmt.Errorf("error getting ssh server details: %v", err)
+		return fmt.Errorf("error getting ssh server details: %w", err)
 	}
 
 	tunnelClosed := make(chan error, 1) // buffered to avoid sender stuckness
@@ -74,12 +75,12 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, u
 			return ctx.Err()
 
 		case err := <-tunnelClosed:
-			return fmt.Errorf("connection failed: %v", err)
+			return fmt.Errorf("connection failed: %w", err)
 
 		case <-t.C:
 			states, err := getPostCreateOutput(ctx, localPort, codespace, sshUser)
 			if err != nil {
-				return fmt.Errorf("get post create output: %v", err)
+				return fmt.Errorf("get post create output: %w", err)
 			}
 
 			poller(states)
@@ -95,13 +96,13 @@ func getPostCreateOutput(ctx context.Context, tunnelPort int, codespace *api.Cod
 	stdout := new(bytes.Buffer)
 	cmd.Stdout = stdout
 	if err := cmd.Run(); err != nil {
-		return nil, fmt.Errorf("run command: %v", err)
+		return nil, fmt.Errorf("run command: %w", err)
 	}
 	var output struct {
 		Steps []PostCreateState `json:"steps"`
 	}
 	if err := json.Unmarshal(stdout.Bytes(), &output); err != nil {
-		return nil, fmt.Errorf("unmarshal output: %v", err)
+		return nil, fmt.Errorf("unmarshal output: %w", err)
 	}
 
 	return output.Steps, nil

Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@ import (`
`6`	`6`	`"net/url"`
`7`	`7`	`"os"`
`8`	`8`
`9`		`- "github.com/github/ghcs/api"`
	`9`	`+ "github.com/github/ghcs/internal/api"`
`10`	`10`	`"github.com/skratchdot/open-golang/open"`
`11`	`11`	`"github.com/spf13/cobra"`
`12`	`12`	`)`
Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ import (`
`11`	`11`
`12`	`12`	`"github.com/AlecAivazis/survey/v2"`
`13`	`13`	`"github.com/AlecAivazis/survey/v2/terminal"`
`14`		`- "github.com/github/ghcs/api"`
	`14`	`+ "github.com/github/ghcs/internal/api"`
`15`	`15`	`"golang.org/x/term"`
`16`	`16`	`)`
`17`	`17`
Original file line number	Diff line number	Diff line change
`@@ -7,8 +7,8 @@ import (`
`7`	`7`	`"os"`
`8`	`8`	`"strings"`
`9`	`9`
`10`		`- "github.com/github/ghcs/api"`
`11`	`10`	`"github.com/github/ghcs/cmd/ghcs/output"`
	`11`	`+ "github.com/github/ghcs/internal/api"`
`12`	`12`	`"github.com/spf13/cobra"`
`13`	`13`	`)`
`14`	`14`
Original file line number	Diff line number	Diff line change
`@@ -5,8 +5,8 @@ import (`
`5`	`5`	`"fmt"`
`6`	`6`	`"os"`
`7`	`7`
`8`		`- "github.com/github/ghcs/api"`
`9`	`8`	`"github.com/github/ghcs/cmd/ghcs/output"`
	`9`	`+ "github.com/github/ghcs/internal/api"`
`10`	`10`	`"github.com/spf13/cobra"`
`11`	`11`	`)`
`12`	`12`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@ import (`
`6`	`6`	`"fmt"`
`7`	`7`	`"time"`
`8`	`8`
`9`		`- "github.com/github/ghcs/api"`
	`9`	`+ "github.com/github/ghcs/internal/api"`
`10`	`10`	`"github.com/github/go-liveshare"`
`11`	`11`	`)`
`12`	`12`