From 168f09fb106ad4b4e61ff2aec06d21ec31436b1a Mon Sep 17 00:00:00 2001 From: nsmag Date: Thu, 15 Sep 2022 21:17:43 +0700 Subject: [PATCH 1/8] Add gh ssh-key delete --- pkg/cmd/ssh-key/delete/delete.go | 94 +++++++++++ pkg/cmd/ssh-key/delete/delete_test.go | 232 ++++++++++++++++++++++++++ pkg/cmd/ssh-key/delete/http.go | 29 ++++ pkg/cmd/ssh-key/ssh_key.go | 4 +- 4 files changed, 358 insertions(+), 1 deletion(-) create mode 100644 pkg/cmd/ssh-key/delete/delete.go create mode 100644 pkg/cmd/ssh-key/delete/delete_test.go create mode 100644 pkg/cmd/ssh-key/delete/http.go diff --git a/pkg/cmd/ssh-key/delete/delete.go b/pkg/cmd/ssh-key/delete/delete.go new file mode 100644 index 00000000000..55d1fa69648 --- /dev/null +++ b/pkg/cmd/ssh-key/delete/delete.go @@ -0,0 +1,94 @@ +package delete + +import ( + "errors" + "fmt" + "net/http" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +type DeleteOptions struct { + IO *iostreams.IOStreams + Config func() (config.Config, error) + HttpClient func() (*http.Client, error) + + KeyID string + Confirmed bool + Prompter prompter.Prompter +} + +func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Command { + opts := &DeleteOptions{ + HttpClient: f.HttpClient, + Config: f.Config, + IO: f.IOStreams, + Prompter: f.Prompter, + } + + cmd := &cobra.Command{ + Use: "delete ", + Short: "Delete an SSH key from your GitHub account", + Args: cmdutil.ExactArgs(1, "cannot delete: key id required"), + RunE: func(cmd *cobra.Command, args []string) error { + opts.KeyID = args[0] + + if !opts.IO.CanPrompt() && !opts.Confirmed { + return cmdutil.FlagErrorf("--confirm required when not running interactively") + } + + if runF != nil { + return runF(opts) + } + return deleteRun(opts) + }, + } + + cmd.Flags().BoolVarP(&opts.Confirmed, "confirm", "y", false, "Skip the confirmation prompt") + return cmd +} + +func deleteRun(opts *DeleteOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + cfg, err := opts.Config() + if err != nil { + return err + } + + if opts.IO.CanPrompt() && !opts.Confirmed { + confirmed, err := opts.Prompter.Confirm("Confirm deletion:", true) + if err != nil { + return err + } + if !confirmed { + return cmdutil.CancelError + } + } + + host, _ := cfg.DefaultHost() + + err = deleteSSHKey(httpClient, host, opts.KeyID) + if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { + return fmt.Errorf("unable to delete SSH key id %s: either the SSH key is not found or it is not owned by you", opts.KeyID) + } + + return err + } + + if opts.IO.IsStdoutTTY() { + cs := opts.IO.ColorScheme() + fmt.Fprintf(opts.IO.Out, "%s Deleted SSH key id %s from your account\n", cs.SuccessIcon(), opts.KeyID) + } + return nil +} diff --git a/pkg/cmd/ssh-key/delete/delete_test.go b/pkg/cmd/ssh-key/delete/delete_test.go new file mode 100644 index 00000000000..6fb3374fd8f --- /dev/null +++ b/pkg/cmd/ssh-key/delete/delete_test.go @@ -0,0 +1,232 @@ +package delete + +import ( + "bytes" + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func TestNewCmdDelete(t *testing.T) { + tests := []struct { + name string + tty bool + input string + output DeleteOptions + wantErr bool + wantErrMsg string + }{ + { + name: "tty", + tty: true, + input: "123", + output: DeleteOptions{KeyID: "123", Confirmed: false}, + }, + { + name: "confirm flag tty", + tty: true, + input: "123 --confirm", + output: DeleteOptions{KeyID: "123", Confirmed: true}, + }, + { + name: "shorthand confirm flag tty", + tty: true, + input: "123 -y", + output: DeleteOptions{KeyID: "123", Confirmed: true}, + }, + { + name: "no tty", + input: "123", + wantErr: true, + wantErrMsg: "--confirm required when not running interactively", + }, + { + name: "confirm flag no tty", + input: "123 --confirm", + output: DeleteOptions{KeyID: "123", Confirmed: true}, + }, + { + name: "shorthand confirm flag no tty", + input: "123 -y", + output: DeleteOptions{KeyID: "123", Confirmed: true}, + }, + { + name: "no args", + input: "", + wantErr: true, + wantErrMsg: "cannot delete: key id required", + }, + { + name: "too many args", + input: "123 456", + wantErr: true, + wantErrMsg: "too many arguments", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + ios.SetStdinTTY(tt.tty) + ios.SetStdoutTTY(tt.tty) + f := &cmdutil.Factory{ + IOStreams: ios, + } + argv, err := shlex.Split(tt.input) + assert.NoError(t, err) + + var cmdOpts *DeleteOptions + cmd := NewCmdDelete(f, func(opts *DeleteOptions) error { + cmdOpts = opts + return nil + }) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.wantErr { + assert.Error(t, err) + assert.EqualError(t, err, tt.wantErrMsg) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.output.KeyID, cmdOpts.KeyID) + assert.Equal(t, tt.output.Confirmed, cmdOpts.Confirmed) + }) + } +} + +func Test_deleteRun(t *testing.T) { + tests := []struct { + name string + tty bool + opts DeleteOptions + httpStubs func(*httpmock.Registry) + prompterStubs func(*prompter.PrompterMock) + wantStdout string + wantErr bool + wantErrMsg string + }{ + { + name: "prompting confirmation delete tty", + tty: true, + opts: DeleteOptions{KeyID: "123"}, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.ConfirmFunc = func(s string, b bool) (bool, error) { + return true, nil + } + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "{}")) + }, + wantErr: false, + wantStdout: "✓ Deleted SSH key id 123 from your account\n", + wantErrMsg: "", + }, + { + name: "cancel delete tty", + tty: true, + opts: DeleteOptions{KeyID: "123"}, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.ConfirmFunc = func(s string, b bool) (bool, error) { + return false, nil + } + }, + wantErr: true, + wantStdout: "", + wantErrMsg: "CancelError", + }, + { + name: "delete with confirm flag tty", + tty: true, + opts: DeleteOptions{KeyID: "123", Confirmed: true}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "{}")) + }, + wantErr: false, + wantStdout: "✓ Deleted SSH key id 123 from your account\n", + wantErrMsg: "", + }, + { + name: "not found tty", + tty: true, + opts: DeleteOptions{KeyID: "123"}, + prompterStubs: func(pm *prompter.PrompterMock) { + pm.ConfirmFunc = func(s string, b bool) (bool, error) { + return true, nil + } + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(404, "{}")) + }, + wantErr: true, + wantStdout: "", + wantErrMsg: "unable to delete SSH key id 123: either the SSH key is not found or it is not owned by you", + }, + { + name: "delete no tty", + opts: DeleteOptions{KeyID: "123"}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "{}")) + }, + wantErr: false, + wantStdout: "", + wantErrMsg: "", + }, + { + name: "not found no tty", + opts: DeleteOptions{KeyID: "123"}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(404, "{}")) + }, + wantErr: true, + wantStdout: "", + wantErrMsg: "unable to delete SSH key id 123: either the SSH key is not found or it is not owned by you", + }, + } + + for _, tt := range tests { + pm := &prompter.PrompterMock{} + if tt.prompterStubs != nil { + tt.prompterStubs(pm) + } + tt.opts.Prompter = pm + + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + tt.opts.Config = func() (config.Config, error) { + return config.NewBlankConfig(), nil + } + ios, _, stdout, _ := iostreams.Test() + ios.SetStdinTTY(tt.tty) + ios.SetStdoutTTY(tt.tty) + tt.opts.IO = ios + + t.Run(tt.name, func(t *testing.T) { + err := deleteRun(&tt.opts) + reg.Verify(t) + if tt.wantErr { + assert.Error(t, err) + assert.EqualError(t, err, tt.wantErrMsg) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.wantStdout, stdout.String()) + }) + } +} diff --git a/pkg/cmd/ssh-key/delete/http.go b/pkg/cmd/ssh-key/delete/http.go new file mode 100644 index 00000000000..98d72dbf1d2 --- /dev/null +++ b/pkg/cmd/ssh-key/delete/http.go @@ -0,0 +1,29 @@ +package delete + +import ( + "fmt" + "net/http" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghinstance" +) + +func deleteSSHKey(httpClient *http.Client, host string, keyID string) error { + url := fmt.Sprintf("%suser/keys/%s", ghinstance.RESTPrefix(host), keyID) + req, err := http.NewRequest("DELETE", url, nil) + if err != nil { + return err + } + + resp, err := httpClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return api.HandleHTTPError(resp) + } + + return nil +} diff --git a/pkg/cmd/ssh-key/ssh_key.go b/pkg/cmd/ssh-key/ssh_key.go index 312aeb77932..809985737fe 100644 --- a/pkg/cmd/ssh-key/ssh_key.go +++ b/pkg/cmd/ssh-key/ssh_key.go @@ -2,6 +2,7 @@ package key import ( cmdAdd "github.com/cli/cli/v2/pkg/cmd/ssh-key/add" + cmdDelete "github.com/cli/cli/v2/pkg/cmd/ssh-key/delete" cmdList "github.com/cli/cli/v2/pkg/cmd/ssh-key/list" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" @@ -14,8 +15,9 @@ func NewCmdSSHKey(f *cmdutil.Factory) *cobra.Command { Long: "Manage SSH keys registered with your GitHub account.", } - cmd.AddCommand(cmdList.NewCmdList(f, nil)) cmd.AddCommand(cmdAdd.NewCmdAdd(f, nil)) + cmd.AddCommand(cmdDelete.NewCmdDelete(f, nil)) + cmd.AddCommand(cmdList.NewCmdList(f, nil)) return cmd } From bccaa48e6551987aa26d81ed27a38d19ba523415 Mon Sep 17 00:00:00 2001 From: nsmag Date: Tue, 20 Sep 2022 11:01:44 +0700 Subject: [PATCH 2/8] Remove unnecessary condition --- pkg/cmd/ssh-key/delete/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/ssh-key/delete/delete.go b/pkg/cmd/ssh-key/delete/delete.go index 55d1fa69648..eeda9539a18 100644 --- a/pkg/cmd/ssh-key/delete/delete.go +++ b/pkg/cmd/ssh-key/delete/delete.go @@ -64,7 +64,7 @@ func deleteRun(opts *DeleteOptions) error { return err } - if opts.IO.CanPrompt() && !opts.Confirmed { + if !opts.Confirmed { confirmed, err := opts.Prompter.Confirm("Confirm deletion:", true) if err != nil { return err From b7029a7a115299b102e91ff777b497fbac0431c2 Mon Sep 17 00:00:00 2001 From: nsmag Date: Tue, 20 Sep 2022 11:13:53 +0700 Subject: [PATCH 3/8] Fix test --- pkg/cmd/ssh-key/delete/delete_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/ssh-key/delete/delete_test.go b/pkg/cmd/ssh-key/delete/delete_test.go index 6fb3374fd8f..fe7e03fd50a 100644 --- a/pkg/cmd/ssh-key/delete/delete_test.go +++ b/pkg/cmd/ssh-key/delete/delete_test.go @@ -174,7 +174,7 @@ func Test_deleteRun(t *testing.T) { }, { name: "delete no tty", - opts: DeleteOptions{KeyID: "123"}, + opts: DeleteOptions{KeyID: "123", Confirmed: true}, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "{}")) }, @@ -184,7 +184,7 @@ func Test_deleteRun(t *testing.T) { }, { name: "not found no tty", - opts: DeleteOptions{KeyID: "123"}, + opts: DeleteOptions{KeyID: "123", Confirmed: true}, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(404, "{}")) }, From 9a996c130af2ea3416a6d06c4483f9b9046a4f2f Mon Sep 17 00:00:00 2001 From: nsmag Date: Tue, 20 Sep 2022 12:14:51 +0700 Subject: [PATCH 4/8] Prompt confirm deletion with key title --- pkg/cmd/ssh-key/delete/delete.go | 20 +++++++++++---- pkg/cmd/ssh-key/delete/delete_test.go | 34 ++++++++++++------------ pkg/cmd/ssh-key/delete/http.go | 37 +++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 21 deletions(-) diff --git a/pkg/cmd/ssh-key/delete/delete.go b/pkg/cmd/ssh-key/delete/delete.go index eeda9539a18..91464967294 100644 --- a/pkg/cmd/ssh-key/delete/delete.go +++ b/pkg/cmd/ssh-key/delete/delete.go @@ -64,18 +64,28 @@ func deleteRun(opts *DeleteOptions) error { return err } + host, _ := cfg.DefaultHost() + key, err := getSSHKey(httpClient, host, opts.KeyID) + if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { + return fmt.Errorf("unable to delete SSH key id %s: either the SSH key is not found or it is not owned by you", opts.KeyID) + } + + return err + } + if !opts.Confirmed { - confirmed, err := opts.Prompter.Confirm("Confirm deletion:", true) + confirmTitle, err := opts.Prompter.Input(fmt.Sprintf("Type \"%s\" to confirm deletion:", key.Title), "") + if err != nil { return err } - if !confirmed { + if confirmTitle != key.Title { return cmdutil.CancelError } } - host, _ := cfg.DefaultHost() - err = deleteSSHKey(httpClient, host, opts.KeyID) if err != nil { var httpErr api.HTTPError @@ -88,7 +98,7 @@ func deleteRun(opts *DeleteOptions) error { if opts.IO.IsStdoutTTY() { cs := opts.IO.ColorScheme() - fmt.Fprintf(opts.IO.Out, "%s Deleted SSH key id %s from your account\n", cs.SuccessIcon(), opts.KeyID) + fmt.Fprintf(opts.IO.Out, "%s SSH key \"%s\" (%s) deleted from your account\n", cs.SuccessIcon(), key.Title, opts.KeyID) } return nil } diff --git a/pkg/cmd/ssh-key/delete/delete_test.go b/pkg/cmd/ssh-key/delete/delete_test.go index fe7e03fd50a..f87d92ccd2f 100644 --- a/pkg/cmd/ssh-key/delete/delete_test.go +++ b/pkg/cmd/ssh-key/delete/delete_test.go @@ -106,6 +106,7 @@ func TestNewCmdDelete(t *testing.T) { } func Test_deleteRun(t *testing.T) { + keyResp := "{\"title\":\"My Key\"}" tests := []struct { name string tty bool @@ -121,15 +122,16 @@ func Test_deleteRun(t *testing.T) { tty: true, opts: DeleteOptions{KeyID: "123"}, prompterStubs: func(pm *prompter.PrompterMock) { - pm.ConfirmFunc = func(s string, b bool) (bool, error) { - return true, nil + pm.InputFunc = func(s1 string, s2 string) (string, error) { + return "My Key", nil } }, httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "{}")) + reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "")) }, wantErr: false, - wantStdout: "✓ Deleted SSH key id 123 from your account\n", + wantStdout: "✓ SSH key \"My Key\" (123) deleted from your account\n", wantErrMsg: "", }, { @@ -137,10 +139,13 @@ func Test_deleteRun(t *testing.T) { tty: true, opts: DeleteOptions{KeyID: "123"}, prompterStubs: func(pm *prompter.PrompterMock) { - pm.ConfirmFunc = func(s string, b bool) (bool, error) { - return false, nil + pm.InputFunc = func(s1 string, s2 string) (string, error) { + return "", nil } }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) + }, wantErr: true, wantStdout: "", wantErrMsg: "CancelError", @@ -150,23 +155,19 @@ func Test_deleteRun(t *testing.T) { tty: true, opts: DeleteOptions{KeyID: "123", Confirmed: true}, httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "{}")) + reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "")) }, wantErr: false, - wantStdout: "✓ Deleted SSH key id 123 from your account\n", + wantStdout: "✓ SSH key \"My Key\" (123) deleted from your account\n", wantErrMsg: "", }, { name: "not found tty", tty: true, opts: DeleteOptions{KeyID: "123"}, - prompterStubs: func(pm *prompter.PrompterMock) { - pm.ConfirmFunc = func(s string, b bool) (bool, error) { - return true, nil - } - }, httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(404, "{}")) + reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(404, "")) }, wantErr: true, wantStdout: "", @@ -176,7 +177,8 @@ func Test_deleteRun(t *testing.T) { name: "delete no tty", opts: DeleteOptions{KeyID: "123", Confirmed: true}, httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "{}")) + reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) + reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "")) }, wantErr: false, wantStdout: "", @@ -186,7 +188,7 @@ func Test_deleteRun(t *testing.T) { name: "not found no tty", opts: DeleteOptions{KeyID: "123", Confirmed: true}, httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(404, "{}")) + reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(404, "")) }, wantErr: true, wantStdout: "", diff --git a/pkg/cmd/ssh-key/delete/http.go b/pkg/cmd/ssh-key/delete/http.go index 98d72dbf1d2..906ae6bc906 100644 --- a/pkg/cmd/ssh-key/delete/http.go +++ b/pkg/cmd/ssh-key/delete/http.go @@ -1,13 +1,19 @@ package delete import ( + "encoding/json" "fmt" + "io" "net/http" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" ) +type sshKey struct { + Title string +} + func deleteSSHKey(httpClient *http.Client, host string, keyID string) error { url := fmt.Sprintf("%suser/keys/%s", ghinstance.RESTPrefix(host), keyID) req, err := http.NewRequest("DELETE", url, nil) @@ -27,3 +33,34 @@ func deleteSSHKey(httpClient *http.Client, host string, keyID string) error { return nil } + +func getSSHKey(httpClient *http.Client, host string, keyID string) (*sshKey, error) { + url := fmt.Sprintf("%suser/keys/%s", ghinstance.RESTPrefix(host), keyID) + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return nil, err + } + + resp, err := httpClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return nil, api.HandleHTTPError(resp) + } + + b, err := io.ReadAll(resp.Body) + if err != nil { + return nil, err + } + + var key sshKey + err = json.Unmarshal(b, &key) + if err != nil { + return nil, err + } + + return &key, nil +} From 547bf0a9604f1c9ba52997d3d7a6498def609b52 Mon Sep 17 00:00:00 2001 From: Natthakit Susanthitanon Date: Wed, 28 Sep 2022 15:42:37 +0700 Subject: [PATCH 5/8] Return err as is --- pkg/cmd/ssh-key/delete/delete.go | 12 ------------ pkg/cmd/ssh-key/delete/delete_test.go | 4 ++-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/ssh-key/delete/delete.go b/pkg/cmd/ssh-key/delete/delete.go index 91464967294..5dc6850be64 100644 --- a/pkg/cmd/ssh-key/delete/delete.go +++ b/pkg/cmd/ssh-key/delete/delete.go @@ -1,11 +1,9 @@ package delete import ( - "errors" "fmt" "net/http" - "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/cmdutil" @@ -67,11 +65,6 @@ func deleteRun(opts *DeleteOptions) error { host, _ := cfg.DefaultHost() key, err := getSSHKey(httpClient, host, opts.KeyID) if err != nil { - var httpErr api.HTTPError - if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { - return fmt.Errorf("unable to delete SSH key id %s: either the SSH key is not found or it is not owned by you", opts.KeyID) - } - return err } @@ -88,11 +81,6 @@ func deleteRun(opts *DeleteOptions) error { err = deleteSSHKey(httpClient, host, opts.KeyID) if err != nil { - var httpErr api.HTTPError - if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { - return fmt.Errorf("unable to delete SSH key id %s: either the SSH key is not found or it is not owned by you", opts.KeyID) - } - return err } diff --git a/pkg/cmd/ssh-key/delete/delete_test.go b/pkg/cmd/ssh-key/delete/delete_test.go index f87d92ccd2f..4ed4ecb2dd9 100644 --- a/pkg/cmd/ssh-key/delete/delete_test.go +++ b/pkg/cmd/ssh-key/delete/delete_test.go @@ -171,7 +171,7 @@ func Test_deleteRun(t *testing.T) { }, wantErr: true, wantStdout: "", - wantErrMsg: "unable to delete SSH key id 123: either the SSH key is not found or it is not owned by you", + wantErrMsg: "HTTP 404 (https://api.github.com/user/keys/123)", }, { name: "delete no tty", @@ -192,7 +192,7 @@ func Test_deleteRun(t *testing.T) { }, wantErr: true, wantStdout: "", - wantErrMsg: "unable to delete SSH key id 123: either the SSH key is not found or it is not owned by you", + wantErrMsg: "HTTP 404 (https://api.github.com/user/keys/123)", }, } From 2b11f02d5693abf69966b74cf01d9f61d0f1f8aa Mon Sep 17 00:00:00 2001 From: Natthakit Susanthitanon Date: Wed, 5 Oct 2022 13:53:11 +0700 Subject: [PATCH 6/8] Use prompter.ConfirmDeletion --- pkg/cmd/ssh-key/delete/delete.go | 7 +------ pkg/cmd/ssh-key/delete/delete_test.go | 20 ++------------------ 2 files changed, 3 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/ssh-key/delete/delete.go b/pkg/cmd/ssh-key/delete/delete.go index 5dc6850be64..32bd2258315 100644 --- a/pkg/cmd/ssh-key/delete/delete.go +++ b/pkg/cmd/ssh-key/delete/delete.go @@ -69,14 +69,9 @@ func deleteRun(opts *DeleteOptions) error { } if !opts.Confirmed { - confirmTitle, err := opts.Prompter.Input(fmt.Sprintf("Type \"%s\" to confirm deletion:", key.Title), "") - - if err != nil { + if err := opts.Prompter.ConfirmDeletion(key.Title); err != nil { return err } - if confirmTitle != key.Title { - return cmdutil.CancelError - } } err = deleteSSHKey(httpClient, host, opts.KeyID) diff --git a/pkg/cmd/ssh-key/delete/delete_test.go b/pkg/cmd/ssh-key/delete/delete_test.go index 4ed4ecb2dd9..da0103493d8 100644 --- a/pkg/cmd/ssh-key/delete/delete_test.go +++ b/pkg/cmd/ssh-key/delete/delete_test.go @@ -122,8 +122,8 @@ func Test_deleteRun(t *testing.T) { tty: true, opts: DeleteOptions{KeyID: "123"}, prompterStubs: func(pm *prompter.PrompterMock) { - pm.InputFunc = func(s1 string, s2 string) (string, error) { - return "My Key", nil + pm.ConfirmDeletionFunc = func(_ string) error { + return nil } }, httpStubs: func(reg *httpmock.Registry) { @@ -134,22 +134,6 @@ func Test_deleteRun(t *testing.T) { wantStdout: "✓ SSH key \"My Key\" (123) deleted from your account\n", wantErrMsg: "", }, - { - name: "cancel delete tty", - tty: true, - opts: DeleteOptions{KeyID: "123"}, - prompterStubs: func(pm *prompter.PrompterMock) { - pm.InputFunc = func(s1 string, s2 string) (string, error) { - return "", nil - } - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) - }, - wantErr: true, - wantStdout: "", - wantErrMsg: "CancelError", - }, { name: "delete with confirm flag tty", tty: true, From 1a1aabddc2171918a9c5403d19622a8114f385bf Mon Sep 17 00:00:00 2001 From: Natthakit Susanthitanon Date: Wed, 5 Oct 2022 22:09:24 +0700 Subject: [PATCH 7/8] Refactor test --- pkg/cmd/ssh-key/delete/delete_test.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/pkg/cmd/ssh-key/delete/delete_test.go b/pkg/cmd/ssh-key/delete/delete_test.go index da0103493d8..437443c5591 100644 --- a/pkg/cmd/ssh-key/delete/delete_test.go +++ b/pkg/cmd/ssh-key/delete/delete_test.go @@ -118,7 +118,7 @@ func Test_deleteRun(t *testing.T) { wantErrMsg string }{ { - name: "prompting confirmation delete tty", + name: "delete tty", tty: true, opts: DeleteOptions{KeyID: "123"}, prompterStubs: func(pm *prompter.PrompterMock) { @@ -130,9 +130,7 @@ func Test_deleteRun(t *testing.T) { reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "")) }, - wantErr: false, wantStdout: "✓ SSH key \"My Key\" (123) deleted from your account\n", - wantErrMsg: "", }, { name: "delete with confirm flag tty", @@ -142,9 +140,7 @@ func Test_deleteRun(t *testing.T) { reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "")) }, - wantErr: false, wantStdout: "✓ SSH key \"My Key\" (123) deleted from your account\n", - wantErrMsg: "", }, { name: "not found tty", @@ -154,7 +150,6 @@ func Test_deleteRun(t *testing.T) { reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(404, "")) }, wantErr: true, - wantStdout: "", wantErrMsg: "HTTP 404 (https://api.github.com/user/keys/123)", }, { @@ -164,9 +159,7 @@ func Test_deleteRun(t *testing.T) { reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(200, keyResp)) reg.Register(httpmock.REST("DELETE", "user/keys/123"), httpmock.StatusStringResponse(204, "")) }, - wantErr: false, wantStdout: "", - wantErrMsg: "", }, { name: "not found no tty", @@ -175,7 +168,6 @@ func Test_deleteRun(t *testing.T) { reg.Register(httpmock.REST("GET", "user/keys/123"), httpmock.StatusStringResponse(404, "")) }, wantErr: true, - wantStdout: "", wantErrMsg: "HTTP 404 (https://api.github.com/user/keys/123)", }, } From 967423722455c631fee1bade5346480833808fed Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Wed, 12 Oct 2022 11:14:23 +0300 Subject: [PATCH 8/8] Use %q instead of quotes --- pkg/cmd/ssh-key/delete/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/ssh-key/delete/delete.go b/pkg/cmd/ssh-key/delete/delete.go index 32bd2258315..a11cbd76928 100644 --- a/pkg/cmd/ssh-key/delete/delete.go +++ b/pkg/cmd/ssh-key/delete/delete.go @@ -81,7 +81,7 @@ func deleteRun(opts *DeleteOptions) error { if opts.IO.IsStdoutTTY() { cs := opts.IO.ColorScheme() - fmt.Fprintf(opts.IO.Out, "%s SSH key \"%s\" (%s) deleted from your account\n", cs.SuccessIcon(), key.Title, opts.KeyID) + fmt.Fprintf(opts.IO.Out, "%s SSH key %q (%s) deleted from your account\n", cs.SuccessIcon(), key.Title, opts.KeyID) } return nil }