From 57c73e82398bc0cb79e19ef6b263bf070268f00e Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Wed, 22 Feb 2023 17:16:36 -0600 Subject: [PATCH] Add `--repo` filter to more `gh codespaces` commands (#6669) * Add --repo flag to commands * Example of using common args * Add -r to stop * Add validation to the add helper * Skip prompt for single option * Migrate (mostly) everything to addGetOrChooseCodespaceCommandArgs * Fix typo in logsCmd * Use R instead of r * Update a couple -r usages * Refactor to codespace_selector * Clean up selector, apply it in a couple examples * Use apiClient instead in constructor * Restore addDeprecatedRepoShorthand * Finish implementing the commands * Update existing tests to use the selector * Add tests for selector * linter * Catch case where there's no conflict * Make the flag persistent for ports * Add support to stop --- pkg/cmd/codespace/code.go | 11 +- pkg/cmd/codespace/code_test.go | 7 +- pkg/cmd/codespace/codespace_selector.go | 123 +++++++++++++++++ pkg/cmd/codespace/codespace_selector_test.go | 137 +++++++++++++++++++ pkg/cmd/codespace/common.go | 47 ++----- pkg/cmd/codespace/delete.go | 16 ++- pkg/cmd/codespace/edit.go | 27 ++-- pkg/cmd/codespace/edit_test.go | 17 ++- pkg/cmd/codespace/jupyter.go | 10 +- pkg/cmd/codespace/logs.go | 13 +- pkg/cmd/codespace/logs_test.go | 3 +- pkg/cmd/codespace/ports.go | 50 +++---- pkg/cmd/codespace/ports_test.go | 13 +- pkg/cmd/codespace/rebuild.go | 15 +- pkg/cmd/codespace/rebuild_test.go | 3 +- pkg/cmd/codespace/select.go | 37 ++++- pkg/cmd/codespace/select_test.go | 5 +- pkg/cmd/codespace/ssh.go | 16 +-- pkg/cmd/codespace/ssh_test.go | 3 +- pkg/cmd/codespace/stop.go | 26 ++-- pkg/cmd/codespace/stop_test.go | 8 +- 21 files changed, 432 insertions(+), 155 deletions(-) create mode 100644 pkg/cmd/codespace/codespace_selector.go create mode 100644 pkg/cmd/codespace/codespace_selector_test.go diff --git a/pkg/cmd/codespace/code.go b/pkg/cmd/codespace/code.go index 429b5ce71..77153ccd7 100644 --- a/pkg/cmd/codespace/code.go +++ b/pkg/cmd/codespace/code.go @@ -10,7 +10,7 @@ import ( func newCodeCmd(app *App) *cobra.Command { var ( - codespace string + selector *CodespaceSelector useInsiders bool useWeb bool ) @@ -20,11 +20,12 @@ func newCodeCmd(app *App) *cobra.Command { Short: "Open a codespace in Visual Studio Code", Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { - return app.VSCode(cmd.Context(), codespace, useInsiders, useWeb) + return app.VSCode(cmd.Context(), selector, useInsiders, useWeb) }, } - codeCmd.Flags().StringVarP(&codespace, "codespace", "c", "", "Name of the codespace") + selector = AddCodespaceSelector(codeCmd, app.apiClient) + codeCmd.Flags().BoolVar(&useInsiders, "insiders", false, "Use the insiders version of Visual Studio Code") codeCmd.Flags().BoolVarP(&useWeb, "web", "w", false, "Use the web version of Visual Studio Code") @@ -32,8 +33,8 @@ func newCodeCmd(app *App) *cobra.Command { } // VSCode opens a codespace in the local VS VSCode application. -func (a *App) VSCode(ctx context.Context, codespaceName string, useInsiders bool, useWeb bool) error { - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) +func (a *App) VSCode(ctx context.Context, selector *CodespaceSelector, useInsiders bool, useWeb bool) error { + codespace, err := selector.Select(ctx) if err != nil { return err } diff --git a/pkg/cmd/codespace/code_test.go b/pkg/cmd/codespace/code_test.go index f43d8a20c..be2743c5d 100644 --- a/pkg/cmd/codespace/code_test.go +++ b/pkg/cmd/codespace/code_test.go @@ -69,7 +69,9 @@ func TestApp_VSCode(t *testing.T) { apiClient: testCodeApiMock(), io: ios, } - if err := a.VSCode(context.Background(), tt.args.codespaceName, tt.args.useInsiders, tt.args.useWeb); (err != nil) != tt.wantErr { + selector := &CodespaceSelector{api: a.apiClient, codespaceName: tt.args.codespaceName} + + if err := a.VSCode(context.Background(), selector, tt.args.useInsiders, tt.args.useWeb); (err != nil) != tt.wantErr { t.Errorf("App.VSCode() error = %v, wantErr %v", err, tt.wantErr) } b.Verify(t, tt.wantURL) @@ -85,8 +87,9 @@ func TestApp_VSCode(t *testing.T) { func TestPendingOperationDisallowsCode(t *testing.T) { app := testingCodeApp() + selector := &CodespaceSelector{api: app.apiClient, codespaceName: "disabledCodespace"} - if err := app.VSCode(context.Background(), "disabledCodespace", false, false); err != nil { + if err := app.VSCode(context.Background(), selector, false, false); err != nil { if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } diff --git a/pkg/cmd/codespace/codespace_selector.go b/pkg/cmd/codespace/codespace_selector.go new file mode 100644 index 000000000..69b9ed06b --- /dev/null +++ b/pkg/cmd/codespace/codespace_selector.go @@ -0,0 +1,123 @@ +package codespace + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/spf13/cobra" +) + +type CodespaceSelector struct { + api apiClient + + repoName string + codespaceName string +} + +var errNoFilteredCodespaces = errors.New("you have no codespaces meeting the filter criteria") + +// AddCodespaceSelector adds persistent flags for selecting a codespace to the given command and returns a CodespaceSelector which applies them +func AddCodespaceSelector(cmd *cobra.Command, api apiClient) *CodespaceSelector { + cs := &CodespaceSelector{api: api} + + cmd.PersistentFlags().StringVarP(&cs.codespaceName, "codespace", "c", "", "Name of the codespace") + cmd.PersistentFlags().StringVarP(&cs.repoName, "repo", "R", "", "Filter codespace selection by repository name (user/repo)") + + cmd.MarkFlagsMutuallyExclusive("codespace", "repo") + + return cs +} + +func (cs *CodespaceSelector) Select(ctx context.Context) (codespace *api.Codespace, err error) { + if cs.codespaceName != "" { + codespace, err = cs.api.GetCodespace(ctx, cs.codespaceName, true) + if err != nil { + return nil, fmt.Errorf("getting full codespace details: %w", err) + } + } else { + codespaces, err := cs.fetchCodespaces(ctx) + if err != nil { + return nil, err + } + + codespace, err = cs.chooseCodespace(ctx, codespaces) + if err != nil { + return nil, err + } + } + + if codespace.PendingOperation { + return nil, fmt.Errorf( + "codespace is disabled while it has a pending operation: %s", + codespace.PendingOperationDisabledReason, + ) + } + + return codespace, nil +} + +func (cs *CodespaceSelector) SelectName(ctx context.Context) (string, error) { + if cs.codespaceName != "" { + return cs.codespaceName, nil + } + + codespaces, err := cs.fetchCodespaces(ctx) + if err != nil { + return "", err + } + + codespace, err := cs.chooseCodespace(ctx, codespaces) + if err != nil { + return "", err + } + + return codespace.Name, nil +} + +func (cs *CodespaceSelector) fetchCodespaces(ctx context.Context) (codespaces []*api.Codespace, err error) { + codespaces, err = cs.api.ListCodespaces(ctx, api.ListCodespacesOptions{}) + if err != nil { + return nil, fmt.Errorf("error getting codespaces: %w", err) + } + + if len(codespaces) == 0 { + return nil, errNoCodespaces + } + + // Note that repo filtering done here can also be done in api.ListCodespaces. + // We do it here instead so that we can differentiate no codespaces in general vs. none after filtering. + if cs.repoName != "" { + var filteredCodespaces []*api.Codespace + for _, c := range codespaces { + if !strings.EqualFold(c.Repository.FullName, cs.repoName) { + continue + } + + filteredCodespaces = append(filteredCodespaces, c) + } + + codespaces = filteredCodespaces + } + + if len(codespaces) == 0 { + return nil, errNoFilteredCodespaces + } + + return codespaces, err +} + +func (cs *CodespaceSelector) chooseCodespace(ctx context.Context, codespaces []*api.Codespace) (codespace *api.Codespace, err error) { + skipPromptForSingleOption := cs.repoName != "" + codespace, err = chooseCodespaceFromList(ctx, codespaces, false, skipPromptForSingleOption) + if err != nil { + if err == errNoCodespaces { + return nil, err + } + return nil, fmt.Errorf("choosing codespace: %w", err) + } + + return codespace, nil +} diff --git a/pkg/cmd/codespace/codespace_selector_test.go b/pkg/cmd/codespace/codespace_selector_test.go new file mode 100644 index 000000000..30ba3c588 --- /dev/null +++ b/pkg/cmd/codespace/codespace_selector_test.go @@ -0,0 +1,137 @@ +package codespace + +import ( + "context" + "fmt" + "testing" + + "github.com/cli/cli/v2/internal/codespaces/api" +) + +func TestSelectWithCodespaceName(t *testing.T) { + wantName := "mock-codespace" + + api := &apiClientMock{ + GetCodespaceFunc: func(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error) { + if name != wantName { + t.Errorf("incorrect name: want %s, got %s", wantName, name) + } + + return &api.Codespace{}, nil + }, + } + + cs := &CodespaceSelector{api: api, codespaceName: wantName} + + _, err := cs.Select(context.Background()) + + if err != nil { + t.Errorf("unexpected error: %v", err) + } +} + +func TestSelectNameWithCodespaceName(t *testing.T) { + wantName := "mock-codespace" + + cs := &CodespaceSelector{codespaceName: wantName} + + name, err := cs.SelectName(context.Background()) + + if name != wantName { + t.Errorf("incorrect name: want %s, got %s", wantName, name) + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + } +} + +func TestFetchCodespaces(t *testing.T) { + var ( + repoA1 = &api.Codespace{Name: "1", Repository: api.Repository{FullName: "mock/A"}} + repoA2 = &api.Codespace{Name: "2", Repository: api.Repository{FullName: "mock/A"}} + + repoB1 = &api.Codespace{Name: "1", Repository: api.Repository{FullName: "mock/B"}} + ) + + tests := []struct { + tName string + apiCodespaces []*api.Codespace + repoName string + wantCodespaces []*api.Codespace + wantErr error + }{ + // Empty case + { + "empty", nil, "", nil, errNoCodespaces, + }, + + // Tests with no filtering + { + "no filtering, single codespace", + []*api.Codespace{repoA1}, + "", + []*api.Codespace{repoA1}, + nil, + }, + { + "no filtering, multiple codespaces", + []*api.Codespace{repoA1, repoA2, repoB1}, + "", + []*api.Codespace{repoA1, repoA2, repoB1}, + nil, + }, + + // Test repo filtering + { + "repo filtering, single codespace", + []*api.Codespace{repoA1}, + "mock/A", + []*api.Codespace{repoA1}, + nil, + }, + { + "repo filtering, multiple codespaces", + []*api.Codespace{repoA1, repoA2, repoB1}, + "mock/A", + []*api.Codespace{repoA1, repoA2}, + nil, + }, + { + "repo filtering, multiple codespaces 2", + []*api.Codespace{repoA1, repoA2, repoB1}, + "mock/B", + []*api.Codespace{repoB1}, + nil, + }, + { + "repo filtering, no matches", + []*api.Codespace{repoA1, repoA2, repoB1}, + "mock/C", + nil, + errNoFilteredCodespaces, + }, + } + + for _, tt := range tests { + t.Run(tt.tName, func(t *testing.T) { + api := &apiClientMock{ + ListCodespacesFunc: func(ctx context.Context, opts api.ListCodespacesOptions) ([]*api.Codespace, error) { + return tt.apiCodespaces, nil + }, + } + + cs := &CodespaceSelector{api: api, repoName: tt.repoName} + + codespaces, err := cs.fetchCodespaces(context.Background()) + + if err != tt.wantErr { + t.Errorf("expected error to be %v, got %v", tt.wantErr, err) + } + + if fmt.Sprintf("%v", tt.wantCodespaces) != fmt.Sprintf("%v", codespaces) { + t.Errorf("expected codespaces to be %v, got %v", tt.wantCodespaces, codespaces) + } + }) + } +} diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 24235ed27..3fb7cac10 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -102,21 +102,17 @@ type apiClient interface { var errNoCodespaces = errors.New("you have no codespaces") -func chooseCodespace(ctx context.Context, apiClient apiClient) (*api.Codespace, error) { - codespaces, err := apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{}) - if err != nil { - return nil, fmt.Errorf("error getting codespaces: %w", err) - } - return chooseCodespaceFromList(ctx, codespaces, false) -} - // chooseCodespaceFromList returns the codespace that the user has interactively selected from the list, or // an error if there are no codespaces. -func chooseCodespaceFromList(ctx context.Context, codespaces []*api.Codespace, includeOwner bool) (*api.Codespace, error) { +func chooseCodespaceFromList(ctx context.Context, codespaces []*api.Codespace, includeOwner bool, skipPromptForSingleOption bool) (*api.Codespace, error) { if len(codespaces) == 0 { return nil, errNoCodespaces } + if skipPromptForSingleOption && len(codespaces) == 1 { + return codespaces[0], nil + } + sortedCodespaces := codespaces sort.Slice(sortedCodespaces, func(i, j int) bool { return sortedCodespaces[i].CreatedAt > sortedCodespaces[j].CreatedAt @@ -154,35 +150,6 @@ func formatCodespacesForSelect(codespaces []*api.Codespace, includeOwner bool) [ return names } -// getOrChooseCodespace prompts the user to choose a codespace if the codespaceName is empty. -// It then fetches the codespace record with full connection details. -// TODO(josebalius): accept a progress indicator or *App and show progress when fetching. -func getOrChooseCodespace(ctx context.Context, apiClient apiClient, codespaceName string) (codespace *api.Codespace, err error) { - if codespaceName == "" { - codespace, err = chooseCodespace(ctx, apiClient) - if err != nil { - if err == errNoCodespaces { - return nil, err - } - return nil, fmt.Errorf("choosing codespace: %w", err) - } - } else { - codespace, err = apiClient.GetCodespace(ctx, codespaceName, true) - if err != nil { - return nil, fmt.Errorf("getting full codespace details: %w", err) - } - } - - if codespace.PendingOperation { - return nil, fmt.Errorf( - "codespace is disabled while it has a pending operation: %s", - codespace.PendingOperationDisabledReason, - ) - } - - return codespace, nil -} - func safeClose(closer io.Closer, err *error) { if closeErr := closer.Close(); *err == nil { *err = closeErr @@ -289,5 +256,9 @@ func addDeprecatedRepoShorthand(cmd *cobra.Command, target *string) error { return fmt.Errorf("error marking `-r` shorthand as deprecated: %w", err) } + if cmd.Flag("codespace") != nil { + cmd.MarkFlagsMutuallyExclusive("codespace", "repo-deprecated") + } + return nil } diff --git a/pkg/cmd/codespace/delete.go b/pkg/cmd/codespace/delete.go index 3af44395e..d01c3c642 100644 --- a/pkg/cmd/codespace/delete.go +++ b/pkg/cmd/codespace/delete.go @@ -41,6 +41,8 @@ func newDeleteCmd(app *App) *cobra.Command { prompter: &surveyPrompter{}, } + var selector *CodespaceSelector + deleteCmd := &cobra.Command{ Use: "delete", Short: "Delete codespaces", @@ -54,6 +56,11 @@ func newDeleteCmd(app *App) *cobra.Command { `), Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { + // TODO: ideally we would use the selector directly, but the logic here is too intertwined with other flags to do so elegantly + // After the admin subcommand is added (see https://github.com/cli/cli/pull/6944#issuecomment-1419553639) we can revisit this. + opts.codespaceName = selector.codespaceName + opts.repoFilter = selector.repoName + if opts.deleteAll && opts.repoFilter != "" { return cmdutil.FlagErrorf("both `--all` and `--repo` is not supported") } @@ -64,13 +71,12 @@ func newDeleteCmd(app *App) *cobra.Command { }, } - deleteCmd.Flags().StringVarP(&opts.codespaceName, "codespace", "c", "", "Name of the codespace") - deleteCmd.Flags().BoolVar(&opts.deleteAll, "all", false, "Delete all codespaces") - deleteCmd.Flags().StringVarP(&opts.repoFilter, "repo", "R", "", "Delete codespaces for a `repository`") - if err := addDeprecatedRepoShorthand(deleteCmd, &opts.repoFilter); err != nil { + selector = AddCodespaceSelector(deleteCmd, app.apiClient) + if err := addDeprecatedRepoShorthand(deleteCmd, &selector.repoName); err != nil { fmt.Fprintf(app.io.ErrOut, "%v\n", err) } + deleteCmd.Flags().BoolVar(&opts.deleteAll, "all", false, "Delete all codespaces") deleteCmd.Flags().BoolVarP(&opts.skipConfirm, "force", "f", false, "Skip confirmation for codespaces that contain unsaved changes") deleteCmd.Flags().Uint16Var(&opts.keepDays, "days", 0, "Delete codespaces older than `N` days") deleteCmd.Flags().StringVarP(&opts.orgName, "org", "o", "", "The `login` handle of the organization (admin-only)") @@ -100,7 +106,7 @@ func (a *App) Delete(ctx context.Context, opts deleteOptions) (err error) { if !opts.deleteAll && opts.repoFilter == "" { includeUsername := opts.orgName != "" - c, err := chooseCodespaceFromList(ctx, codespaces, includeUsername) + c, err := chooseCodespaceFromList(ctx, codespaces, includeUsername, false) if err != nil { return fmt.Errorf("error choosing codespace: %w", err) } diff --git a/pkg/cmd/codespace/edit.go b/pkg/cmd/codespace/edit.go index d07173623..0eecbc257 100644 --- a/pkg/cmd/codespace/edit.go +++ b/pkg/cmd/codespace/edit.go @@ -2,6 +2,7 @@ package codespace import ( "context" + "errors" "fmt" "github.com/cli/cli/v2/internal/codespaces/api" @@ -10,9 +11,9 @@ import ( ) type editOptions struct { - codespaceName string - displayName string - machine string + selector *CodespaceSelector + displayName string + machine string } func newEditCmd(app *App) *cobra.Command { @@ -31,7 +32,7 @@ func newEditCmd(app *App) *cobra.Command { }, } - editCmd.Flags().StringVarP(&opts.codespaceName, "codespace", "c", "", "Name of the codespace") + opts.selector = AddCodespaceSelector(editCmd, app.apiClient) editCmd.Flags().StringVarP(&opts.displayName, "display-name", "d", "", "Set the display name") editCmd.Flags().StringVar(&opts.displayName, "displayName", "", "display name") if err := editCmd.Flags().MarkDeprecated("displayName", "use `--display-name` instead"); err != nil { @@ -44,21 +45,17 @@ func newEditCmd(app *App) *cobra.Command { // Edits a codespace func (a *App) Edit(ctx context.Context, opts editOptions) error { - codespaceName := opts.codespaceName - - if codespaceName == "" { - selectedCodespace, err := chooseCodespace(ctx, a.apiClient) - if err != nil { - if err == errNoCodespaces { - return err - } - return fmt.Errorf("error choosing codespace: %w", err) + codespaceName, err := opts.selector.SelectName(ctx) + if err != nil { + // TODO: is there a cleaner way to do this? + if errors.Is(err, errNoCodespaces) || errors.Is(err, errNoFilteredCodespaces) { + return err } - codespaceName = selectedCodespace.Name + return fmt.Errorf("error choosing codespace: %w", err) } a.StartProgressIndicatorWithLabel("Editing codespace") - _, err := a.apiClient.EditCodespace(ctx, codespaceName, &api.EditCodespaceParams{ + _, err = a.apiClient.EditCodespace(ctx, codespaceName, &api.EditCodespaceParams{ DisplayName: opts.displayName, Machine: opts.machine, }) diff --git a/pkg/cmd/codespace/edit_test.go b/pkg/cmd/codespace/edit_test.go index 886d9e455..01fb4f4ef 100644 --- a/pkg/cmd/codespace/edit_test.go +++ b/pkg/cmd/codespace/edit_test.go @@ -23,9 +23,9 @@ func TestEdit(t *testing.T) { { name: "edit codespace display name", opts: editOptions{ - codespaceName: "hubot", - displayName: "hubot-changed", - machine: "", + selector: &CodespaceSelector{codespaceName: "hubot"}, + displayName: "hubot-changed", + machine: "", }, wantEdits: &api.EditCodespaceParams{ DisplayName: "hubot-changed", @@ -54,9 +54,9 @@ func TestEdit(t *testing.T) { { name: "edit codespace machine", opts: editOptions{ - codespaceName: "hubot", - displayName: "", - machine: "machine", + selector: &CodespaceSelector{codespaceName: "hubot"}, + displayName: "", + machine: "machine", }, wantEdits: &api.EditCodespaceParams{ Machine: "machine", @@ -92,6 +92,11 @@ func TestEdit(t *testing.T) { var err error if tt.cliArgs == nil { + if tt.opts.selector == nil { + t.Fatalf("selector must be set in opts if cliArgs are not provided") + } + + tt.opts.selector.api = apiMock err = a.Edit(context.Background(), tt.opts) } else { cmd := newEditCmd(a) diff --git a/pkg/cmd/codespace/jupyter.go b/pkg/cmd/codespace/jupyter.go index 0e3e0dee0..1374916bb 100644 --- a/pkg/cmd/codespace/jupyter.go +++ b/pkg/cmd/codespace/jupyter.go @@ -13,28 +13,28 @@ import ( ) func newJupyterCmd(app *App) *cobra.Command { - var codespace string + var selector *CodespaceSelector jupyterCmd := &cobra.Command{ Use: "jupyter", Short: "Open a codespace in JupyterLab", Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { - return app.Jupyter(cmd.Context(), codespace) + return app.Jupyter(cmd.Context(), selector) }, } - jupyterCmd.Flags().StringVarP(&codespace, "codespace", "c", "", "Name of the codespace") + selector = AddCodespaceSelector(jupyterCmd, app.apiClient) return jupyterCmd } -func (a *App) Jupyter(ctx context.Context, codespaceName string) (err error) { +func (a *App) Jupyter(ctx context.Context, selector *CodespaceSelector) (err error) { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := selector.Select(ctx) if err != nil { return err } diff --git a/pkg/cmd/codespace/logs.go b/pkg/cmd/codespace/logs.go index 9a42b9866..6f0e88402 100644 --- a/pkg/cmd/codespace/logs.go +++ b/pkg/cmd/codespace/logs.go @@ -12,8 +12,8 @@ import ( func newLogsCmd(app *App) *cobra.Command { var ( - codespace string - follow bool + selector *CodespaceSelector + follow bool ) logsCmd := &cobra.Command{ @@ -21,22 +21,23 @@ func newLogsCmd(app *App) *cobra.Command { Short: "Access codespace logs", Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { - return app.Logs(cmd.Context(), codespace, follow) + return app.Logs(cmd.Context(), selector, follow) }, } - logsCmd.Flags().StringVarP(&codespace, "codespace", "c", "", "Name of the codespace") + selector = AddCodespaceSelector(logsCmd, app.apiClient) + logsCmd.Flags().BoolVarP(&follow, "follow", "f", false, "Tail and follow the logs") return logsCmd } -func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err error) { +func (a *App) Logs(ctx context.Context, selector *CodespaceSelector, follow bool) (err error) { // Ensure all child tasks (port forwarding, remote exec) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := selector.Select(ctx) if err != nil { return err } diff --git a/pkg/cmd/codespace/logs_test.go b/pkg/cmd/codespace/logs_test.go index 161657b4d..c4ba1ef59 100644 --- a/pkg/cmd/codespace/logs_test.go +++ b/pkg/cmd/codespace/logs_test.go @@ -10,8 +10,9 @@ import ( func TestPendingOperationDisallowsLogs(t *testing.T) { app := testingLogsApp() + selector := &CodespaceSelector{api: app.apiClient, codespaceName: "disabledCodespace"} - if err := app.Logs(context.Background(), "disabledCodespace", false); err != nil { + if err := app.Logs(context.Background(), selector, false); err != nil { if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } diff --git a/pkg/cmd/codespace/ports.go b/pkg/cmd/codespace/ports.go index d36fa54a1..2f84d1678 100644 --- a/pkg/cmd/codespace/ports.go +++ b/pkg/cmd/codespace/ports.go @@ -29,30 +29,33 @@ const ( // newPortsCmd returns a Cobra "ports" command that displays a table of available ports, // according to the specified flags. func newPortsCmd(app *App) *cobra.Command { - var codespace string - var exporter cmdutil.Exporter + var ( + selector *CodespaceSelector + exporter cmdutil.Exporter + ) portsCmd := &cobra.Command{ Use: "ports", Short: "List ports in a codespace", Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { - return app.ListPorts(cmd.Context(), codespace, exporter) + return app.ListPorts(cmd.Context(), selector, exporter) }, } - portsCmd.PersistentFlags().StringVarP(&codespace, "codespace", "c", "", "Name of the codespace") + selector = AddCodespaceSelector(portsCmd, app.apiClient) + cmdutil.AddJSONFlags(portsCmd, &exporter, portFields) - portsCmd.AddCommand(newPortsForwardCmd(app)) - portsCmd.AddCommand(newPortsVisibilityCmd(app)) + portsCmd.AddCommand(newPortsForwardCmd(app, selector)) + portsCmd.AddCommand(newPortsVisibilityCmd(app, selector)) return portsCmd } // ListPorts lists known ports in a codespace. -func (a *App) ListPorts(ctx context.Context, codespaceName string, exporter cmdutil.Exporter) (err error) { - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) +func (a *App) ListPorts(ctx context.Context, selector *CodespaceSelector, exporter cmdutil.Exporter) (err error) { + codespace, err := selector.Select(ctx) if err != nil { return err } @@ -218,21 +221,14 @@ func getDevContainer(ctx context.Context, apiClient apiClient, codespace *api.Co return ch } -func newPortsVisibilityCmd(app *App) *cobra.Command { +func newPortsVisibilityCmd(app *App, selector *CodespaceSelector) *cobra.Command { return &cobra.Command{ Use: "visibility :{public|private|org}...", Short: "Change the visibility of the forwarded port", Example: "gh codespace ports visibility 80:org 3000:private 8000:public", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - codespace, err := cmd.Flags().GetString("codespace") - if err != nil { - // should only happen if flag is not defined - // or if the flag is not of string type - // since it's a persistent flag that we control it should never happen - return fmt.Errorf("get codespace flag: %w", err) - } - return app.UpdatePortVisibility(cmd.Context(), codespace, args) + return app.UpdatePortVisibility(cmd.Context(), selector, args) }, } } @@ -261,13 +257,13 @@ func (e *ErrUpdatingPortVisibility) Unwrap() error { var errUpdatePortVisibilityForbidden = errors.New("organization admin has forbidden this privacy setting") -func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName string, args []string) (err error) { +func (a *App) UpdatePortVisibility(ctx context.Context, selector *CodespaceSelector, args []string) (err error) { ports, err := a.parsePortVisibilities(args) if err != nil { return fmt.Errorf("error parsing port arguments: %w", err) } - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := selector.Select(ctx) if err != nil { return err } @@ -347,32 +343,24 @@ func (a *App) parsePortVisibilities(args []string) ([]portVisibility, error) { // NewPortsForwardCmd returns a Cobra "ports forward" subcommand, which forwards a set of // port pairs from the codespace to localhost. -func newPortsForwardCmd(app *App) *cobra.Command { +func newPortsForwardCmd(app *App, selector *CodespaceSelector) *cobra.Command { return &cobra.Command{ Use: "forward :...", Short: "Forward ports", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - codespace, err := cmd.Flags().GetString("codespace") - if err != nil { - // should only happen if flag is not defined - // or if the flag is not of string type - // since it's a persistent flag that we control it should never happen - return fmt.Errorf("get codespace flag: %w", err) - } - - return app.ForwardPorts(cmd.Context(), codespace, args) + return app.ForwardPorts(cmd.Context(), selector, args) }, } } -func (a *App) ForwardPorts(ctx context.Context, codespaceName string, ports []string) (err error) { +func (a *App) ForwardPorts(ctx context.Context, selector *CodespaceSelector, ports []string) (err error) { portPairs, err := getPortPairs(ports) if err != nil { return fmt.Errorf("get port pairs: %w", err) } - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := selector.Select(ctx) if err != nil { return err } diff --git a/pkg/cmd/codespace/ports_test.go b/pkg/cmd/codespace/ports_test.go index ea61b11a5..9979345b2 100644 --- a/pkg/cmd/codespace/ports_test.go +++ b/pkg/cmd/codespace/ports_test.go @@ -207,13 +207,16 @@ func runUpdateVisibilityTest(t *testing.T, portVisibilities []portVisibility, ev portArgs = append(portArgs, fmt.Sprintf("%d:%s", pv.number, pv.visibility)) } - return a.UpdatePortVisibility(ctx, "codespace-name", portArgs) + selector := &CodespaceSelector{api: a.apiClient, codespaceName: "codespace-name"} + + return a.UpdatePortVisibility(ctx, selector, portArgs) } func TestPendingOperationDisallowsListPorts(t *testing.T) { app := testingPortsApp() + selector := &CodespaceSelector{api: app.apiClient, codespaceName: "disabledCodespace"} - if err := app.ListPorts(context.Background(), "disabledCodespace", nil); err != nil { + if err := app.ListPorts(context.Background(), selector, nil); err != nil { if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } @@ -224,8 +227,9 @@ func TestPendingOperationDisallowsListPorts(t *testing.T) { func TestPendingOperationDisallowsUpdatePortVisability(t *testing.T) { app := testingPortsApp() + selector := &CodespaceSelector{api: app.apiClient, codespaceName: "disabledCodespace"} - if err := app.UpdatePortVisibility(context.Background(), "disabledCodespace", nil); err != nil { + if err := app.UpdatePortVisibility(context.Background(), selector, nil); err != nil { if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } @@ -236,8 +240,9 @@ func TestPendingOperationDisallowsUpdatePortVisability(t *testing.T) { func TestPendingOperationDisallowsForwardPorts(t *testing.T) { app := testingPortsApp() + selector := &CodespaceSelector{api: app.apiClient, codespaceName: "disabledCodespace"} - if err := app.ForwardPorts(context.Background(), "disabledCodespace", nil); err != nil { + if err := app.ForwardPorts(context.Background(), selector, nil); err != nil { if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } diff --git a/pkg/cmd/codespace/rebuild.go b/pkg/cmd/codespace/rebuild.go index 923f471c4..17e00670b 100644 --- a/pkg/cmd/codespace/rebuild.go +++ b/pkg/cmd/codespace/rebuild.go @@ -10,8 +10,10 @@ import ( ) func newRebuildCmd(app *App) *cobra.Command { - var codespace string - var fullRebuild bool + var ( + selector *CodespaceSelector + fullRebuild bool + ) rebuildCmd := &cobra.Command{ Use: "rebuild", @@ -21,21 +23,22 @@ preserved. Your codespace will be rebuilt using your working directory's dev container. A full rebuild also removes cached Docker images.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - return app.Rebuild(cmd.Context(), codespace, fullRebuild) + return app.Rebuild(cmd.Context(), selector, fullRebuild) }, } - rebuildCmd.Flags().StringVarP(&codespace, "codespace", "c", "", "name of the codespace") + selector = AddCodespaceSelector(rebuildCmd, app.apiClient) + rebuildCmd.Flags().BoolVar(&fullRebuild, "full", false, "perform a full rebuild") return rebuildCmd } -func (a *App) Rebuild(ctx context.Context, codespaceName string, full bool) (err error) { +func (a *App) Rebuild(ctx context.Context, selector *CodespaceSelector, full bool) (err error) { ctx, cancel := context.WithCancel(ctx) defer cancel() - codespace, err := getOrChooseCodespace(ctx, a.apiClient, codespaceName) + codespace, err := selector.Select(ctx) if err != nil { return err } diff --git a/pkg/cmd/codespace/rebuild_test.go b/pkg/cmd/codespace/rebuild_test.go index f2496d089..b38bababe 100644 --- a/pkg/cmd/codespace/rebuild_test.go +++ b/pkg/cmd/codespace/rebuild_test.go @@ -14,8 +14,9 @@ func TestAlreadyRebuildingCodespace(t *testing.T) { State: api.CodespaceStateRebuilding, } app := testingRebuildApp(*rebuildingCodespace) + selector := &CodespaceSelector{api: app.apiClient, codespaceName: "rebuildingCodespace"} - err := app.Rebuild(context.Background(), "rebuildingCodespace", false) + err := app.Rebuild(context.Background(), selector, false) if err != nil { t.Errorf("rebuilding a codespace that was already rebuilding: %v", err) } diff --git a/pkg/cmd/codespace/select.go b/pkg/cmd/codespace/select.go index 32cc277b2..cb6a4128e 100644 --- a/pkg/cmd/codespace/select.go +++ b/pkg/cmd/codespace/select.go @@ -10,10 +10,13 @@ import ( type selectOptions struct { filePath string + selector *CodespaceSelector } func newSelectCmd(app *App) *cobra.Command { - opts := selectOptions{} + var ( + opts selectOptions + ) selectCmd := &cobra.Command{ Use: "select", @@ -21,19 +24,39 @@ func newSelectCmd(app *App) *cobra.Command { Hidden: true, Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { - return app.Select(cmd.Context(), "", opts) + return app.Select(cmd.Context(), opts) }, } + opts.selector = AddCodespaceSelector(selectCmd, app.apiClient) selectCmd.Flags().StringVarP(&opts.filePath, "file", "f", "", "Output file path") return selectCmd } -// Hidden codespace select command allows to reuse existing codespace selection -// dialog by external GH CLI extensions. By default, print selected codespace name -// to stdout. Pass file argument to save result into a file instead. -func (a *App) Select(ctx context.Context, name string, opts selectOptions) (err error) { - codespace, err := getOrChooseCodespace(ctx, a.apiClient, name) +// Hidden codespace `select` command allows to reuse existing codespace selection +// dialog by external GH CLI extensions. By default output selected codespace name +// into `stdout`. Pass `--file`(`-f`) flag along with a file path to output selected +// codespace name into a file instead. +// +// ## Examples +// +// With `stdout` output: +// +// ```shell +// +// gh codespace select +// +// ``` +// +// With `into-a-file` output: +// +// ```shell +// +// gh codespace select --file /tmp/selected_codespace.txt +// +// ``` +func (a *App) Select(ctx context.Context, opts selectOptions) (err error) { + codespace, err := opts.selector.Select(ctx) if err != nil { return err } diff --git a/pkg/cmd/codespace/select_test.go b/pkg/cmd/codespace/select_test.go index e97a7720e..02ea6f967 100644 --- a/pkg/cmd/codespace/select_test.go +++ b/pkg/cmd/codespace/select_test.go @@ -50,6 +50,7 @@ func TestApp_Select(t *testing.T) { a := NewApp(ios, nil, testSelectApiMock(), nil, nil) opts := selectOptions{} + if tt.outputToFile { file, err := os.CreateTemp("", "codespace-selection-test") if err != nil { @@ -61,7 +62,9 @@ func TestApp_Select(t *testing.T) { opts = selectOptions{filePath: file.Name()} } - if err := a.Select(context.Background(), tt.arg, opts); (err != nil) != tt.wantErr { + opts.selector = &CodespaceSelector{api: a.apiClient, codespaceName: tt.arg} + + if err := a.Select(context.Background(), opts); (err != nil) != tt.wantErr { t.Errorf("App.Select() error = %v, wantErr %v", err, tt.wantErr) } diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index d8788b45a..3295956f7 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -36,7 +36,7 @@ const automaticPrivateKeyName = "codespaces.auto" var errKeyFileNotFound = errors.New("SSH key file does not exist") type sshOptions struct { - codespace string + selector *CodespaceSelector profile string serverPort int debug bool @@ -87,7 +87,7 @@ func newSSHCmd(app *App) *cobra.Command { `), PreRunE: func(c *cobra.Command, args []string) error { if opts.stdio { - if opts.codespace == "" { + if opts.selector.codespaceName == "" { return errors.New("`--stdio` requires explicit `--codespace`") } if opts.config { @@ -122,7 +122,7 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().StringVarP(&opts.profile, "profile", "", "", "Name of the SSH profile to use") 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") + opts.selector = AddCodespaceSelector(sshCmd, app.apiClient) 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") sshCmd.Flags().BoolVarP(&opts.config, "config", "", false, "Write OpenSSH configuration to stdout") @@ -160,7 +160,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e args = append([]string{"-i", keyPair.PrivateKeyPath}, args...) } - codespace, err := getOrChooseCodespace(ctx, a.apiClient, opts.codespace) + codespace, err := opts.selector.Select(ctx) if err != nil { return err } @@ -471,13 +471,13 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro defer cancel() var csList []*api.Codespace - if opts.codespace == "" { + if opts.selector.codespaceName == "" { a.StartProgressIndicatorWithLabel("Fetching codespaces") csList, err = a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{}) a.StopProgressIndicator() } else { var codespace *api.Codespace - codespace, err = getOrChooseCodespace(ctx, a.apiClient, opts.codespace) + codespace, err = opts.selector.Select(ctx) csList = []*api.Codespace{codespace} } if err != nil { @@ -494,7 +494,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err erro var wg sync.WaitGroup var status error for _, cs := range csList { - if cs.State != "Available" && opts.codespace == "" { + if cs.State != "Available" && opts.selector.codespaceName == "" { fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) status = cmdutil.SilentError continue @@ -656,7 +656,7 @@ func newCpCmd(app *App) *cobra.Command { // We don't expose all sshOptions. cpCmd.Flags().BoolVarP(&opts.recursive, "recursive", "r", false, "Recursively copy directories") cpCmd.Flags().BoolVarP(&opts.expand, "expand", "e", false, "Expand remote file names on remote shell") - cpCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") + opts.selector = AddCodespaceSelector(cpCmd, app.apiClient) cpCmd.Flags().StringVarP(&opts.profile, "profile", "p", "", "Name of the SSH profile to use") return cpCmd } diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index 1740b00f7..b76d304d7 100644 --- a/pkg/cmd/codespace/ssh_test.go +++ b/pkg/cmd/codespace/ssh_test.go @@ -15,8 +15,9 @@ import ( func TestPendingOperationDisallowsSSH(t *testing.T) { app := testingSSHApp() + selector := &CodespaceSelector{api: app.apiClient, codespaceName: "disabledCodespace"} - if err := app.SSH(context.Background(), []string{}, sshOptions{codespace: "disabledCodespace"}); err != nil { + if err := app.SSH(context.Background(), []string{}, sshOptions{selector: selector}); err != nil { if err.Error() != "codespace is disabled while it has a pending operation: Some pending operation" { t.Errorf("expected pending operation error, but got: %v", err) } diff --git a/pkg/cmd/codespace/stop.go b/pkg/cmd/codespace/stop.go index bc60e9dc1..1deb27bfa 100644 --- a/pkg/cmd/codespace/stop.go +++ b/pkg/cmd/codespace/stop.go @@ -11,9 +11,9 @@ import ( ) type stopOptions struct { - codespaceName string - orgName string - userName string + selector *CodespaceSelector + orgName string + userName string } func newStopCmd(app *App) *cobra.Command { @@ -24,13 +24,13 @@ func newStopCmd(app *App) *cobra.Command { Short: "Stop a running codespace", Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { - if opts.orgName != "" && opts.codespaceName != "" && opts.userName == "" { + if opts.orgName != "" && opts.selector.codespaceName != "" && opts.userName == "" { return cmdutil.FlagErrorf("using `--org` with `--codespace` requires `--user`") } return app.StopCodespace(cmd.Context(), opts) }, } - stopCmd.Flags().StringVarP(&opts.codespaceName, "codespace", "c", "", "Name of the codespace") + opts.selector = AddCodespaceSelector(stopCmd, app.apiClient) stopCmd.Flags().StringVarP(&opts.orgName, "org", "o", "", "The `login` handle of the organization (admin-only)") stopCmd.Flags().StringVarP(&opts.userName, "user", "u", "", "The `username` to stop codespace for (used with --org)") @@ -38,12 +38,19 @@ func newStopCmd(app *App) *cobra.Command { } func (a *App) StopCodespace(ctx context.Context, opts *stopOptions) error { - codespaceName := opts.codespaceName - ownerName := opts.userName + var ( + codespaceName = opts.selector.codespaceName + repoName = opts.selector.repoName + ownerName = opts.userName + ) if codespaceName == "" { a.StartProgressIndicatorWithLabel("Fetching codespaces") - codespaces, err := a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{OrgName: opts.orgName, UserName: ownerName}) + codespaces, err := a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{ + RepoName: repoName, + OrgName: opts.orgName, + UserName: ownerName, + }) a.StopProgressIndicator() if err != nil { return fmt.Errorf("failed to list codespaces: %w", err) @@ -61,7 +68,8 @@ func (a *App) StopCodespace(ctx context.Context, opts *stopOptions) error { } includeOwner := opts.orgName != "" - codespace, err := chooseCodespaceFromList(ctx, runningCodespaces, includeOwner) + skipPromptForSingleOption := repoName != "" + codespace, err := chooseCodespaceFromList(ctx, runningCodespaces, includeOwner, skipPromptForSingleOption) if err != nil { return fmt.Errorf("failed to choose codespace: %w", err) } diff --git a/pkg/cmd/codespace/stop_test.go b/pkg/cmd/codespace/stop_test.go index bdd3721c0..78e07fcec 100644 --- a/pkg/cmd/codespace/stop_test.go +++ b/pkg/cmd/codespace/stop_test.go @@ -22,7 +22,7 @@ func TestApp_StopCodespace(t *testing.T) { { name: "Stop a codespace I own", opts: &stopOptions{ - codespaceName: "test-codespace", + selector: &CodespaceSelector{codespaceName: "test-codespace"}, }, fields: fields{ apiClient: &apiClientMock{ @@ -52,9 +52,9 @@ func TestApp_StopCodespace(t *testing.T) { { name: "Stop a codespace as an org admin", opts: &stopOptions{ - codespaceName: "test-codespace", - orgName: "test-org", - userName: "test-user", + selector: &CodespaceSelector{codespaceName: "test-codespace"}, + orgName: "test-org", + userName: "test-user", }, fields: fields{ apiClient: &apiClientMock{