From 20d75f0ff9198f952311a8aa21bf648df6147767 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 12 Aug 2021 14:37:23 +0200 Subject: [PATCH] Normalize logging, output, and error reporting - Return errors as errors, not print to stdout and return nil - Ensure errors and warnings are always written to stderr, not stout - Do not print progress to stdout unless stdout is a terminal --- cmd/ghcs/code.go | 3 +-- cmd/ghcs/create.go | 12 +++++++----- cmd/ghcs/delete.go | 12 ++++++++---- cmd/ghcs/list.go | 5 ----- cmd/ghcs/logs.go | 4 +++- cmd/ghcs/main.go | 2 +- cmd/ghcs/ports.go | 30 ++++++++++++++++-------------- cmd/ghcs/ssh.go | 16 +++++----------- internal/codespaces/codespaces.go | 19 +++++++++++-------- 9 files changed, 52 insertions(+), 51 deletions(-) diff --git a/cmd/ghcs/code.go b/cmd/ghcs/code.go index ccb4788ee..4880436ed 100644 --- a/cmd/ghcs/code.go +++ b/cmd/ghcs/code.go @@ -43,8 +43,7 @@ func Code(codespaceName string) error { codespace, err := codespaces.ChooseCodespace(ctx, apiClient, user) if err != nil { if err == codespaces.ErrNoCodespaces { - fmt.Println(err.Error()) - return nil + return err } return fmt.Errorf("error choosing codespace: %v", err) } diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index 385e5d957..b4a4fcb1f 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "os" "strings" @@ -9,6 +10,7 @@ import ( "github.com/AlecAivazis/survey/v2" "github.com/fatih/camelcase" "github.com/github/ghcs/api" + "github.com/github/ghcs/cmd/ghcs/output" "github.com/spf13/cobra" ) @@ -17,7 +19,7 @@ var repo, branch, machine string func newCreateCmd() *cobra.Command { createCmd := &cobra.Command{ Use: "create", - Short: "Create a GitHub Codespace.", + Short: "Create a GitHub Codespace.", RunE: func(cmd *cobra.Command, args []string) error { return Create() }, @@ -39,6 +41,7 @@ func Create() error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) locationCh := getLocation(ctx, apiClient) userCh := getUser(ctx, apiClient) + log := output.NewLogger(os.Stdout, os.Stderr, false) repo, err := getRepoName() if err != nil { @@ -69,18 +72,17 @@ func Create() error { return fmt.Errorf("error getting machine type: %v", err) } if machine == "" { - fmt.Println("There are no available machine types for this repository") - return nil + return errors.New("There are no available machine types for this repository") } - fmt.Println("Creating your codespace...") + log.Println("Creating your codespace...") codespace, err := apiClient.CreateCodespace(ctx, userResult.User, repository, machine, branch, locationResult.Location) if err != nil { return fmt.Errorf("error creating codespace: %v", err) } - fmt.Println("Codespace created: " + codespace.Name) + log.Printf("Codespace created: %s\n", codespace.Name) return nil } diff --git a/cmd/ghcs/delete.go b/cmd/ghcs/delete.go index df7a3f1be..d625a1ca3 100644 --- a/cmd/ghcs/delete.go +++ b/cmd/ghcs/delete.go @@ -7,6 +7,7 @@ import ( "os" "github.com/github/ghcs/api" + "github.com/github/ghcs/cmd/ghcs/output" "github.com/github/ghcs/internal/codespaces" "github.com/spf13/cobra" ) @@ -58,6 +59,7 @@ func init() { func Delete(codespaceName string) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() + log := output.NewLogger(os.Stdout, os.Stderr, false) user, err := apiClient.GetUser(ctx) if err != nil { @@ -73,7 +75,7 @@ func Delete(codespaceName string) error { return fmt.Errorf("error deleting codespace: %v", err) } - fmt.Println("Codespace deleted.") + log.Println("Codespace deleted.") return List(&ListOptions{}) } @@ -81,6 +83,7 @@ func Delete(codespaceName string) error { func DeleteAll() error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() + log := output.NewLogger(os.Stdout, os.Stderr, false) user, err := apiClient.GetUser(ctx) if err != nil { @@ -102,7 +105,7 @@ func DeleteAll() error { return fmt.Errorf("error deleting codespace: %v", err) } - fmt.Printf("Codespace deleted: %s\n", c.Name) + log.Printf("Codespace deleted: %s\n", c.Name) } return List(&ListOptions{}) @@ -111,6 +114,7 @@ func DeleteAll() error { func DeleteByRepo(repo string) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() + log := output.NewLogger(os.Stdout, os.Stderr, false) user, err := apiClient.GetUser(ctx) if err != nil { @@ -138,11 +142,11 @@ func DeleteByRepo(repo string) error { return fmt.Errorf("error deleting codespace: %v", err) } - fmt.Printf("Codespace deleted: %s\n", c.Name) + log.Printf("Codespace deleted: %s\n", c.Name) } if !deleted { - fmt.Printf("No codespace was found for repository: %s\n", repo) + return fmt.Errorf("No codespace was found for repository: %s", repo) } return List(&ListOptions{}) diff --git a/cmd/ghcs/list.go b/cmd/ghcs/list.go index bbee2aae8..acc01b7ad 100644 --- a/cmd/ghcs/list.go +++ b/cmd/ghcs/list.go @@ -48,11 +48,6 @@ func List(opts *ListOptions) error { return fmt.Errorf("error getting codespaces: %v", err) } - if len(codespaces) == 0 { - fmt.Println("You have no codespaces.") - return nil - } - table := output.NewTable(os.Stdout, opts.AsJSON) table.SetHeader([]string{"Name", "Repository", "Branch", "State", "Created At"}) for _, codespace := range codespaces { diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index 03a7c963a..45fd8bca8 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -7,6 +7,7 @@ import ( "os" "github.com/github/ghcs/api" + "github.com/github/ghcs/cmd/ghcs/output" "github.com/github/ghcs/internal/codespaces" "github.com/spf13/cobra" ) @@ -38,6 +39,7 @@ func init() { func Logs(tail bool, codespaceName string) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() + log := output.NewLogger(os.Stdout, os.Stderr, false) user, err := apiClient.GetUser(ctx) if err != nil { @@ -49,7 +51,7 @@ func Logs(tail bool, codespaceName string) error { return fmt.Errorf("get or choose codespace: %v", err) } - lsclient, err := codespaces.ConnectToLiveshare(ctx, apiClient, token, codespace) + lsclient, err := codespaces.ConnectToLiveshare(ctx, log, apiClient, token, codespace) if err != nil { return fmt.Errorf("connecting to liveshare: %v", err) } diff --git a/cmd/ghcs/main.go b/cmd/ghcs/main.go index 00e8be894..a2617788a 100644 --- a/cmd/ghcs/main.go +++ b/cmd/ghcs/main.go @@ -22,7 +22,7 @@ var rootCmd = &cobra.Command{ func Execute() { if os.Getenv("GITHUB_TOKEN") == "" { - fmt.Println("The GITHUB_TOKEN environment variable is required. Create a Personal Access Token at https://github.com/settings/tokens/new?scopes=repo and make sure to enable SSO for the GitHub organization after creating the token.") + fmt.Fprintln(os.Stderr, "The GITHUB_TOKEN environment variable is required. Create a Personal Access Token at https://github.com/settings/tokens/new?scopes=repo and make sure to enable SSO for the GitHub organization after creating the token.") os.Exit(1) } diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index 27b4e0614..613a1834b 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -61,15 +61,14 @@ func Ports(opts *PortsOptions) error { codespace, token, err := codespaces.GetOrChooseCodespace(ctx, apiClient, user, opts.CodespaceName) if err != nil { if err == codespaces.ErrNoCodespaces { - fmt.Println(err.Error()) - return nil + return err } return fmt.Errorf("error choosing codespace: %v", err) } devContainerCh := getDevContainer(ctx, apiClient, codespace) - liveShareClient, err := codespaces.ConnectToLiveshare(ctx, apiClient, token, codespace) + liveShareClient, err := codespaces.ConnectToLiveshare(ctx, log, apiClient, token, codespace) if err != nil { return fmt.Errorf("error connecting to liveshare: %v", err) } @@ -177,7 +176,8 @@ func NewPortsPublicCmd() *cobra.Command { return errors.New("[codespace_name] [source] port number are required.") } - return updatePortVisibility(args[0], args[1], true) + log := output.NewLogger(os.Stdout, os.Stderr, false) + return updatePortVisibility(log, args[0], args[1], true) }, } } @@ -192,12 +192,13 @@ func NewPortsPrivateCmd() *cobra.Command { return errors.New("[codespace_name] [source] port number are required.") } - return updatePortVisibility(args[0], args[1], false) + log := output.NewLogger(os.Stdout, os.Stderr, false) + return updatePortVisibility(log, args[0], args[1], false) }, } } -func updatePortVisibility(codespaceName, sourcePort string, public bool) error { +func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, public bool) error { ctx := context.Background() apiClient := api.New(os.Getenv("GITHUB_TOKEN")) @@ -216,7 +217,7 @@ func updatePortVisibility(codespaceName, sourcePort string, public bool) error { return fmt.Errorf("error getting codespace: %v", err) } - lsclient, err := codespaces.ConnectToLiveshare(ctx, apiClient, token, codespace) + lsclient, err := codespaces.ConnectToLiveshare(ctx, log, apiClient, token, codespace) if err != nil { return fmt.Errorf("error connecting to liveshare: %v", err) } @@ -236,11 +237,10 @@ func updatePortVisibility(codespaceName, sourcePort string, public bool) error { } state := "PUBLIC" - if public == false { + if !public { state = "PRIVATE" } - - fmt.Println(fmt.Sprintf("Port %s is now %s.", sourcePort, state)) + log.Printf("Port %s is now %s.\n", sourcePort, state) return nil } @@ -254,12 +254,14 @@ func NewPortsForwardCmd() *cobra.Command { if len(args) < 3 { return errors.New("[codespace_name] [source] [dst] port number are required.") } - return forwardPort(args[0], args[1], args[2]) + + log := output.NewLogger(os.Stdout, os.Stderr, false) + return forwardPort(log, args[0], args[1], args[2]) }, } } -func forwardPort(codespaceName, sourcePort, destPort string) error { +func forwardPort(log *output.Logger, codespaceName, sourcePort, destPort string) error { ctx := context.Background() apiClient := api.New(os.Getenv("GITHUB_TOKEN")) @@ -278,7 +280,7 @@ func forwardPort(codespaceName, sourcePort, destPort string) error { return fmt.Errorf("error getting codespace: %v", err) } - lsclient, err := codespaces.ConnectToLiveshare(ctx, apiClient, token, codespace) + lsclient, err := codespaces.ConnectToLiveshare(ctx, log, apiClient, token, codespace) if err != nil { return fmt.Errorf("error connecting to liveshare: %v", err) } @@ -302,7 +304,7 @@ func forwardPort(codespaceName, sourcePort, destPort string) error { return fmt.Errorf("error sharing source port: %v", err) } - fmt.Println("Forwarding port: " + sourcePort + " -> " + destPort) + log.Println("Forwarding port: " + sourcePort + " -> " + destPort) portForwarder := liveshare.NewPortForwarder(lsclient, server, dstPortInt) if err := portForwarder.Start(ctx); err != nil { return fmt.Errorf("error forwarding port: %v", err) diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 60fdee498..372061c55 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -9,6 +9,7 @@ import ( "time" "github.com/github/ghcs/api" + "github.com/github/ghcs/cmd/ghcs/output" "github.com/github/ghcs/internal/codespaces" "github.com/github/go-liveshare" "github.com/spf13/cobra" @@ -40,6 +41,7 @@ func init() { func SSH(sshProfile, codespaceName string, sshServerPort int) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() + log := output.NewLogger(os.Stdout, os.Stderr, false) user, err := apiClient.GetUser(ctx) if err != nil { @@ -51,7 +53,7 @@ func SSH(sshProfile, codespaceName string, sshServerPort int) error { return fmt.Errorf("get or choose codespace: %v", err) } - lsclient, err := codespaces.ConnectToLiveshare(ctx, apiClient, token, codespace) + lsclient, err := codespaces.ConnectToLiveshare(ctx, log, apiClient, token, codespace) if err != nil { return fmt.Errorf("error connecting to liveshare: %v", err) } @@ -61,7 +63,7 @@ func SSH(sshProfile, codespaceName string, sshServerPort int) error { return fmt.Errorf("error creating liveshare terminal: %v", err) } - fmt.Println("Preparing SSH...") + log.Println("Preparing SSH...") if sshProfile == "" { containerID, err := getContainerID(ctx, terminal) if err != nil { @@ -71,8 +73,6 @@ func SSH(sshProfile, codespaceName string, sshServerPort int) error { if err := setupSSH(ctx, terminal, containerID, codespace.RepositoryName); err != nil { return fmt.Errorf("error creating ssh server: %v", err) } - - fmt.Printf("\n") } tunnelPort, tunnelClosed, err := codespaces.MakeSSHTunnel(ctx, lsclient, sshServerPort) @@ -88,7 +88,7 @@ func SSH(sshProfile, codespaceName string, sshServerPort int) error { usingCustomPort := tunnelPort == sshServerPort connClosed := codespaces.ConnectToTunnel(ctx, tunnelPort, connectDestination, usingCustomPort) - fmt.Println("Ready...") + log.Println("Ready...") select { case err := <-tunnelClosed: if err != nil { @@ -104,7 +104,6 @@ func SSH(sshProfile, codespaceName string, sshServerPort int) error { } func getContainerID(ctx context.Context, terminal *liveshare.Terminal) (string, error) { - fmt.Print(".") cmd := terminal.NewCommand( "/", "/usr/bin/docker ps -aq --filter label=Type=codespaces --filter status=running", @@ -114,17 +113,14 @@ func getContainerID(ctx context.Context, terminal *liveshare.Terminal) (string, return "", fmt.Errorf("error running command: %v", err) } - fmt.Print(".") scanner := bufio.NewScanner(stream) scanner.Scan() - fmt.Print(".") containerID := scanner.Text() if err := scanner.Err(); err != nil { return "", fmt.Errorf("error scanning stream: %v", err) } - fmt.Print(".") if err := stream.Close(); err != nil { return "", fmt.Errorf("error closing stream: %v", err) } @@ -135,7 +131,6 @@ func getContainerID(ctx context.Context, terminal *liveshare.Terminal) (string, func setupSSH(ctx context.Context, terminal *liveshare.Terminal, containerID, repositoryName string) error { setupBashProfileCmd := fmt.Sprintf(`echo "cd /workspaces/%v; export $(cat /workspaces/.codespaces/shared/.env | xargs); exec /bin/zsh;" > /home/codespace/.bash_profile`, repositoryName) - fmt.Print(".") compositeCommand := []string{setupBashProfileCmd} cmd := terminal.NewCommand( "/", @@ -146,7 +141,6 @@ func setupSSH(ctx context.Context, terminal *liveshare.Terminal, containerID, re return fmt.Errorf("error running command: %v", err) } - fmt.Print(".") if err := stream.Close(); err != nil { return fmt.Errorf("error closing stream: %v", err) } diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 4c62d9aff..48369cfa0 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -57,9 +57,14 @@ func ChooseCodespace(ctx context.Context, apiClient *api.API, user *api.User) (* return codespace, nil } -func ConnectToLiveshare(ctx context.Context, apiClient *api.API, token string, codespace *api.Codespace) (client *liveshare.Client, err error) { +type logger interface { + Print(v ...interface{}) (int, error) + Println(v ...interface{}) (int, error) +} + +func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, token string, codespace *api.Codespace) (client *liveshare.Client, err error) { if codespace.Environment.State != api.CodespaceEnvironmentStateAvailable { - fmt.Println("Starting your codespace...") // TODO(josebalius): better way of notifying of events + log.Println("Starting your codespace...") if err := apiClient.StartCodespace(ctx, token, codespace); err != nil { return nil, fmt.Errorf("error starting codespace: %v", err) } @@ -69,7 +74,7 @@ func ConnectToLiveshare(ctx context.Context, apiClient *api.API, token string, c for codespace.Environment.Connection.SessionID == "" || codespace.Environment.State != api.CodespaceEnvironmentStateAvailable { if retries > 1 { if retries%2 == 0 { - fmt.Print(".") + log.Print(".") } time.Sleep(1 * time.Second) @@ -88,10 +93,10 @@ func ConnectToLiveshare(ctx context.Context, apiClient *api.API, token string, c } if retries >= 2 { - fmt.Print("\n") + log.Print("\n") } - fmt.Println("Connecting to your codespace...") + log.Println("Connecting to your codespace...") lsclient, err := liveshare.NewClient( liveshare.WithConnection(liveshare.Connection{ @@ -117,10 +122,8 @@ func GetOrChooseCodespace(ctx context.Context, apiClient *api.API, user *api.Use codespace, err = ChooseCodespace(ctx, apiClient, user) if err != nil { if err == ErrNoCodespaces { - fmt.Println(err.Error()) - return nil, "", nil + return nil, "", err } - return nil, "", fmt.Errorf("choosing codespace: %v", err) } codespaceName = codespace.Name