X Tutup
Skip to content

Commit 5170a29

Browse files
josebaliusjosebalius
committed
Switch to standard lib log.Logger & support dfile
- --debug-file flag can now be used in conjuction with --debug to specify the debug file path - Push out logger concerns to callers of liveshare
1 parent 1aefc74 commit 5170a29

File tree

9 files changed

+59
-42
lines changed

9 files changed

+59
-42
lines changed

‎internal/codespaces/codespaces.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,16 @@ import (
1212

1313
type logger interface {
1414
Print(v ...interface{}) (int, error)
15-
Printf(f string, v ...interface{}) (int, error)
1615
Println(v ...interface{}) (int, error)
1716
}
1817

18+
// TODO(josebalius): clean this up once we standardrize
19+
// logging for codespaces
20+
type liveshareLogger interface {
21+
Println(v ...interface{})
22+
Printf(f string, v ...interface{})
23+
}
24+
1925
func connectionReady(codespace *api.Codespace) bool {
2026
return codespace.Environment.Connection.SessionID != "" &&
2127
codespace.Environment.Connection.SessionToken != "" &&
@@ -31,7 +37,7 @@ type apiClient interface {
3137

3238
// ConnectToLiveshare waits for a Codespace to become running,
3339
// and connects to it using a Live Share session.
34-
func ConnectToLiveshare(ctx context.Context, log, sessionLogger logger, apiClient apiClient, codespace *api.Codespace) (*liveshare.Session, error) {
40+
func ConnectToLiveshare(ctx context.Context, log logger, sessionLogger liveshareLogger, apiClient apiClient, codespace *api.Codespace) (*liveshare.Session, error) {
3541
var startedCodespace bool
3642
if codespace.Environment.State != api.CodespaceEnvironmentStateAvailable {
3743
startedCodespace = true

‎internal/codespaces/states.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import (
55
"context"
66
"encoding/json"
77
"fmt"
8+
"io/ioutil"
9+
"log"
810
"net"
911
"strings"
1012
"time"
@@ -36,8 +38,10 @@ type PostCreateState struct {
3638
// PollPostCreateStates watches for state changes in a codespace,
3739
// and calls the supplied poller for each batch of state changes.
3840
// It runs until it encounters an error, including cancellation of the context.
39-
func PollPostCreateStates(ctx context.Context, log logger, apiClient apiClient, codespace *api.Codespace, poller func([]PostCreateState)) (err error) {
40-
session, err := ConnectToLiveshare(ctx, log, nil, apiClient, codespace)
41+
func PollPostCreateStates(ctx context.Context, logger logger, apiClient apiClient, codespace *api.Codespace, poller func([]PostCreateState)) (err error) {
42+
noopLogger := log.New(ioutil.Discard, "", 0)
43+
44+
session, err := ConnectToLiveshare(ctx, logger, noopLogger, apiClient, codespace)
4145
if err != nil {
4246
return fmt.Errorf("connect to Live Share: %w", err)
4347
}
@@ -54,7 +58,7 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient apiClient,
5458
}
5559
localPort := listen.Addr().(*net.TCPAddr).Port
5660

57-
log.Println("Fetching SSH Details...")
61+
logger.Println("Fetching SSH Details...")
5862
remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx)
5963
if err != nil {
6064
return fmt.Errorf("error getting ssh server details: %w", err)

‎pkg/cmd/codespace/common.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"errors"
88
"fmt"
99
"io"
10+
"io/ioutil"
11+
"log"
1012
"os"
1113
"sort"
1214
"strings"
@@ -211,6 +213,10 @@ func noArgsConstraint(cmd *cobra.Command, args []string) error {
211213
return nil
212214
}
213215

216+
func noopLogger() *log.Logger {
217+
return log.New(ioutil.Discard, "", 0)
218+
}
219+
214220
type codespace struct {
215221
*api.Codespace
216222
}

‎pkg/cmd/codespace/logs.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err
5151
return fmt.Errorf("get or choose codespace: %w", err)
5252
}
5353

54-
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
54+
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
5555
if err != nil {
5656
return fmt.Errorf("connecting to Live Share: %w", err)
5757
}

‎pkg/cmd/codespace/ports.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func (a *App) ListPorts(ctx context.Context, codespaceName string, asJSON bool)
6060

6161
devContainerCh := getDevContainer(ctx, a.apiClient, codespace)
6262

63-
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
63+
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
6464
if err != nil {
6565
return fmt.Errorf("error connecting to Live Share: %w", err)
6666
}
@@ -194,7 +194,7 @@ func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName, sourcePor
194194
return fmt.Errorf("error getting codespace: %w", err)
195195
}
196196

197-
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
197+
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
198198
if err != nil {
199199
return fmt.Errorf("error connecting to Live Share: %w", err)
200200
}
@@ -253,7 +253,7 @@ func (a *App) ForwardPorts(ctx context.Context, codespaceName string, ports []st
253253
return fmt.Errorf("error getting codespace: %w", err)
254254
}
255255

256-
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
256+
session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
257257
if err != nil {
258258
return fmt.Errorf("error connecting to Live Share: %w", err)
259259
}

‎pkg/cmd/codespace/ssh.go‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@ import (
44
"context"
55
"fmt"
66
"io/ioutil"
7+
"log"
78
"net"
89
"os"
910

1011
"github.com/cli/cli/v2/internal/codespaces"
11-
"github.com/cli/cli/v2/pkg/cmd/codespace/output"
1212
"github.com/cli/cli/v2/pkg/liveshare"
1313
"github.com/spf13/cobra"
1414
)
@@ -18,6 +18,7 @@ type sshOptions struct {
1818
profile string
1919
serverPort int
2020
debug bool
21+
debugFile string
2122
}
2223

2324
func newSSHCmd(app *App) *cobra.Command {
@@ -35,6 +36,7 @@ func newSSHCmd(app *App) *cobra.Command {
3536
sshCmd.Flags().IntVarP(&opts.serverPort, "server-port", "", 0, "SSH server port number (0 => pick unused)")
3637
sshCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace")
3738
sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file")
39+
sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to")
3840

3941
return sshCmd
4042
}
@@ -62,7 +64,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e
6264

6365
var debugLogger *fileLogger
6466
if opts.debug {
65-
debugLogger, err = newFileLogger("gh-cs-ssh")
67+
debugLogger, err = newFileLogger(opts.debugFile)
6668
if err != nil {
6769
return fmt.Errorf("error creating debug logger: %w", err)
6870
}
@@ -125,26 +127,34 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e
125127
}
126128
}
127129

128-
// fileLogger is a wrapper around an output.Logger configured to write
130+
// fileLogger is a wrapper around an log.Logger configured to write
129131
// to a file. It exports two additional methods to get the log file name
130132
// and close the file handle when the operation is finished.
131133
type fileLogger struct {
132-
// TODO(josebalius): should we use https://pkg.go.dev/log#New instead?
133-
*output.Logger
134+
*log.Logger
134135

135136
f *os.File
136137
}
137138

138139
// newFileLogger creates a new fileLogger. It returns an error if the file
139-
// cannot be created. The file is created in the operating system tmp directory
140-
// under the name parameter.
141-
func newFileLogger(name string) (*fileLogger, error) {
142-
f, err := ioutil.TempFile("", name)
143-
if err != nil {
144-
return nil, err
140+
// cannot be created. The file is created on the specified path, if the path
141+
// is empty it is created in the temporary directory.
142+
func newFileLogger(file string) (fl *fileLogger, err error) {
143+
var f *os.File
144+
if file == "" {
145+
f, err = ioutil.TempFile("", "")
146+
if err != nil {
147+
return nil, fmt.Errorf("failed to create tmp file: %w", err)
148+
}
149+
} else {
150+
f, err = os.Create(file)
151+
if err != nil {
152+
return nil, err
153+
}
145154
}
155+
146156
return &fileLogger{
147-
Logger: output.NewLogger(f, f, false),
157+
Logger: log.New(f, "", log.LstdFlags),
148158
f: f,
149159
}, nil
150160
}

‎pkg/liveshare/client.go‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,8 @@ import (
2424
)
2525

2626
type logger interface {
27-
Println(v ...interface{}) (int, error)
28-
Printf(f string, v ...interface{}) (int, error)
29-
}
30-
31-
type noopLogger struct{}
32-
33-
func (n noopLogger) Println(...interface{}) (int, error) {
34-
return 0, nil
35-
}
36-
37-
func (n noopLogger) Printf(string, ...interface{}) (int, error) {
38-
return 0, nil
27+
Println(v ...interface{})
28+
Printf(f string, v ...interface{})
3929
}
4030

4131
// An Options specifies Live Share connection parameters.
@@ -46,8 +36,8 @@ type Options struct {
4636
RelaySAS string
4737
RelayEndpoint string
4838
HostPublicKeys []string
39+
Logger logger // required
4940
TLSConfig *tls.Config // (optional)
50-
Logger logger // (optional)
5141
}
5242

5343
// uri returns a websocket URL for the specified options.
@@ -85,9 +75,8 @@ func Connect(ctx context.Context, opts Options) (*Session, error) {
8575
return nil, err
8676
}
8777

88-
var sessionLogger logger = noopLogger{}
89-
if opts.Logger != nil {
90-
sessionLogger = opts.Logger
78+
if opts.Logger == nil {
79+
return nil, errors.New("Logger is required")
9180
}
9281

9382
sock := newSocket(uri, opts.TLSConfig)
@@ -124,7 +113,7 @@ func Connect(ctx context.Context, opts Options) (*Session, error) {
124113
rpc: rpc,
125114
clientName: opts.ClientName,
126115
keepAliveReason: make(chan string, 1),
127-
logger: sessionLogger,
116+
logger: opts.Logger,
128117
}
129118
go s.heartbeat(ctx, 1*time.Minute)
130119

‎pkg/liveshare/client_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ func TestConnect(t *testing.T) {
2020
SessionToken: "session-token",
2121
RelaySAS: "relay-sas",
2222
HostPublicKeys: []string{livesharetest.SSHPublicKey},
23+
Logger: newMockLogger(),
2324
}
2425
joinWorkspace := func(req *jsonrpc2.Request) (interface{}, error) {
2526
var joinWorkspaceReq joinWorkspaceArgs

‎pkg/liveshare/session_test.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ func makeMockSession(opts ...livesharetest.ServerOption) (*livesharetest.Server,
4141
RelaySAS: "relay-sas",
4242
HostPublicKeys: []string{livesharetest.SSHPublicKey},
4343
TLSConfig: &tls.Config{InsecureSkipVerify: true},
44+
Logger: newMockLogger(),
4445
})
4546
if err != nil {
4647
return nil, nil, fmt.Errorf("error connecting to Live Share: %w", err)
@@ -383,16 +384,16 @@ func newMockLogger() *mockLogger {
383384
return &mockLogger{buf: new(bytes.Buffer)}
384385
}
385386

386-
func (m *mockLogger) Printf(format string, v ...interface{}) (int, error) {
387+
func (m *mockLogger) Printf(format string, v ...interface{}) {
387388
m.Lock()
388389
defer m.Unlock()
389-
return m.buf.WriteString(fmt.Sprintf(format, v...))
390+
m.buf.WriteString(fmt.Sprintf(format, v...))
390391
}
391392

392-
func (m *mockLogger) Println(v ...interface{}) (int, error) {
393+
func (m *mockLogger) Println(v ...interface{}) {
393394
m.Lock()
394395
defer m.Unlock()
395-
return m.buf.WriteString(fmt.Sprintln(v...))
396+
m.buf.WriteString(fmt.Sprintln(v...))
396397
}
397398

398399
func (m *mockLogger) String() string {

0 commit comments

Comments
 (0)
X Tutup