From 4a81e46c1ae3f52612a79883f368ef00da2105c5 Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Fri, 7 Oct 2022 22:05:16 +0000 Subject: [PATCH 1/9] Add rebuilding state --- internal/codespaces/api/api.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index b8d44d9e0..524c2dcee 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -210,6 +210,8 @@ const ( CodespaceStateShutdown = "Shutdown" // CodespaceStateStarting is the state for a starting codespace environment. CodespaceStateStarting = "Starting" + // CodespaceStateRebuilding is the state for a rebuilding codespace environment. + CodespaceStateRebuilding = "Rebuilding" ) type CodespaceConnection struct { From da91216c31a864c1ad13595d9e5635ec33848bb2 Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Sat, 8 Oct 2022 00:04:45 +0000 Subject: [PATCH 2/9] Add new Rebuild function --- pkg/cmd/codespace/common.go | 1 + pkg/liveshare/session.go | 14 ++++++++++++++ pkg/liveshare/session_test.go | 18 ++++++++++++++++++ 3 files changed, 33 insertions(+) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 46e69d97c..a2d1a2316 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -66,6 +66,7 @@ type liveshareSession interface { StartSharing(context.Context, string, int) (liveshare.ChannelID, error) StartSSHServer(context.Context) (int, string, error) StartSSHServerWithOptions(context.Context, liveshare.StartSSHServerOptions) (int, string, error) + Rebuild(context.Context) error } // Connects to a codespace using Live Share and returns that session diff --git a/pkg/liveshare/session.go b/pkg/liveshare/session.go index f912fa5ea..8518afd56 100644 --- a/pkg/liveshare/session.go +++ b/pkg/liveshare/session.go @@ -123,6 +123,20 @@ func (s *Session) StartJupyterServer(ctx context.Context) (int, string, error) { return port, response.ServerUrl, nil } +func (s *Session) Rebuild(ctx context.Context) error { + var success bool + err := s.rpc.do(ctx, "IEnvironmentConfigurationService.rebuildContainer", []string{}, &success) + if err != nil { + return fmt.Errorf("invoking rebuild RPC: %w", err) + } + + if !success { + return fmt.Errorf("couldn't rebuild codespace") + } else { + return nil + } +} + // heartbeat runs until context cancellation, periodically checking whether there is a // reason to keep the connection alive, and if so, notifying the Live Share host to do so. // Heartbeat ensures it does not send more than one request every "interval" to ratelimit diff --git a/pkg/liveshare/session_test.go b/pkg/liveshare/session_test.go index cfe8ccd11..2160caf6e 100644 --- a/pkg/liveshare/session_test.go +++ b/pkg/liveshare/session_test.go @@ -399,6 +399,24 @@ func TestSessionHeartbeat(t *testing.T) { } } +func TestRebuild(t *testing.T) { + getSharedServers := func(conn *jsonrpc2.Conn, req *jsonrpc2.Request) (interface{}, error) { + return true, nil + } + testServer, session, err := makeMockSession( + livesharetest.WithService("IEnvironmentConfigurationService.rebuildContainer", getSharedServers), + ) + if err != nil { + t.Fatalf("creating mock session: %v", err) + } + defer testServer.Close() + + err = session.Rebuild(context.Background()) + if err != nil { + t.Fatalf("rebuilding codespace over mock session: %v", err) + } +} + type mockLogger struct { sync.Mutex buf *bytes.Buffer From 4c49fd3e64d39e74e9020309b86c112e94e4a1b0 Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Sat, 8 Oct 2022 06:12:36 +0000 Subject: [PATCH 3/9] Add rebuild command --- pkg/cmd/codespace/rebuild.go | 57 +++++++++++++++++++++++++++++++ pkg/cmd/codespace/rebuild_test.go | 36 +++++++++++++++++++ pkg/cmd/codespace/root.go | 1 + 3 files changed, 94 insertions(+) create mode 100644 pkg/cmd/codespace/rebuild.go create mode 100644 pkg/cmd/codespace/rebuild_test.go diff --git a/pkg/cmd/codespace/rebuild.go b/pkg/cmd/codespace/rebuild.go new file mode 100644 index 000000000..56e841dfe --- /dev/null +++ b/pkg/cmd/codespace/rebuild.go @@ -0,0 +1,57 @@ +package codespace + +import ( + "context" + "fmt" + + "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/spf13/cobra" +) + +func newRebuildCmd(app *App) *cobra.Command { + var codespace string + + rebuildCmd := &cobra.Command{ + Use: "rebuild", + Short: "Rebuild a codespace", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + return app.Rebuild(cmd.Context(), codespace) + }, + } + + rebuildCmd.Flags().StringVarP(&codespace, "codespace", "c", "", "Name of the codespace") + + return rebuildCmd +} + +func (a *App) Rebuild(ctx context.Context, codespaceName string) (err error) { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + if err != nil { + return err + } + + // Users can't change their codespace while it's rebuilding, so there's + // no need to execute this command. + if codespace.State == api.CodespaceStateRebuilding { + fmt.Fprintf(a.io.Out, "%s is already rebuilding\n", codespace.Name) + return nil + } + + session, err := startLiveShareSession(ctx, codespace, a, false, "") + if err != nil { + return err + } + defer safeClose(session, &err) + + err = session.Rebuild(ctx) + if err != nil { + return fmt.Errorf("couldn't rebuild codespace: %w", err) + } + + fmt.Fprintf(a.io.Out, "%s is rebuilding\n", codespace.Name) + return nil +} diff --git a/pkg/cmd/codespace/rebuild_test.go b/pkg/cmd/codespace/rebuild_test.go new file mode 100644 index 000000000..effaae28d --- /dev/null +++ b/pkg/cmd/codespace/rebuild_test.go @@ -0,0 +1,36 @@ +package codespace + +import ( + "context" + "testing" + + "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/cli/cli/v2/pkg/iostreams" +) + +func TestAlreadyRebuildingCodespace(t *testing.T) { + app := testingRebuildApp() + + err := app.Rebuild(context.Background(), "rebuildingCodespace") + if err != nil { + t.Errorf("error rebuilding a codespace that is already rebuilding: %v", err) + } +} + +func testingRebuildApp() *App { + rebuildingCodespace := &api.Codespace{ + Name: "rebuildingCodespace", + State: api.CodespaceStateRebuilding, + } + apiMock := &apiClientMock{ + GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) { + if name == rebuildingCodespace.Name { + return rebuildingCodespace, nil + } + return nil, nil + }, + } + + ios, _, _, _ := iostreams.Test() + return NewApp(ios, nil, apiMock, nil) +} diff --git a/pkg/cmd/codespace/root.go b/pkg/cmd/codespace/root.go index d700664b1..8439430aa 100644 --- a/pkg/cmd/codespace/root.go +++ b/pkg/cmd/codespace/root.go @@ -22,6 +22,7 @@ func NewRootCmd(app *App) *cobra.Command { root.AddCommand(newCpCmd(app)) root.AddCommand(newStopCmd(app)) root.AddCommand(newSelectCmd(app)) + root.AddCommand(newRebuildCmd(app)) return root } From d05cdf5ff32629f8661066b1c4e4b67f79abad0a Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Sat, 8 Oct 2022 06:30:42 +0000 Subject: [PATCH 4/9] Tidy up comments and errors --- pkg/cmd/codespace/rebuild.go | 7 +++---- pkg/cmd/codespace/rebuild_test.go | 2 +- pkg/liveshare/session.go | 6 +++--- pkg/liveshare/session_test.go | 2 +- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/codespace/rebuild.go b/pkg/cmd/codespace/rebuild.go index 56e841dfe..9988f196a 100644 --- a/pkg/cmd/codespace/rebuild.go +++ b/pkg/cmd/codespace/rebuild.go @@ -34,8 +34,7 @@ func (a *App) Rebuild(ctx context.Context, codespaceName string) (err error) { return err } - // Users can't change their codespace while it's rebuilding, so there's - // no need to execute this command. + // There's no need to rebuild again because users can't modify their codespace while it rebuilds if codespace.State == api.CodespaceStateRebuilding { fmt.Fprintf(a.io.Out, "%s is already rebuilding\n", codespace.Name) return nil @@ -43,13 +42,13 @@ func (a *App) Rebuild(ctx context.Context, codespaceName string) (err error) { session, err := startLiveShareSession(ctx, codespace, a, false, "") if err != nil { - return err + return fmt.Errorf("starting Live Share session: %w", err) } defer safeClose(session, &err) err = session.Rebuild(ctx) if err != nil { - return fmt.Errorf("couldn't rebuild codespace: %w", err) + return fmt.Errorf("rebuilding codespace via session: %w", err) } fmt.Fprintf(a.io.Out, "%s is rebuilding\n", codespace.Name) diff --git a/pkg/cmd/codespace/rebuild_test.go b/pkg/cmd/codespace/rebuild_test.go index effaae28d..bb268efee 100644 --- a/pkg/cmd/codespace/rebuild_test.go +++ b/pkg/cmd/codespace/rebuild_test.go @@ -13,7 +13,7 @@ func TestAlreadyRebuildingCodespace(t *testing.T) { err := app.Rebuild(context.Background(), "rebuildingCodespace") if err != nil { - t.Errorf("error rebuilding a codespace that is already rebuilding: %v", err) + t.Errorf("rebuilding a codespace that was already rebuilding: %v", err) } } diff --git a/pkg/liveshare/session.go b/pkg/liveshare/session.go index 8518afd56..99f82af99 100644 --- a/pkg/liveshare/session.go +++ b/pkg/liveshare/session.go @@ -124,13 +124,13 @@ func (s *Session) StartJupyterServer(ctx context.Context) (int, string, error) { } func (s *Session) Rebuild(ctx context.Context) error { - var success bool - err := s.rpc.do(ctx, "IEnvironmentConfigurationService.rebuildContainer", []string{}, &success) + var rebuildSuccess bool + err := s.rpc.do(ctx, "IEnvironmentConfigurationService.rebuildContainer", []string{}, &rebuildSuccess) if err != nil { return fmt.Errorf("invoking rebuild RPC: %w", err) } - if !success { + if !rebuildSuccess { return fmt.Errorf("couldn't rebuild codespace") } else { return nil diff --git a/pkg/liveshare/session_test.go b/pkg/liveshare/session_test.go index 2160caf6e..1af35b2e0 100644 --- a/pkg/liveshare/session_test.go +++ b/pkg/liveshare/session_test.go @@ -413,7 +413,7 @@ func TestRebuild(t *testing.T) { err = session.Rebuild(context.Background()) if err != nil { - t.Fatalf("rebuilding codespace over mock session: %v", err) + t.Fatalf("rebuilding codespace via mock session: %v", err) } } From 30fcddeccb7a2b45c38dd52527f871f06ab6e464 Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Sun, 9 Oct 2022 00:59:54 +0000 Subject: [PATCH 5/9] Use nil instead of []string{} --- pkg/liveshare/session.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/liveshare/session.go b/pkg/liveshare/session.go index 99f82af99..386d4d3f2 100644 --- a/pkg/liveshare/session.go +++ b/pkg/liveshare/session.go @@ -125,7 +125,7 @@ func (s *Session) StartJupyterServer(ctx context.Context) (int, string, error) { func (s *Session) Rebuild(ctx context.Context) error { var rebuildSuccess bool - err := s.rpc.do(ctx, "IEnvironmentConfigurationService.rebuildContainer", []string{}, &rebuildSuccess) + err := s.rpc.do(ctx, "IEnvironmentConfigurationService.rebuildContainer", nil, &rebuildSuccess) if err != nil { return fmt.Errorf("invoking rebuild RPC: %w", err) } From b373fbbe4447b7740b86571ef7c5ad3166b8148d Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Sun, 9 Oct 2022 01:00:14 +0000 Subject: [PATCH 6/9] Remove else block --- pkg/liveshare/session.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/liveshare/session.go b/pkg/liveshare/session.go index 386d4d3f2..ee97e52bb 100644 --- a/pkg/liveshare/session.go +++ b/pkg/liveshare/session.go @@ -132,9 +132,9 @@ func (s *Session) Rebuild(ctx context.Context) error { if !rebuildSuccess { return fmt.Errorf("couldn't rebuild codespace") - } else { - return nil } + + return nil } // heartbeat runs until context cancellation, periodically checking whether there is a From 6704f38ffcbcbd10d3473a628ba6c5fa430d730c Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Sun, 9 Oct 2022 01:05:52 +0000 Subject: [PATCH 7/9] Verify that RPC was called --- pkg/liveshare/session_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/liveshare/session_test.go b/pkg/liveshare/session_test.go index 1af35b2e0..98110a0cd 100644 --- a/pkg/liveshare/session_test.go +++ b/pkg/liveshare/session_test.go @@ -400,7 +400,9 @@ func TestSessionHeartbeat(t *testing.T) { } func TestRebuild(t *testing.T) { + requestCount := 0 getSharedServers := func(conn *jsonrpc2.Conn, req *jsonrpc2.Request) (interface{}, error) { + requestCount++ return true, nil } testServer, session, err := makeMockSession( @@ -415,6 +417,10 @@ func TestRebuild(t *testing.T) { if err != nil { t.Fatalf("rebuilding codespace via mock session: %v", err) } + + if requestCount == 0 { + t.Fatalf("no requests were made") + } } type mockLogger struct { From 0e963ebd4f85be5216852dd0ae6d6224d67920e0 Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Sun, 9 Oct 2022 01:11:04 +0000 Subject: [PATCH 8/9] Move codespace construction to its test --- pkg/cmd/codespace/rebuild_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/codespace/rebuild_test.go b/pkg/cmd/codespace/rebuild_test.go index bb268efee..fff40fe1b 100644 --- a/pkg/cmd/codespace/rebuild_test.go +++ b/pkg/cmd/codespace/rebuild_test.go @@ -9,7 +9,11 @@ import ( ) func TestAlreadyRebuildingCodespace(t *testing.T) { - app := testingRebuildApp() + rebuildingCodespace := &api.Codespace{ + Name: "rebuildingCodespace", + State: api.CodespaceStateRebuilding, + } + app := testingRebuildApp(*rebuildingCodespace) err := app.Rebuild(context.Background(), "rebuildingCodespace") if err != nil { @@ -17,15 +21,11 @@ func TestAlreadyRebuildingCodespace(t *testing.T) { } } -func testingRebuildApp() *App { - rebuildingCodespace := &api.Codespace{ - Name: "rebuildingCodespace", - State: api.CodespaceStateRebuilding, - } +func testingRebuildApp(mockCodespace api.Codespace) *App { apiMock := &apiClientMock{ GetCodespaceFunc: func(_ context.Context, name string, _ bool) (*api.Codespace, error) { - if name == rebuildingCodespace.Name { - return rebuildingCodespace, nil + if name == mockCodespace.Name { + return &mockCodespace, nil } return nil, nil }, From 982aa5ba82810043507711689706ba23afe94359 Mon Sep 17 00:00:00 2001 From: JP Ungaretti Date: Mon, 10 Oct 2022 21:01:29 +0000 Subject: [PATCH 9/9] Rename RPC command to RebuildContainer --- pkg/cmd/codespace/common.go | 2 +- pkg/cmd/codespace/rebuild.go | 2 +- pkg/liveshare/session.go | 2 +- pkg/liveshare/session_test.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index a2d1a2316..0cdd69dfa 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -66,7 +66,7 @@ type liveshareSession interface { StartSharing(context.Context, string, int) (liveshare.ChannelID, error) StartSSHServer(context.Context) (int, string, error) StartSSHServerWithOptions(context.Context, liveshare.StartSSHServerOptions) (int, string, error) - Rebuild(context.Context) error + RebuildContainer(context.Context) error } // Connects to a codespace using Live Share and returns that session diff --git a/pkg/cmd/codespace/rebuild.go b/pkg/cmd/codespace/rebuild.go index 9988f196a..f2128d632 100644 --- a/pkg/cmd/codespace/rebuild.go +++ b/pkg/cmd/codespace/rebuild.go @@ -46,7 +46,7 @@ func (a *App) Rebuild(ctx context.Context, codespaceName string) (err error) { } defer safeClose(session, &err) - err = session.Rebuild(ctx) + err = session.RebuildContainer(ctx) if err != nil { return fmt.Errorf("rebuilding codespace via session: %w", err) } diff --git a/pkg/liveshare/session.go b/pkg/liveshare/session.go index ee97e52bb..40bdf287b 100644 --- a/pkg/liveshare/session.go +++ b/pkg/liveshare/session.go @@ -123,7 +123,7 @@ func (s *Session) StartJupyterServer(ctx context.Context) (int, string, error) { return port, response.ServerUrl, nil } -func (s *Session) Rebuild(ctx context.Context) error { +func (s *Session) RebuildContainer(ctx context.Context) error { var rebuildSuccess bool err := s.rpc.do(ctx, "IEnvironmentConfigurationService.rebuildContainer", nil, &rebuildSuccess) if err != nil { diff --git a/pkg/liveshare/session_test.go b/pkg/liveshare/session_test.go index 98110a0cd..06000c344 100644 --- a/pkg/liveshare/session_test.go +++ b/pkg/liveshare/session_test.go @@ -413,7 +413,7 @@ func TestRebuild(t *testing.T) { } defer testServer.Close() - err = session.Rebuild(context.Background()) + err = session.RebuildContainer(context.Background()) if err != nil { t.Fatalf("rebuilding codespace via mock session: %v", err) }