diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 3c7be9c01..c9475ad4d 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -12,10 +12,16 @@ import ( type logger interface { Print(v ...interface{}) (int, error) - Printf(f string, v ...interface{}) (int, error) Println(v ...interface{}) (int, error) } +// TODO(josebalius): clean this up once we standardrize +// logging for codespaces +type liveshareLogger interface { + Println(v ...interface{}) + Printf(f string, v ...interface{}) +} + func connectionReady(codespace *api.Codespace) bool { return codespace.Environment.Connection.SessionID != "" && codespace.Environment.Connection.SessionToken != "" && @@ -31,7 +37,7 @@ type apiClient interface { // ConnectToLiveshare waits for a Codespace to become running, // and connects to it using a Live Share session. -func ConnectToLiveshare(ctx context.Context, log, sessionLogger logger, apiClient apiClient, codespace *api.Codespace) (*liveshare.Session, error) { +func ConnectToLiveshare(ctx context.Context, log logger, sessionLogger liveshareLogger, apiClient apiClient, codespace *api.Codespace) (*liveshare.Session, error) { var startedCodespace bool if codespace.Environment.State != api.CodespaceEnvironmentStateAvailable { startedCodespace = true diff --git a/internal/codespaces/states.go b/internal/codespaces/states.go index 10d7dd4ca..00170d3ec 100644 --- a/internal/codespaces/states.go +++ b/internal/codespaces/states.go @@ -5,6 +5,8 @@ import ( "context" "encoding/json" "fmt" + "io/ioutil" + "log" "net" "strings" "time" @@ -36,8 +38,10 @@ type PostCreateState struct { // PollPostCreateStates watches for state changes in a codespace, // and calls the supplied poller for each batch of state changes. // It runs until it encounters an error, including cancellation of the context. -func PollPostCreateStates(ctx context.Context, log logger, apiClient apiClient, codespace *api.Codespace, poller func([]PostCreateState)) (err error) { - session, err := ConnectToLiveshare(ctx, log, nil, apiClient, codespace) +func PollPostCreateStates(ctx context.Context, logger logger, apiClient apiClient, codespace *api.Codespace, poller func([]PostCreateState)) (err error) { + noopLogger := log.New(ioutil.Discard, "", 0) + + session, err := ConnectToLiveshare(ctx, logger, noopLogger, apiClient, codespace) if err != nil { return fmt.Errorf("connect to Live Share: %w", err) } @@ -54,7 +58,7 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient apiClient, } localPort := listen.Addr().(*net.TCPAddr).Port - log.Println("Fetching SSH Details...") + logger.Println("Fetching SSH Details...") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) if err != nil { return fmt.Errorf("error getting ssh server details: %w", err) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index d304f8d0d..d49995344 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -7,6 +7,8 @@ import ( "errors" "fmt" "io" + "io/ioutil" + "log" "os" "sort" "strings" @@ -211,6 +213,10 @@ func noArgsConstraint(cmd *cobra.Command, args []string) error { return nil } +func noopLogger() *log.Logger { + return log.New(ioutil.Discard, "", 0) +} + type codespace struct { *api.Codespace } diff --git a/pkg/cmd/codespace/logs.go b/pkg/cmd/codespace/logs.go index 3a7c6948f..317059918 100644 --- a/pkg/cmd/codespace/logs.go +++ b/pkg/cmd/codespace/logs.go @@ -51,7 +51,7 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err return fmt.Errorf("get or choose codespace: %w", err) } - session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace) + session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace) if err != nil { return fmt.Errorf("connecting to Live Share: %w", err) } diff --git a/pkg/cmd/codespace/ports.go b/pkg/cmd/codespace/ports.go index 5b4b7fadd..898ede494 100644 --- a/pkg/cmd/codespace/ports.go +++ b/pkg/cmd/codespace/ports.go @@ -60,7 +60,7 @@ func (a *App) ListPorts(ctx context.Context, codespaceName string, asJSON bool) devContainerCh := getDevContainer(ctx, a.apiClient, codespace) - session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace) + session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace) if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } @@ -194,7 +194,7 @@ func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName, sourcePor return fmt.Errorf("error getting codespace: %w", err) } - session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace) + session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace) if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } @@ -253,7 +253,7 @@ func (a *App) ForwardPorts(ctx context.Context, codespaceName string, ports []st return fmt.Errorf("error getting codespace: %w", err) } - session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace) + session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace) if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 1f183e9d1..928bd044e 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -4,11 +4,11 @@ import ( "context" "fmt" "io/ioutil" + "log" "net" "os" "github.com/cli/cli/v2/internal/codespaces" - "github.com/cli/cli/v2/pkg/cmd/codespace/output" "github.com/cli/cli/v2/pkg/liveshare" "github.com/spf13/cobra" ) @@ -18,6 +18,7 @@ type sshOptions struct { profile string serverPort int debug bool + debugFile string } func newSSHCmd(app *App) *cobra.Command { @@ -35,6 +36,7 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().IntVarP(&opts.serverPort, "server-port", "", 0, "SSH server port number (0 => pick unused)") sshCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") + sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") return sshCmd } @@ -62,7 +64,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e var debugLogger *fileLogger if opts.debug { - debugLogger, err = newFileLogger("gh-cs-ssh") + debugLogger, err = newFileLogger(opts.debugFile) if err != nil { return fmt.Errorf("error creating debug logger: %w", err) } @@ -125,26 +127,34 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -// fileLogger is a wrapper around an output.Logger configured to write +// fileLogger is a wrapper around an log.Logger configured to write // to a file. It exports two additional methods to get the log file name // and close the file handle when the operation is finished. type fileLogger struct { - // TODO(josebalius): should we use https://pkg.go.dev/log#New instead? - *output.Logger + *log.Logger f *os.File } // newFileLogger creates a new fileLogger. It returns an error if the file -// cannot be created. The file is created in the operating system tmp directory -// under the name parameter. -func newFileLogger(name string) (*fileLogger, error) { - f, err := ioutil.TempFile("", name) - if err != nil { - return nil, err +// cannot be created. The file is created on the specified path, if the path +// is empty it is created in the temporary directory. +func newFileLogger(file string) (fl *fileLogger, err error) { + var f *os.File + if file == "" { + f, err = ioutil.TempFile("", "") + if err != nil { + return nil, fmt.Errorf("failed to create tmp file: %w", err) + } + } else { + f, err = os.Create(file) + if err != nil { + return nil, err + } } + return &fileLogger{ - Logger: output.NewLogger(f, f, false), + Logger: log.New(f, "", log.LstdFlags), f: f, }, nil } diff --git a/pkg/liveshare/client.go b/pkg/liveshare/client.go index c3e92004d..840e99db9 100644 --- a/pkg/liveshare/client.go +++ b/pkg/liveshare/client.go @@ -24,18 +24,8 @@ import ( ) type logger interface { - Println(v ...interface{}) (int, error) - Printf(f string, v ...interface{}) (int, error) -} - -type noopLogger struct{} - -func (n noopLogger) Println(...interface{}) (int, error) { - return 0, nil -} - -func (n noopLogger) Printf(string, ...interface{}) (int, error) { - return 0, nil + Println(v ...interface{}) + Printf(f string, v ...interface{}) } // An Options specifies Live Share connection parameters. @@ -46,8 +36,8 @@ type Options struct { RelaySAS string RelayEndpoint string HostPublicKeys []string + Logger logger // required TLSConfig *tls.Config // (optional) - Logger logger // (optional) } // uri returns a websocket URL for the specified options. @@ -85,9 +75,8 @@ func Connect(ctx context.Context, opts Options) (*Session, error) { return nil, err } - var sessionLogger logger = noopLogger{} - if opts.Logger != nil { - sessionLogger = opts.Logger + if opts.Logger == nil { + return nil, errors.New("Logger is required") } sock := newSocket(uri, opts.TLSConfig) @@ -124,7 +113,7 @@ func Connect(ctx context.Context, opts Options) (*Session, error) { rpc: rpc, clientName: opts.ClientName, keepAliveReason: make(chan string, 1), - logger: sessionLogger, + logger: opts.Logger, } go s.heartbeat(ctx, 1*time.Minute) diff --git a/pkg/liveshare/client_test.go b/pkg/liveshare/client_test.go index a775ba4af..c6502d684 100644 --- a/pkg/liveshare/client_test.go +++ b/pkg/liveshare/client_test.go @@ -20,6 +20,7 @@ func TestConnect(t *testing.T) { SessionToken: "session-token", RelaySAS: "relay-sas", HostPublicKeys: []string{livesharetest.SSHPublicKey}, + Logger: newMockLogger(), } joinWorkspace := func(req *jsonrpc2.Request) (interface{}, error) { var joinWorkspaceReq joinWorkspaceArgs diff --git a/pkg/liveshare/session_test.go b/pkg/liveshare/session_test.go index 3be528fe8..998de6ac0 100644 --- a/pkg/liveshare/session_test.go +++ b/pkg/liveshare/session_test.go @@ -41,6 +41,7 @@ func makeMockSession(opts ...livesharetest.ServerOption) (*livesharetest.Server, RelaySAS: "relay-sas", HostPublicKeys: []string{livesharetest.SSHPublicKey}, TLSConfig: &tls.Config{InsecureSkipVerify: true}, + Logger: newMockLogger(), }) if err != nil { return nil, nil, fmt.Errorf("error connecting to Live Share: %w", err) @@ -383,16 +384,16 @@ func newMockLogger() *mockLogger { return &mockLogger{buf: new(bytes.Buffer)} } -func (m *mockLogger) Printf(format string, v ...interface{}) (int, error) { +func (m *mockLogger) Printf(format string, v ...interface{}) { m.Lock() defer m.Unlock() - return m.buf.WriteString(fmt.Sprintf(format, v...)) + m.buf.WriteString(fmt.Sprintf(format, v...)) } -func (m *mockLogger) Println(v ...interface{}) (int, error) { +func (m *mockLogger) Println(v ...interface{}) { m.Lock() defer m.Unlock() - return m.buf.WriteString(fmt.Sprintln(v...)) + m.buf.WriteString(fmt.Sprintln(v...)) } func (m *mockLogger) String() string {