From 590acaa1d666543b93a3c6adb81e50dcf722ac92 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Tue, 15 Feb 2022 19:35:53 -0500 Subject: [PATCH 1/6] WIP --- internal/codespaces/api/api.go | 26 +++++++++++++++++++++++ pkg/cmd/codespace/code.go | 13 +++--------- pkg/cmd/codespace/code_test.go | 6 ++++-- pkg/cmd/codespace/common.go | 8 ++++++- pkg/cmd/codespace/create.go | 36 +++++++++++++++++++++++++++++++- pkg/cmd/codespace/delete_test.go | 2 +- pkg/cmd/root/root.go | 1 + 7 files changed, 77 insertions(+), 15 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index a207f562c..63f86c931 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -614,6 +614,15 @@ type startCreateRequest struct { var errProvisioningInProgress = errors.New("provisioning in progress") +type AcceptPermissionsRequiredError struct { + Message string `json:"message"` + AllowPermissionsURL string `json:"allow_permissions_url"` +} + +func (e AcceptPermissionsRequiredError) Error() string { + return e.Message +} + // startCreate starts the creation of a codespace. // It may return success or an error, or errProvisioningInProgress indicating that the operation // did not complete before the GitHub API's time limit for RPCs (10s), in which case the caller @@ -648,6 +657,23 @@ func (a *API) startCreate(ctx context.Context, params *CreateCodespaceParams) (* if resp.StatusCode == http.StatusAccepted { return nil, errProvisioningInProgress // RPC finished before result of creation known + } else if resp.StatusCode == http.StatusUnauthorized { + var ue AcceptPermissionsRequiredError + + b, err := ioutil.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("error reading response body: %w", err) + } + if err := json.Unmarshal(b, &ue); err != nil { + return nil, fmt.Errorf("error unmarshaling response: %w", err) + } + + if ue.AllowPermissionsURL != "" { + return nil, ue + } + + return nil, api.HandleHTTPError(resp) + } else if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/codespace/code.go b/pkg/cmd/codespace/code.go index 2c66b4655..f80e32527 100644 --- a/pkg/cmd/codespace/code.go +++ b/pkg/cmd/codespace/code.go @@ -3,17 +3,11 @@ package codespace import ( "context" "fmt" - "io/ioutil" "net/url" - "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) -type browser interface { - Browse(string) error -} - func newCodeCmd(app *App) *cobra.Command { var ( codespace string @@ -25,8 +19,7 @@ func newCodeCmd(app *App) *cobra.Command { Short: "Open a codespace in Visual Studio Code", Args: noArgsConstraint, RunE: func(cmd *cobra.Command, args []string) error { - b := cmdutil.NewBrowser("", ioutil.Discard, app.io.ErrOut) - return app.VSCode(cmd.Context(), b, codespace, useInsiders) + return app.VSCode(cmd.Context(), codespace, useInsiders) }, } @@ -37,7 +30,7 @@ func newCodeCmd(app *App) *cobra.Command { } // VSCode opens a codespace in the local VS VSCode application. -func (a *App) VSCode(ctx context.Context, browser browser, codespaceName string, useInsiders bool) error { +func (a *App) VSCode(ctx context.Context, codespaceName string, useInsiders bool) error { if codespaceName == "" { codespace, err := chooseCodespace(ctx, a.apiClient) if err != nil { @@ -50,7 +43,7 @@ func (a *App) VSCode(ctx context.Context, browser browser, codespaceName string, } url := vscodeProtocolURL(codespaceName, useInsiders) - if err := browser.Browse(url); err != nil { + if err := a.browser.Browse(url); err != nil { return fmt.Errorf("error opening Visual Studio Code: %w", err) } diff --git a/pkg/cmd/codespace/code_test.go b/pkg/cmd/codespace/code_test.go index 956219bb2..cbc59864a 100644 --- a/pkg/cmd/codespace/code_test.go +++ b/pkg/cmd/codespace/code_test.go @@ -40,8 +40,10 @@ func TestApp_VSCode(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { b := &cmdutil.TestBrowser{} - a := &App{} - if err := a.VSCode(context.Background(), b, tt.args.codespaceName, tt.args.useInsiders); (err != nil) != tt.wantErr { + a := &App{ + browser: b, + } + if err := a.VSCode(context.Background(), tt.args.codespaceName, tt.args.useInsiders); (err != nil) != tt.wantErr { t.Errorf("App.VSCode() error = %v, wantErr %v", err, tt.wantErr) } b.Verify(t, tt.wantURL) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index d7273edb8..d21f9ff6c 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -21,6 +21,10 @@ import ( "golang.org/x/term" ) +type browser interface { + Browse(string) error +} + type executable interface { Executable() string } @@ -30,9 +34,10 @@ type App struct { apiClient apiClient errLogger *log.Logger executable executable + browser browser } -func NewApp(io *iostreams.IOStreams, exe executable, apiClient apiClient) *App { +func NewApp(io *iostreams.IOStreams, exe executable, apiClient apiClient, browser browser) *App { errLogger := log.New(io.ErrOut, "", 0) return &App{ @@ -40,6 +45,7 @@ func NewApp(io *iostreams.IOStreams, exe executable, apiClient apiClient) *App { apiClient: apiClient, errLogger: errLogger, executable: exe, + browser: browser, } } diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index ab04c2bae..97a292067 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -1,14 +1,17 @@ package codespace import ( + "bufio" "context" "errors" "fmt" + "io" "time" "github.com/AlecAivazis/survey/v2" "github.com/cli/cli/v2/internal/codespaces" "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/cli/cli/v2/utils" "github.com/spf13/cobra" ) @@ -117,7 +120,32 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { IdleTimeoutMinutes: int(opts.idleTimeout.Minutes()), }) a.StopProgressIndicator() + + out := a.io.Out + if err != nil { + var aerr api.AcceptPermissionsRequiredError + if errors.As(err, &aerr) && aerr.AllowPermissionsURL != "" { + + var ( + isInteractive = a.io.CanPrompt() + cs = a.io.ColorScheme() + displayURL = utils.DisplayURL(aerr.AllowPermissionsURL) + ) + + if !isInteractive { + fmt.Fprintf(out, "%s to continue in your web browser: %s\n", cs.Bold("Open this URL"), displayURL) + return nil + } + + fmt.Fprintf(out, "You must accept the permissions requested by the repository before you can create a codespace.\n") + fmt.Fprintf(out, "%s to open %s in your browser... ", cs.Bold("Press Enter"), displayURL) + _ = waitForEnter(a.io.In) + + if err := a.browser.Browse(aerr.AllowPermissionsURL); err != nil { + return fmt.Errorf("error opening browser: %w", err) + } + } return fmt.Errorf("error creating codespace: %w", err) } @@ -127,7 +155,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } } - fmt.Fprintln(a.io.Out, codespace.Name) + fmt.Fprintln(out, codespace.Name) return nil } @@ -296,3 +324,9 @@ func buildDisplayName(displayName string, prebuildAvailability string) string { return fmt.Sprintf("%s%s", displayName, prebuildText) } + +func waitForEnter(r io.Reader) error { + scanner := bufio.NewScanner(r) + scanner.Scan() + return scanner.Err() +} diff --git a/pkg/cmd/codespace/delete_test.go b/pkg/cmd/codespace/delete_test.go index 89318b5e3..638641d64 100644 --- a/pkg/cmd/codespace/delete_test.go +++ b/pkg/cmd/codespace/delete_test.go @@ -190,7 +190,7 @@ func TestDelete(t *testing.T) { io, _, stdout, stderr := iostreams.Test() io.SetStdinTTY(true) io.SetStdoutTTY(true) - app := NewApp(io, nil, apiMock) + app := NewApp(io, nil, apiMock, nil) err := app.Delete(context.Background(), opts) if (err != nil) != tt.wantErr { t.Errorf("delete() error = %v, wantErr %v", err, tt.wantErr) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index cff797e93..68ba157ff 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -142,6 +142,7 @@ func newCodespaceCmd(f *cmdutil.Factory) *cobra.Command { vscsURL, &lazyLoadedHTTPClient{factory: f}, ), + f.Browser, ) cmd := codespaceCmd.NewRootCmd(app) cmd.Use = "codespace" From e7c2f973ae828d05cead21a5f9be3c4f3b9c8db6 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Thu, 17 Feb 2022 15:11:12 -0500 Subject: [PATCH 2/6] Support opting out of permissions --- internal/codespaces/api/api.go | 3 ++ pkg/cmd/codespace/create.go | 92 ++++++++++++++++++++++++++-------- 2 files changed, 73 insertions(+), 22 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 63f86c931..aae1c08be 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -565,6 +565,7 @@ type CreateCodespaceParams struct { Branch string Machine string Location string + PermissionsOptOut bool } // CreateCodespace creates a codespace with the given parameters and returns a non-nil error if it @@ -610,6 +611,7 @@ type startCreateRequest struct { Ref string `json:"ref"` Location string `json:"location"` Machine string `json:"machine"` + PermissionsOptOut bool `json:"devcontainer_permissions_opt_out"` } var errProvisioningInProgress = errors.New("provisioning in progress") @@ -638,6 +640,7 @@ func (a *API) startCreate(ctx context.Context, params *CreateCodespaceParams) (* Ref: params.Branch, Location: params.Location, Machine: params.Machine, + PermissionsOptOut: params.PermissionsOptOut, }) if err != nil { return nil, fmt.Errorf("error marshaling request: %w", err) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 97a292067..32fb4b77a 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -16,11 +16,12 @@ import ( ) type createOptions struct { - repo string - branch string - machine string - showStatus bool - idleTimeout time.Duration + repo string + branch string + machine string + showStatus bool + permissionsOptOut bool + idleTimeout time.Duration } func newCreateCmd(app *App) *cobra.Command { @@ -38,6 +39,7 @@ func newCreateCmd(app *App) *cobra.Command { createCmd.Flags().StringVarP(&opts.repo, "repo", "r", "", "repository name with owner: user/repo") createCmd.Flags().StringVarP(&opts.branch, "branch", "b", "", "repository branch") createCmd.Flags().StringVarP(&opts.machine, "machine", "m", "", "hardware specifications for the VM") + createCmd.Flags().BoolVarP(&opts.permissionsOptOut, "skip-permissions", "", false, "do not accept additional permissions requested by the codespace") createCmd.Flags().BoolVarP(&opts.showStatus, "status", "s", false, "show status of post-create command and dotfiles") createCmd.Flags().DurationVar(&opts.idleTimeout, "idle-timeout", 0, "allowed inactivity before codespace is stopped, e.g. \"10m\", \"1h\"") @@ -118,35 +120,81 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { Machine: machine, Location: locationResult.Location, IdleTimeoutMinutes: int(opts.idleTimeout.Minutes()), + PermissionsOptOut: opts.permissionsOptOut, }) a.StopProgressIndicator() - out := a.io.Out - if err != nil { var aerr api.AcceptPermissionsRequiredError - if errors.As(err, &aerr) && aerr.AllowPermissionsURL != "" { + if !errors.As(err, &aerr) || aerr.AllowPermissionsURL == "" { + return fmt.Errorf("error creating codespace: %w", err) + } - var ( - isInteractive = a.io.CanPrompt() - cs = a.io.ColorScheme() - displayURL = utils.DisplayURL(aerr.AllowPermissionsURL) - ) + var ( + isInteractive = a.io.CanPrompt() + cs = a.io.ColorScheme() + displayURL = utils.DisplayURL(aerr.AllowPermissionsURL) + ) - if !isInteractive { - fmt.Fprintf(out, "%s to continue in your web browser: %s\n", cs.Bold("Open this URL"), displayURL) - return nil - } + fmt.Fprintf(a.io.Out, "You must accept or deny additional permissions requested by the repository before you can create a codespace.\n") - fmt.Fprintf(out, "You must accept the permissions requested by the repository before you can create a codespace.\n") - fmt.Fprintf(out, "%s to open %s in your browser... ", cs.Bold("Press Enter"), displayURL) - _ = waitForEnter(a.io.In) + if !isInteractive { + fmt.Fprintf(a.io.Out, "%s to continue in your web browser to accept: %s\n", cs.Bold("Open this URL"), displayURL) + fmt.Fprintf(a.io.Out, "Alternatively, you can run %q with the %q option to create the codespace without these additional permissions.\n", a.io.ColorScheme().Bold("create"), cs.Bold("--skip-permissions")) + return nil + } + choices := []string{ + "Continue in browser to accept the additional permissions", + "Create the codespace without these additional permissions", + } + + permsSurvey := []*survey.Question{ + { + Name: "accept", + Prompt: &survey.Select{ + Message: "What would you like to do?", + Options: choices, + Default: choices[0], + }, + Validate: survey.Required, + }, + } + + var answers struct { + Accept string + } + + if err := ask(permsSurvey, &answers); err != nil { + return fmt.Errorf("error getting answers: %w", err) + } + + // if the user chose to continue in the browser, open the URL + if answers.Accept == choices[0] { if err := a.browser.Browse(aerr.AllowPermissionsURL); err != nil { return fmt.Errorf("error opening browser: %w", err) } + // browser opened successfully but we do not know if they accepted the permissions + // so we must exit and wait for the user to attempt the create again + return nil + } + + // if the user chose to create the codespace without the permissions, + // we can continue with the create opting out of the additional permissions + a.StartProgressIndicatorWithLabel("Creating codespace") + codespace, err = a.apiClient.CreateCodespace(ctx, &api.CreateCodespaceParams{ + RepositoryID: repository.ID, + Branch: branch, + Machine: machine, + Location: locationResult.Location, + IdleTimeoutMinutes: int(opts.idleTimeout.Minutes()), + PermissionsOptOut: true, + }) + a.StopProgressIndicator() + + if err != nil { + return fmt.Errorf("error creating codespace: %w", err) } - return fmt.Errorf("error creating codespace: %w", err) } if opts.showStatus { @@ -155,7 +203,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { } } - fmt.Fprintln(out, codespace.Name) + fmt.Fprintln(a.io.Out, codespace.Name) return nil } From 1ea26f33d8eeefaa8924c934ce771ae5e4ca896c Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Tue, 22 Feb 2022 09:50:43 -0500 Subject: [PATCH 3/6] Add create test --- pkg/cmd/codespace/create_test.go | 50 ++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 4bf4671ea..795552284 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -69,6 +69,56 @@ func TestApp_Create(t *testing.T) { }, wantStdout: "monalisa-dotfiles-abcd1234\n", }, + { + name: "create codespace that requires accepting additional permissions", + fields: fields{ + apiClient: &apiClientMock{ + GetCodespaceRegionLocationFunc: func(ctx context.Context) (string, error) { + return "EUROPE", nil + }, + GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { + return &api.Repository{ + ID: 1234, + FullName: nwo, + DefaultBranch: "main", + }, nil + }, + GetCodespacesMachinesFunc: func(ctx context.Context, repoID int, branch, location string) ([]*api.Machine, error) { + return []*api.Machine{ + { + Name: "GIGA", + DisplayName: "Gigabits of a machine", + }, + }, nil + }, + CreateCodespaceFunc: func(ctx context.Context, params *api.CreateCodespaceParams) (*api.Codespace, error) { + if params.Branch != "main" { + return nil, fmt.Errorf("got branch %q, want %q", params.Branch, "main") + } + if params.IdleTimeoutMinutes != 30 { + return nil, fmt.Errorf("idle timeout minutes was %v", params.IdleTimeoutMinutes) + } + return &api.Codespace{}, api.AcceptPermissionsRequiredError{ + AllowPermissionsURL: "https://example.com/permissions", + } + }, + GetCodespaceRepoSuggestionsFunc: func(ctx context.Context, partialSearch string, params api.RepoSearchParameters) ([]string, error) { + return nil, nil // We can't ask for suggestions without a terminal. + }, + }, + }, + opts: createOptions{ + repo: "monalisa/dotfiles", + branch: "", + machine: "GIGA", + showStatus: false, + idleTimeout: 30 * time.Minute, + }, + wantStdout: `You must accept or deny additional permissions requested by the repository before you can create a codespace. +Open this URL to continue in your web browser to accept: example.com/permissions +Alternatively, you can run "create" with the "--skip-permissions" option to create the codespace without these additional permissions. +`, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 707220a9b1f045d1f76447b1d18de0c73d10ec83 Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Tue, 22 Feb 2022 14:16:03 -0500 Subject: [PATCH 4/6] PR updates --- internal/codespaces/api/api.go | 10 ++- pkg/cmd/codespace/create.go | 114 +++++++++++++++++---------------- 2 files changed, 66 insertions(+), 58 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index aae1c08be..14da851ae 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -661,9 +661,13 @@ func (a *API) startCreate(ctx context.Context, params *CreateCodespaceParams) (* if resp.StatusCode == http.StatusAccepted { return nil, errProvisioningInProgress // RPC finished before result of creation known } else if resp.StatusCode == http.StatusUnauthorized { - var ue AcceptPermissionsRequiredError + var ( + ue AcceptPermissionsRequiredError + bodyCopy = &bytes.Buffer{} + r = io.TeeReader(resp.Body, bodyCopy) + ) - b, err := ioutil.ReadAll(resp.Body) + b, err := ioutil.ReadAll(r) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } @@ -675,6 +679,8 @@ func (a *API) startCreate(ctx context.Context, params *CreateCodespaceParams) (* return nil, ue } + resp.Body = ioutil.NopCloser(bodyCopy) + return nil, api.HandleHTTPError(resp) } else if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 32fb4b77a..20952bb6d 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -1,16 +1,15 @@ package codespace import ( - "bufio" "context" "errors" "fmt" - "io" "time" "github.com/AlecAivazis/survey/v2" "github.com/cli/cli/v2/internal/codespaces" "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/utils" "github.com/spf13/cobra" ) @@ -39,7 +38,7 @@ func newCreateCmd(app *App) *cobra.Command { createCmd.Flags().StringVarP(&opts.repo, "repo", "r", "", "repository name with owner: user/repo") createCmd.Flags().StringVarP(&opts.branch, "branch", "b", "", "repository branch") createCmd.Flags().StringVarP(&opts.machine, "machine", "m", "", "hardware specifications for the VM") - createCmd.Flags().BoolVarP(&opts.permissionsOptOut, "skip-permissions", "", false, "do not accept additional permissions requested by the codespace") + createCmd.Flags().BoolVarP(&opts.permissionsOptOut, "default-permissions", "", false, "do not prompt to accept additional permissions requested by the codespace") createCmd.Flags().BoolVarP(&opts.showStatus, "status", "s", false, "show status of post-create command and dotfiles") createCmd.Flags().DurationVar(&opts.idleTimeout, "idle-timeout", 0, "allowed inactivity before codespace is stopped, e.g. \"10m\", \"1h\"") @@ -130,53 +129,9 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { return fmt.Errorf("error creating codespace: %w", err) } - var ( - isInteractive = a.io.CanPrompt() - cs = a.io.ColorScheme() - displayURL = utils.DisplayURL(aerr.AllowPermissionsURL) - ) - - fmt.Fprintf(a.io.Out, "You must accept or deny additional permissions requested by the repository before you can create a codespace.\n") - - if !isInteractive { - fmt.Fprintf(a.io.Out, "%s to continue in your web browser to accept: %s\n", cs.Bold("Open this URL"), displayURL) - fmt.Fprintf(a.io.Out, "Alternatively, you can run %q with the %q option to create the codespace without these additional permissions.\n", a.io.ColorScheme().Bold("create"), cs.Bold("--skip-permissions")) - return nil - } - - choices := []string{ - "Continue in browser to accept the additional permissions", - "Create the codespace without these additional permissions", - } - - permsSurvey := []*survey.Question{ - { - Name: "accept", - Prompt: &survey.Select{ - Message: "What would you like to do?", - Options: choices, - Default: choices[0], - }, - Validate: survey.Required, - }, - } - - var answers struct { - Accept string - } - - if err := ask(permsSurvey, &answers); err != nil { - return fmt.Errorf("error getting answers: %w", err) - } - - // if the user chose to continue in the browser, open the URL - if answers.Accept == choices[0] { - if err := a.browser.Browse(aerr.AllowPermissionsURL); err != nil { - return fmt.Errorf("error opening browser: %w", err) - } - // browser opened successfully but we do not know if they accepted the permissions - // so we must exit and wait for the user to attempt the create again - return nil + if err := a.handleAdditionalPermissions(ctx, aerr.AllowPermissionsURL); err != nil { + // this error could be a cmdutil.SilentError (in the case that the user opened the browser) so we don't want to wrap it + return err } // if the user chose to create the codespace without the permissions, @@ -207,6 +162,59 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { return nil } +func (a *App) handleAdditionalPermissions(ctx context.Context, allowPermissionsURL string) error { + var ( + isInteractive = a.io.CanPrompt() + cs = a.io.ColorScheme() + displayURL = utils.DisplayURL(allowPermissionsURL) + ) + + fmt.Fprintf(a.io.ErrOut, "You must accept or deny additional permissions requested by the repository before you can create a codespace.\n") + + if !isInteractive { + fmt.Fprintf(a.io.ErrOut, "%s to continue in your web browser to accept: %s\n", cs.Bold("Open this URL"), displayURL) + fmt.Fprintf(a.io.ErrOut, "Alternatively, you can run %q with the %q option to create the codespace without these additional permissions.\n", a.io.ColorScheme().Bold("create"), cs.Bold("--default-permissions")) + return nil + } + + choices := []string{ + "Continue in browser to accept the additional permissions", + "Create the codespace without these additional permissions", + } + + permsSurvey := []*survey.Question{ + { + Name: "accept", + Prompt: &survey.Select{ + Message: "What would you like to do?", + Options: choices, + Default: choices[0], + }, + Validate: survey.Required, + }, + } + + var answers struct { + Accept string + } + + if err := ask(permsSurvey, &answers); err != nil { + return fmt.Errorf("error getting answers: %w", err) + } + + // if the user chose to continue in the browser, open the URL + if answers.Accept == choices[0] { + if err := a.browser.Browse(allowPermissionsURL); err != nil { + return fmt.Errorf("error opening browser: %w", err) + } + // browser opened successfully but we do not know if they accepted the permissions + // so we must exit and wait for the user to attempt the create again + return cmdutil.SilentError + } + + return nil +} + // showStatus polls the codespace for a list of post create states and their status. It will keep polling // until all states have finished. Once all states have finished, we poll once more to check if any new // states have been introduced and stop polling otherwise. @@ -372,9 +380,3 @@ func buildDisplayName(displayName string, prebuildAvailability string) string { return fmt.Sprintf("%s%s", displayName, prebuildText) } - -func waitForEnter(r io.Reader) error { - scanner := bufio.NewScanner(r) - scanner.Scan() - return scanner.Err() -} From 71f32376d09d1d8330c72dec2e15982797232bce Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Tue, 22 Feb 2022 20:20:52 -0500 Subject: [PATCH 5/6] Fix tests --- pkg/cmd/codespace/create.go | 52 +++++++++++++++----------------- pkg/cmd/codespace/create_test.go | 10 +++--- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 20952bb6d..3f2ee0c35 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -112,15 +112,17 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { return errors.New("there are no available machine types for this repository") } - a.StartProgressIndicatorWithLabel("Creating codespace") - codespace, err := a.apiClient.CreateCodespace(ctx, &api.CreateCodespaceParams{ + createParams := &api.CreateCodespaceParams{ RepositoryID: repository.ID, Branch: branch, Machine: machine, Location: locationResult.Location, IdleTimeoutMinutes: int(opts.idleTimeout.Minutes()), PermissionsOptOut: opts.permissionsOptOut, - }) + } + + a.StartProgressIndicatorWithLabel("Creating codespace") + codespace, err := a.apiClient.CreateCodespace(ctx, createParams) a.StopProgressIndicator() if err != nil { @@ -129,27 +131,11 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { return fmt.Errorf("error creating codespace: %w", err) } - if err := a.handleAdditionalPermissions(ctx, aerr.AllowPermissionsURL); err != nil { + codespace, err = a.handleAdditionalPermissions(ctx, createParams, aerr.AllowPermissionsURL) + if err != nil { // this error could be a cmdutil.SilentError (in the case that the user opened the browser) so we don't want to wrap it return err } - - // if the user chose to create the codespace without the permissions, - // we can continue with the create opting out of the additional permissions - a.StartProgressIndicatorWithLabel("Creating codespace") - codespace, err = a.apiClient.CreateCodespace(ctx, &api.CreateCodespaceParams{ - RepositoryID: repository.ID, - Branch: branch, - Machine: machine, - Location: locationResult.Location, - IdleTimeoutMinutes: int(opts.idleTimeout.Minutes()), - PermissionsOptOut: true, - }) - a.StopProgressIndicator() - - if err != nil { - return fmt.Errorf("error creating codespace: %w", err) - } } if opts.showStatus { @@ -162,7 +148,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error { return nil } -func (a *App) handleAdditionalPermissions(ctx context.Context, allowPermissionsURL string) error { +func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api.CreateCodespaceParams, allowPermissionsURL string) (*api.Codespace, error) { var ( isInteractive = a.io.CanPrompt() cs = a.io.ColorScheme() @@ -174,7 +160,7 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, allowPermissionsU if !isInteractive { fmt.Fprintf(a.io.ErrOut, "%s to continue in your web browser to accept: %s\n", cs.Bold("Open this URL"), displayURL) fmt.Fprintf(a.io.ErrOut, "Alternatively, you can run %q with the %q option to create the codespace without these additional permissions.\n", a.io.ColorScheme().Bold("create"), cs.Bold("--default-permissions")) - return nil + return nil, cmdutil.SilentError } choices := []string{ @@ -199,20 +185,32 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, allowPermissionsU } if err := ask(permsSurvey, &answers); err != nil { - return fmt.Errorf("error getting answers: %w", err) + return nil, fmt.Errorf("error getting answers: %w", err) } // if the user chose to continue in the browser, open the URL if answers.Accept == choices[0] { if err := a.browser.Browse(allowPermissionsURL); err != nil { - return fmt.Errorf("error opening browser: %w", err) + return nil, fmt.Errorf("error opening browser: %w", err) } // browser opened successfully but we do not know if they accepted the permissions // so we must exit and wait for the user to attempt the create again - return cmdutil.SilentError + return nil, cmdutil.SilentError } - return nil + // if the user chose to create the codespace without the permissions, + // we can continue with the create opting out of the additional permissions + createParams.PermissionsOptOut = true + + a.StartProgressIndicatorWithLabel("Creating codespace") + codespace, err := a.apiClient.CreateCodespace(ctx, createParams) + a.StopProgressIndicator() + + if err != nil { + return nil, fmt.Errorf("error creating codespace: %w", err) + } + + return codespace, nil } // showStatus polls the codespace for a list of post create states and their status. It will keep polling diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 795552284..376f8c9eb 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/cli/cli/v2/internal/codespaces/api" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" ) @@ -18,7 +19,7 @@ func TestApp_Create(t *testing.T) { name string fields fields opts createOptions - wantErr bool + wantErr error wantStdout string wantStderr string }{ @@ -114,9 +115,10 @@ func TestApp_Create(t *testing.T) { showStatus: false, idleTimeout: 30 * time.Minute, }, - wantStdout: `You must accept or deny additional permissions requested by the repository before you can create a codespace. + wantErr: cmdutil.SilentError, + wantStderr: `You must accept or deny additional permissions requested by the repository before you can create a codespace. Open this URL to continue in your web browser to accept: example.com/permissions -Alternatively, you can run "create" with the "--skip-permissions" option to create the codespace without these additional permissions. +Alternatively, you can run "create" with the "--default-permissions" option to create the codespace without these additional permissions. `, }, } @@ -127,7 +129,7 @@ Alternatively, you can run "create" with the "--skip-permissions" option to crea io: io, apiClient: tt.fields.apiClient, } - if err := a.Create(context.Background(), tt.opts); (err != nil) != tt.wantErr { + if err := a.Create(context.Background(), tt.opts); err != tt.wantErr { t.Errorf("App.Create() error = %v, wantErr %v", err, tt.wantErr) } if got := stdout.String(); got != tt.wantStdout { From dd5feda00a2357275fb2cc2d1b4eb854e882faed Mon Sep 17 00:00:00 2001 From: Mark Phelps Date: Fri, 25 Feb 2022 17:32:08 -0500 Subject: [PATCH 6/6] Update wording after consulting product --- pkg/cmd/codespace/create.go | 10 +++++----- pkg/cmd/codespace/create_test.go | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go index 3f2ee0c35..b145a6a7d 100644 --- a/pkg/cmd/codespace/create.go +++ b/pkg/cmd/codespace/create.go @@ -155,17 +155,17 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api displayURL = utils.DisplayURL(allowPermissionsURL) ) - fmt.Fprintf(a.io.ErrOut, "You must accept or deny additional permissions requested by the repository before you can create a codespace.\n") + fmt.Fprintf(a.io.ErrOut, "You must authorize or deny additional permissions requested by this codespace before continuing.\n") if !isInteractive { - fmt.Fprintf(a.io.ErrOut, "%s to continue in your web browser to accept: %s\n", cs.Bold("Open this URL"), displayURL) - fmt.Fprintf(a.io.ErrOut, "Alternatively, you can run %q with the %q option to create the codespace without these additional permissions.\n", a.io.ColorScheme().Bold("create"), cs.Bold("--default-permissions")) + fmt.Fprintf(a.io.ErrOut, "%s in your browser to review and authorize additional permissions: %s\n", cs.Bold("Open this URL"), displayURL) + fmt.Fprintf(a.io.ErrOut, "Alternatively, you can run %q with the %q option to continue without authorizing additional permissions.\n", a.io.ColorScheme().Bold("create"), cs.Bold("--default-permissions")) return nil, cmdutil.SilentError } choices := []string{ - "Continue in browser to accept the additional permissions", - "Create the codespace without these additional permissions", + "Continue in browser to review and authorize additional permissions", + "Continue without authorizing additional permissions", } permsSurvey := []*survey.Question{ diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go index 376f8c9eb..2d417620d 100644 --- a/pkg/cmd/codespace/create_test.go +++ b/pkg/cmd/codespace/create_test.go @@ -116,9 +116,9 @@ func TestApp_Create(t *testing.T) { idleTimeout: 30 * time.Minute, }, wantErr: cmdutil.SilentError, - wantStderr: `You must accept or deny additional permissions requested by the repository before you can create a codespace. -Open this URL to continue in your web browser to accept: example.com/permissions -Alternatively, you can run "create" with the "--default-permissions" option to create the codespace without these additional permissions. + wantStderr: `You must authorize or deny additional permissions requested by this codespace before continuing. +Open this URL in your browser to review and authorize additional permissions: example.com/permissions +Alternatively, you can run "create" with the "--default-permissions" option to continue without authorizing additional permissions. `, }, }