From e373195817c6cc0b5d2143f8668a1805fd879575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Jul 2020 18:12:46 +0200 Subject: [PATCH 01/10] WIP migrate gist create to separate package --- api/client.go | 18 ++- command/gist.go | 175 -------------------------- command/gist_test.go | 56 --------- command/root.go | 9 ++ pkg/cmd/gist/create/create.go | 157 +++++++++++++++++++++++ pkg/cmd/gist/create/create_test.go | 196 +++++++++++++++++++++++++++++ pkg/cmd/gist/create/http.go | 53 ++++++++ pkg/iostreams/iostreams.go | 18 +++ 8 files changed, 446 insertions(+), 236 deletions(-) delete mode 100644 command/gist.go delete mode 100644 command/gist_test.go create mode 100644 pkg/cmd/gist/create/create.go create mode 100644 pkg/cmd/gist/create/create_test.go create mode 100644 pkg/cmd/gist/create/http.go diff --git a/api/client.go b/api/client.go index f504269d8..1f16cffc7 100644 --- a/api/client.go +++ b/api/client.go @@ -33,6 +33,12 @@ func NewClient(opts ...ClientOption) *Client { return client } +// NewClientFromHTTP takes in an http.Client instance +func NewClientFromHTTP(httpClient *http.Client) *Client { + client := &Client{http: httpClient} + return client +} + // AddHeader turns a RoundTripper into one that adds a request header func AddHeader(name, value string) ClientOption { return func(tr http.RoundTripper) http.RoundTripper { @@ -179,9 +185,10 @@ func (gr GraphQLErrorResponse) Error() string { // HTTPError is an error returned by a failed API call type HTTPError struct { - StatusCode int - RequestURL *url.URL - Message string + StatusCode int + RequestURL *url.URL + Message string + OAuthScopes string } func (err HTTPError) Error() string { @@ -322,8 +329,9 @@ func handleResponse(resp *http.Response, data interface{}) error { func handleHTTPError(resp *http.Response) error { httpError := HTTPError{ - StatusCode: resp.StatusCode, - RequestURL: resp.Request.URL, + StatusCode: resp.StatusCode, + RequestURL: resp.Request.URL, + OAuthScopes: resp.Header.Get("X-Oauth-Scopes"), } body, err := ioutil.ReadAll(resp.Body) diff --git a/command/gist.go b/command/gist.go deleted file mode 100644 index 92bcf6b52..000000000 --- a/command/gist.go +++ /dev/null @@ -1,175 +0,0 @@ -package command - -import ( - "errors" - "fmt" - "io" - "io/ioutil" - "os" - "path" - - "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/api" - "github.com/cli/cli/pkg/cmdutil" - "github.com/cli/cli/utils" - "github.com/spf13/cobra" -) - -func init() { - RootCmd.AddCommand(gistCmd) - gistCmd.AddCommand(gistCreateCmd) - gistCreateCmd.Flags().StringP("desc", "d", "", "A description for this gist") - gistCreateCmd.Flags().BoolP("public", "p", false, "List the gist publicly (default: private)") -} - -var gistCmd = &cobra.Command{ - Use: "gist", - Short: "Create gists", - Long: `Work with GitHub gists.`, -} - -var gistCreateCmd = &cobra.Command{ - Use: `create [... | -]`, - Short: "Create a new gist", - Long: `Create a new GitHub gist with given contents. - -Gists can be created from one or multiple files. Alternatively, pass "-" as -file name to read from standard input. - -By default, gists are private; use '--public' to make publicly listed ones.`, - Example: heredoc.Doc(` - # publish file 'hello.py' as a public gist - $ gh gist create --public hello.py - - # create a gist with a description - $ gh gist create hello.py -d "my Hello-World program in Python" - - # create a gist containing several files - $ gh gist create hello.py world.py cool.txt - - # read from standard input to create a gist - $ gh gist create - - - # create a gist from output piped from another command - $ cat cool.txt | gh gist create - `), - Args: func(cmd *cobra.Command, args []string) error { - if len(args) > 0 { - return nil - } - - info, err := os.Stdin.Stat() - if err != nil { - return fmt.Errorf("failed to check STDIN: %w", err) - } - - stdinIsTTY := (info.Mode() & os.ModeCharDevice) == os.ModeCharDevice - if stdinIsTTY { - return &cmdutil.FlagError{Err: errors.New("no filenames passed and nothing on STDIN")} - } - return nil - }, - RunE: gistCreate, -} - -type Opts struct { - Description string - Public bool -} - -func gistCreate(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - client, err := apiClientForContext(ctx) - if err != nil { - return err - } - - // This performs a dummy query, checks what scopes we have, and then asks for a user to reauth - // with expanded scopes. it introduces latency whenever this command is run: a trade-off to avoid - // having every single user reauth as a result of this feature even if they never once use gists. - // - // In the future we'd rather have the ability to detect a "reauth needed" scenario and replay - // failed requests but some short spikes indicated that that would be a fair bit of work. - client, err = ensureScopes(ctx, client, "gist") - if err != nil { - return err - } - - opts, err := processOpts(cmd) - if err != nil { - return fmt.Errorf("did not understand arguments: %w", err) - } - - fileArgs := args - if len(args) == 0 { - fileArgs = []string{"-"} - } - - files, err := processFiles(os.Stdin, fileArgs) - if err != nil { - return fmt.Errorf("failed to collect files for posting: %w", err) - } - - errOut := colorableErr(cmd) - fmt.Fprintf(errOut, "%s Creating gist...\n", utils.Gray("-")) - - gist, err := api.GistCreate(client, opts.Description, opts.Public, files) - if err != nil { - return fmt.Errorf("%s Failed to create gist: %w", utils.Red("X"), err) - } - - fmt.Fprintf(errOut, "%s Created gist\n", utils.Green("✓")) - - fmt.Fprintln(cmd.OutOrStdout(), gist.HTMLURL) - - return nil -} - -func processOpts(cmd *cobra.Command) (*Opts, error) { - description, err := cmd.Flags().GetString("desc") - if err != nil { - return nil, err - } - - public, err := cmd.Flags().GetBool("public") - if err != nil { - return nil, err - } - - return &Opts{ - Description: description, - Public: public, - }, err -} - -func processFiles(stdin io.ReadCloser, filenames []string) (map[string]string, error) { - fs := map[string]string{} - - if len(filenames) == 0 { - return nil, errors.New("no files passed") - } - - for i, f := range filenames { - var filename string - var content []byte - var err error - if f == "-" { - filename = fmt.Sprintf("gistfile%d.txt", i) - content, err = ioutil.ReadAll(stdin) - if err != nil { - return fs, fmt.Errorf("failed to read from stdin: %w", err) - } - stdin.Close() - } else { - content, err = ioutil.ReadFile(f) - if err != nil { - return fs, fmt.Errorf("failed to read file %s: %w", f, err) - } - filename = path.Base(f) - } - - fs[filename] = string(content) - } - - return fs, nil -} diff --git a/command/gist_test.go b/command/gist_test.go deleted file mode 100644 index 9f39f49ff..000000000 --- a/command/gist_test.go +++ /dev/null @@ -1,56 +0,0 @@ -package command - -import ( - "encoding/json" - "io/ioutil" - "strings" - "testing" - - "github.com/cli/cli/pkg/httpmock" - "github.com/stretchr/testify/assert" -) - -func TestGistCreate(t *testing.T) { - initBlankContext("", "OWNER/REPO", "trunk") - - http := initFakeHTTP() - http.Register(httpmock.REST("POST", "gists"), httpmock.StringResponse(` - { - "html_url": "https://gist.github.com/aa5a315d61ae9438b18d" - } - `)) - - output, err := RunCommand(`gist create "../test/fixtures/gistCreate.json" -d "Gist description" --public`) - assert.NoError(t, err) - - bodyBytes, _ := ioutil.ReadAll(http.Requests[0].Body) - reqBody := make(map[string]interface{}) - err = json.Unmarshal(bodyBytes, &reqBody) - if err != nil { - t.Fatalf("error decoding JSON: %v", err) - } - - expectParams := map[string]interface{}{ - "description": "Gist description", - "files": map[string]interface{}{ - "gistCreate.json": map[string]interface{}{ - "content": "{}", - }, - }, - "public": true, - } - - assert.Equal(t, expectParams, reqBody) - assert.Equal(t, "https://gist.github.com/aa5a315d61ae9438b18d\n", output.String()) -} - -func TestGistCreate_stdin(t *testing.T) { - fakeStdin := strings.NewReader("hey cool how is it going") - files, err := processFiles(ioutil.NopCloser(fakeStdin), []string{"-"}) - if err != nil { - t.Fatalf("unexpected error processing files: %s", err) - } - - assert.Equal(t, 1, len(files)) - assert.Equal(t, "hey cool how is it going", files["gistfile0.txt"]) -} diff --git a/command/root.go b/command/root.go index 7d0b2258f..3c28e53ed 100644 --- a/command/root.go +++ b/command/root.go @@ -20,6 +20,7 @@ import ( "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" apiCmd "github.com/cli/cli/pkg/cmd/api" + gistCreateCmd "github.com/cli/cli/pkg/cmd/gist/create" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" @@ -95,6 +96,14 @@ func init() { }, } RootCmd.AddCommand(apiCmd.NewCmdApi(cmdFactory, nil)) + + gistCmd := &cobra.Command{ + Use: "gist", + Short: "Create gists", + Long: `Work with GitHub gists.`, + } + RootCmd.AddCommand(gistCmd) + gistCmd.AddCommand(gistCreateCmd.NewCmdCreate(cmdFactory, nil)) } // RootCmd is the entry point of command-line execution diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go new file mode 100644 index 000000000..fe25a2813 --- /dev/null +++ b/pkg/cmd/gist/create/create.go @@ -0,0 +1,157 @@ +package create + +import ( + "errors" + "fmt" + "io" + "io/ioutil" + "net/http" + "os" + "path" + "strings" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/api" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type CreateOptions struct { + IO *iostreams.IOStreams + + Description string + Public bool + Filenames []string + + HttpClient func() (*http.Client, error) +} + +func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Command { + opts := CreateOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + } + + cmd := &cobra.Command{ + Use: "create [... | -]", + Short: "Create a new gist", + Long: heredoc.Doc(` + Create a new GitHub gist with given contents. + + Gists can be created from one or multiple files. Alternatively, pass "-" as + file name to read from standard input. + + By default, gists are private; use '--public' to make publicly listed ones. + `), + Example: heredoc.Doc(` + # publish file 'hello.py' as a public gist + $ gh gist create --public hello.py + + # create a gist with a description + $ gh gist create hello.py -d "my Hello-World program in Python" + + # create a gist containing several files + $ gh gist create hello.py world.py cool.txt + + # read from standard input to create a gist + $ gh gist create - + + # create a gist from output piped from another command + $ cat cool.txt | gh gist create + `), + Args: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + return nil + } + if opts.IO.IsStdinTTY() { + return &cmdutil.FlagError{Err: errors.New("no filenames passed and nothing on STDIN")} + } + return nil + }, + RunE: func(c *cobra.Command, args []string) error { + opts.HttpClient = f.HttpClient + + opts.Filenames = args + + if runF != nil { + return runF(&opts) + } + return createRun(&opts) + }, + } + + cmd.Flags().StringVarP(&opts.Description, "desc", "d", "", "A description for this gist") + cmd.Flags().BoolVarP(&opts.Public, "public", "p", false, "List the gist publicly (default: private)") + return cmd +} + +func createRun(opts *CreateOptions) error { + fileArgs := opts.Filenames + if len(fileArgs) == 0 { + fileArgs = []string{"-"} + } + + files, err := processFiles(os.Stdin, fileArgs) + if err != nil { + return fmt.Errorf("failed to collect files for posting: %w", err) + } + + errOut := opts.IO.ErrOut + fmt.Fprintf(errOut, "%s Creating gist...\n", utils.Gray("-")) + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + gist, err := apiCreate(httpClient, opts.Description, opts.Public, files) + if err != nil { + var httpError api.HTTPError + if errors.As(err, &httpError) { + if httpError.OAuthScopes != "" && !strings.Contains(httpError.OAuthScopes, "gist") { + return fmt.Errorf("This command requires the 'gist' OAuth scope.\nPlease re-authenticate by doing `gh config set -h github.com oauth_token ''` and running the command again.") + } + } + return fmt.Errorf("%s Failed to create gist: %w", utils.Red("X"), err) + } + + fmt.Fprintf(errOut, "%s Created gist\n", utils.Green("✓")) + + fmt.Fprintln(opts.IO.Out, gist.HTMLURL) + + return nil +} + +func processFiles(stdin io.ReadCloser, filenames []string) (map[string]string, error) { + fs := map[string]string{} + + if len(filenames) == 0 { + return nil, errors.New("no files passed") + } + + for i, f := range filenames { + var filename string + var content []byte + var err error + if f == "-" { + filename = fmt.Sprintf("gistfile%d.txt", i) + content, err = ioutil.ReadAll(stdin) + if err != nil { + return fs, fmt.Errorf("failed to read from stdin: %w", err) + } + stdin.Close() + } else { + content, err = ioutil.ReadFile(f) + if err != nil { + return fs, fmt.Errorf("failed to read file %s: %w", f, err) + } + filename = path.Base(f) + } + + fs[filename] = string(content) + } + + return fs, nil +} diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go new file mode 100644 index 000000000..f829d0bcc --- /dev/null +++ b/pkg/cmd/gist/create/create_test.go @@ -0,0 +1,196 @@ +package create + +import ( + "bytes" + "io/ioutil" + "net/http" + "strings" + "testing" + + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +// func TestGistCreate(t *testing.T) { +// initBlankContext("", "OWNER/REPO", "trunk") + +// http := initFakeHTTP() +// http.Register(httpmock.REST("POST", "gists"), httpmock.StringResponse(` +// { +// "html_url": "https://gist.github.com/aa5a315d61ae9438b18d" +// } +// `)) + +// output, err := RunCommand(`gist create "../test/fixtures/gistCreate.json" -d "Gist description" --public`) +// assert.NoError(t, err) + +// bodyBytes, _ := ioutil.ReadAll(http.Requests[0].Body) +// reqBody := make(map[string]interface{}) +// err = json.Unmarshal(bodyBytes, &reqBody) +// if err != nil { +// t.Fatalf("error decoding JSON: %v", err) +// } + +// expectParams := map[string]interface{}{ +// "description": "Gist description", +// "files": map[string]interface{}{ +// "gistCreate.json": map[string]interface{}{ +// "content": "{}", +// }, +// }, +// "public": true, +// } + +// assert.Equal(t, expectParams, reqBody) +// assert.Equal(t, "https://gist.github.com/aa5a315d61ae9438b18d\n", output.String()) +// } + +func Test_processFiles(t *testing.T) { + fakeStdin := strings.NewReader("hey cool how is it going") + files, err := processFiles(ioutil.NopCloser(fakeStdin), []string{"-"}) + if err != nil { + t.Fatalf("unexpected error processing files: %s", err) + } + + assert.Equal(t, 1, len(files)) + assert.Equal(t, "hey cool how is it going", files["gistfile0.txt"]) +} + +func TestNewCmdCreate(t *testing.T) { + tests := []struct { + name string + cli string + factory func(*cmdutil.Factory) *cmdutil.Factory + wants CreateOptions + wantsErr bool + }{ + { + name: "no arguments", + cli: "", + wants: CreateOptions{ + Description: "", + Public: false, + Filenames: []string{""}, + }, + wantsErr: false, + }, + { + name: "no arguments with TTY stdin", + factory: func(f *cmdutil.Factory) *cmdutil.Factory { + f.IOStreams.SetStdinTTY(true) + return f + }, + cli: "", + wants: CreateOptions{ + Description: "", + Public: false, + Filenames: []string{""}, + }, + wantsErr: true, + }, + { + name: "stdin argument", + cli: "-", + wants: CreateOptions{ + Description: "", + Public: false, + Filenames: []string{"-"}, + }, + wantsErr: false, + }, + { + name: "with description", + cli: `-d "my new gist" -`, + wants: CreateOptions{ + Description: "my new gist", + Public: false, + Filenames: []string{"-"}, + }, + wantsErr: false, + }, + { + name: "public", + cli: `--public -`, + wants: CreateOptions{ + Description: "", + Public: true, + Filenames: []string{"-"}, + }, + wantsErr: false, + }, + { + name: "list of files", + cli: "file1.txt file2.txt", + wants: CreateOptions{ + Description: "", + Public: false, + Filenames: []string{"file1.txt", "file2.txt"}, + }, + wantsErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: io, + } + + if tt.factory != nil { + f = tt.factory(f) + } + + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + + var gotOpts *CreateOptions + cmd := NewCmdCreate(f, func(opts *CreateOptions) error { + gotOpts = opts + return nil + }) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + + assert.Equal(t, tt.wants.Description, gotOpts.Description) + assert.Equal(t, tt.wants.Public, gotOpts.Public) + }) + } +} + +func Test_createRun(t *testing.T) { + tests := []struct { + name string + opts *CreateOptions + wantErr bool + }{ + { + name: "basic", + opts: &CreateOptions{ + Filenames: []string{"-"}, + HttpClient: func() (*http.Client, error) { + // TODO: return an http.Client that wraps an httpmock instance + return nil, nil + }, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := createRun(tt.opts); (err != nil) != tt.wantErr { + t.Errorf("createRun() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/pkg/cmd/gist/create/http.go b/pkg/cmd/gist/create/http.go new file mode 100644 index 000000000..55ea61033 --- /dev/null +++ b/pkg/cmd/gist/create/http.go @@ -0,0 +1,53 @@ +package create + +import ( + "bytes" + "encoding/json" + "net/http" + + "github.com/cli/cli/api" +) + +// Gist represents a GitHub's gist. +type Gist struct { + Description string `json:"description,omitempty"` + Public bool `json:"public,omitempty"` + Files map[GistFilename]GistFile `json:"files,omitempty"` + HTMLURL string `json:"html_url,omitempty"` +} + +type GistFilename string + +type GistFile struct { + Content string `json:"content,omitempty"` +} + +func apiCreate(httpClient *http.Client, description string, public bool, files map[string]string) (*Gist, error) { + gistFiles := map[GistFilename]GistFile{} + + for filename, content := range files { + gistFiles[GistFilename(filename)] = GistFile{content} + } + + path := "gists" + body := &Gist{ + Description: description, + Public: public, + Files: gistFiles, + } + result := Gist{} + + requestByte, err := json.Marshal(body) + if err != nil { + return nil, err + } + requestBody := bytes.NewReader(requestByte) + + apiClient := api.NewClientFromHTTP(httpClient) + err = apiClient.REST("POST", path, requestBody, &result) + if err != nil { + return nil, err + } + + return &result, nil +} diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 8fd213e9f..91660a135 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -16,12 +16,30 @@ type IOStreams struct { ErrOut io.Writer colorEnabled bool + + stdinTTYOverride bool + stdinIsTTY bool } func (s *IOStreams) ColorEnabled() bool { return s.colorEnabled } +func (s *IOStreams) SetStdinTTY(isTTY bool) { + s.stdinTTYOverride = true + s.stdinIsTTY = isTTY +} + +func (s *IOStreams) IsStdinTTY() bool { + if s.stdinTTYOverride { + return s.stdinIsTTY + } + if stdin, ok := s.In.(*os.File); ok { + return isTerminal(stdin) + } + return false +} + func System() *IOStreams { var out io.Writer = os.Stdout var colorEnabled bool From 5085a5eda2359232de44c82a8581f6d4884bddd2 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Jul 2020 15:17:30 -0500 Subject: [PATCH 02/10] WIP test works with mock --- pkg/cmd/gist/create/create_test.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index f829d0bcc..bbf3f1371 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" @@ -168,20 +169,40 @@ func TestNewCmdCreate(t *testing.T) { } } +func testIO() *iostreams.IOStreams { + tio, _, _, _ := iostreams.Test() + return tio +} + func Test_createRun(t *testing.T) { tests := []struct { name string opts *CreateOptions + want func(t *testing.T) wantErr bool }{ + // TODO stdin passed as - + // TODO stdin as | + // TODO multiple files + // TODO description + // TODO public { name: "basic", opts: &CreateOptions{ + IO: testIO(), Filenames: []string{"-"}, HttpClient: func() (*http.Client, error) { - // TODO: return an http.Client that wraps an httpmock instance - return nil, nil + reg := &httpmock.Registry{} + reg.Register(httpmock.REST("POST", "gists"), httpmock.StringResponse(` + { + "html_url": "https://gist.github.com/aa5a315d61ae9438b18d" + }`)) + + return &http.Client{Transport: reg}, nil }, + }, + want: func(t *testing.T) { + }, wantErr: false, }, @@ -191,6 +212,7 @@ func Test_createRun(t *testing.T) { if err := createRun(tt.opts); (err != nil) != tt.wantErr { t.Errorf("createRun() error = %v, wantErr %v", err, tt.wantErr) } + tt.want(t) }) } } From ba46362a7f166ec9aa307afcd3ecac679410985e Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Jul 2020 17:19:59 -0500 Subject: [PATCH 03/10] add createRun tests --- pkg/cmd/gist/create/create.go | 3 +- pkg/cmd/gist/create/create_test.go | 150 ++++++++++++++++++----------- 2 files changed, 95 insertions(+), 58 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index fe25a2813..d4fcd276c 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -6,7 +6,6 @@ import ( "io" "io/ioutil" "net/http" - "os" "path" "strings" @@ -93,7 +92,7 @@ func createRun(opts *CreateOptions) error { fileArgs = []string{"-"} } - files, err := processFiles(os.Stdin, fileArgs) + files, err := processFiles(opts.IO.In, fileArgs) if err != nil { return fmt.Errorf("failed to collect files for posting: %w", err) } diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index bbf3f1371..0b6a021b7 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -2,6 +2,7 @@ package create import ( "bytes" + "encoding/json" "io/ioutil" "net/http" "strings" @@ -14,40 +15,6 @@ import ( "github.com/stretchr/testify/assert" ) -// func TestGistCreate(t *testing.T) { -// initBlankContext("", "OWNER/REPO", "trunk") - -// http := initFakeHTTP() -// http.Register(httpmock.REST("POST", "gists"), httpmock.StringResponse(` -// { -// "html_url": "https://gist.github.com/aa5a315d61ae9438b18d" -// } -// `)) - -// output, err := RunCommand(`gist create "../test/fixtures/gistCreate.json" -d "Gist description" --public`) -// assert.NoError(t, err) - -// bodyBytes, _ := ioutil.ReadAll(http.Requests[0].Body) -// reqBody := make(map[string]interface{}) -// err = json.Unmarshal(bodyBytes, &reqBody) -// if err != nil { -// t.Fatalf("error decoding JSON: %v", err) -// } - -// expectParams := map[string]interface{}{ -// "description": "Gist description", -// "files": map[string]interface{}{ -// "gistCreate.json": map[string]interface{}{ -// "content": "{}", -// }, -// }, -// "public": true, -// } - -// assert.Equal(t, expectParams, reqBody) -// assert.Equal(t, "https://gist.github.com/aa5a315d61ae9438b18d\n", output.String()) -// } - func Test_processFiles(t *testing.T) { fakeStdin := strings.NewReader("hey cool how is it going") files, err := processFiles(ioutil.NopCloser(fakeStdin), []string{"-"}) @@ -169,6 +136,12 @@ func TestNewCmdCreate(t *testing.T) { } } +func testIOWithStdin(stdinContent string) *iostreams.IOStreams { + tio, stdin, _, _ := iostreams.Test() + stdin.WriteString(stdinContent) + return tio +} + func testIO() *iostreams.IOStreams { tio, _, _, _ := iostreams.Test() return tio @@ -176,43 +149,108 @@ func testIO() *iostreams.IOStreams { func Test_createRun(t *testing.T) { tests := []struct { - name string - opts *CreateOptions - want func(t *testing.T) - wantErr bool + name string + opts *CreateOptions + wantOut string + wantParams map[string]interface{} + wantErr bool }{ - // TODO stdin passed as - - // TODO stdin as | - // TODO multiple files - // TODO description - // TODO public { - name: "basic", + name: "public", opts: &CreateOptions{ IO: testIO(), - Filenames: []string{"-"}, - HttpClient: func() (*http.Client, error) { - reg := &httpmock.Registry{} - reg.Register(httpmock.REST("POST", "gists"), httpmock.StringResponse(` - { - "html_url": "https://gist.github.com/aa5a315d61ae9438b18d" - }`)) - - return &http.Client{Transport: reg}, nil + Public: true, + Filenames: []string{"../../../../test/fixtures/gistCreate.json"}, + }, + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantErr: false, + wantParams: map[string]interface{}{ + "public": true, + "files": map[string]interface{}{ + "gistCreate.json": map[string]interface{}{ + "content": "{}", + }, }, }, - want: func(t *testing.T) { - + }, + { + name: "with description", + opts: &CreateOptions{ + IO: testIO(), + Description: "an incredibly interesting gist", + Filenames: []string{"../../../../test/fixtures/gistCreate.json"}, }, + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", wantErr: false, + wantParams: map[string]interface{}{ + "description": "an incredibly interesting gist", + "files": map[string]interface{}{ + "gistCreate.json": map[string]interface{}{ + "content": "{}", + }, + }, + }, + }, + { + name: "multiple files", + opts: &CreateOptions{ + IO: testIOWithStdin("cool stdin content"), + Filenames: []string{"../../../../test/fixtures/gistCreate.json", "-"}, + }, + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantErr: false, + wantParams: map[string]interface{}{ + "files": map[string]interface{}{ + "gistCreate.json": map[string]interface{}{ + "content": "{}", + }, + "gistfile1.txt": map[string]interface{}{ + "content": "cool stdin content", + }, + }, + }, + }, + { + name: "stdin arg", + opts: &CreateOptions{ + IO: testIOWithStdin("cool stdin content"), + Filenames: []string{"-"}, + }, + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantErr: false, + wantParams: map[string]interface{}{ + "files": map[string]interface{}{ + "gistfile0.txt": map[string]interface{}{ + "content": "cool stdin content", + }, + }, + }, }, } for _, tt := range tests { + reg := &httpmock.Registry{} + reg.Register(httpmock.REST("POST", "gists"), httpmock.StringResponse(` + { "html_url": "https://gist.github.com/aa5a315d61ae9438b18d"}`)) + + mockClient := func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + + tt.opts.HttpClient = mockClient + t.Run(tt.name, func(t *testing.T) { if err := createRun(tt.opts); (err != nil) != tt.wantErr { t.Errorf("createRun() error = %v, wantErr %v", err, tt.wantErr) } - tt.want(t) + bodyBytes, _ := ioutil.ReadAll(reg.Requests[0].Body) + reqBody := make(map[string]interface{}) + err := json.Unmarshal(bodyBytes, &reqBody) + if err != nil { + t.Fatalf("error decoding JSON: %v", err) + } + assert.Equal(t, tt.wantOut, tt.opts.IO.Out.(*bytes.Buffer).String()) + assert.Equal(t, tt.wantParams, reqBody) + reg.Verify(t) }) } } From d12ff4b9e568eff3c740cffd43702a6b73d34add Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Jul 2020 17:39:49 -0500 Subject: [PATCH 04/10] oauth scope check test --- pkg/cmd/gist/create/create_test.go | 49 +++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 0b6a021b7..958d09355 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -15,6 +15,10 @@ import ( "github.com/stretchr/testify/assert" ) +const ( + fixtureFile = "../../../../test/fixtures/gistCreate.json" +) + func Test_processFiles(t *testing.T) { fakeStdin := strings.NewReader("hey cool how is it going") files, err := processFiles(ioutil.NopCloser(fakeStdin), []string{"-"}) @@ -160,7 +164,7 @@ func Test_createRun(t *testing.T) { opts: &CreateOptions{ IO: testIO(), Public: true, - Filenames: []string{"../../../../test/fixtures/gistCreate.json"}, + Filenames: []string{fixtureFile}, }, wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", wantErr: false, @@ -178,7 +182,7 @@ func Test_createRun(t *testing.T) { opts: &CreateOptions{ IO: testIO(), Description: "an incredibly interesting gist", - Filenames: []string{"../../../../test/fixtures/gistCreate.json"}, + Filenames: []string{fixtureFile}, }, wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", wantErr: false, @@ -195,7 +199,7 @@ func Test_createRun(t *testing.T) { name: "multiple files", opts: &CreateOptions{ IO: testIOWithStdin("cool stdin content"), - Filenames: []string{"../../../../test/fixtures/gistCreate.json", "-"}, + Filenames: []string{fixtureFile, "-"}, }, wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", wantErr: false, @@ -229,8 +233,10 @@ func Test_createRun(t *testing.T) { } for _, tt := range tests { reg := &httpmock.Registry{} - reg.Register(httpmock.REST("POST", "gists"), httpmock.StringResponse(` - { "html_url": "https://gist.github.com/aa5a315d61ae9438b18d"}`)) + reg.Register(httpmock.REST("POST", "gists"), + httpmock.JSONResponse(struct { + Html_url string + }{"https://gist.github.com/aa5a315d61ae9438b18d"})) mockClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil @@ -254,3 +260,36 @@ func Test_createRun(t *testing.T) { }) } } + +func Test_CreateRun_reauth(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register(httpmock.REST("POST", "gists"), func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 404, + Request: req, + Header: map[string][]string{ + "X-Oauth-Scopes": []string{"coolScope"}, + }, + Body: ioutil.NopCloser(bytes.NewBufferString("oh no")), + }, nil + }) + + mockClient := func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + + opts := &CreateOptions{ + IO: testIO(), + HttpClient: mockClient, + Filenames: []string{fixtureFile}, + } + + err := createRun(opts) + if err == nil { + t.Fatalf("expected oauth error") + } + + if !strings.Contains(err.Error(), "Please re-authenticate") { + t.Errorf("got unexpected error: %s", err) + } +} From c06eacf4ee663fd19fcdd66faf9eb93ac214cf88 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Jul 2020 17:42:52 -0500 Subject: [PATCH 05/10] remove obsolete ensureScopes --- command/root.go | 40 ---------------------------------------- command/testing.go | 3 --- 2 files changed, 43 deletions(-) diff --git a/command/root.go b/command/root.go index 3c28e53ed..01f7d524a 100644 --- a/command/root.go +++ b/command/root.go @@ -261,46 +261,6 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { return api.NewClient(opts...), nil } -var ensureScopes = func(ctx context.Context, client *api.Client, wantedScopes ...string) (*api.Client, error) { - hasScopes, appID, err := client.HasScopes(wantedScopes...) - if err != nil { - return client, err - } - - if hasScopes { - return client, nil - } - - tokenFromEnv := len(os.Getenv("GITHUB_TOKEN")) > 0 - - if config.IsGitHubApp(appID) && !tokenFromEnv && utils.IsTerminal(os.Stdin) && utils.IsTerminal(os.Stderr) { - cfg, err := ctx.Config() - if err != nil { - return nil, err - } - _, err = config.AuthFlowWithConfig(cfg, defaultHostname, "Notice: additional authorization required") - if err != nil { - return nil, err - } - - reloadedClient, err := apiClientForContext(ctx) - if err != nil { - return client, err - } - return reloadedClient, nil - } else { - fmt.Fprintf(os.Stderr, "Warning: gh now requires %s OAuth scopes.\n", wantedScopes) - fmt.Fprintf(os.Stderr, "Visit https://github.com/settings/tokens and edit your token to enable %s\n", wantedScopes) - if tokenFromEnv { - fmt.Fprintln(os.Stderr, "or generate a new token for the GITHUB_TOKEN environment variable") - } else { - fmt.Fprintln(os.Stderr, "or generate a new token and paste it via `gh config set -h github.com oauth_token MYTOKEN`") - } - return client, errors.New("Unable to reauthenticate") - } - -} - func apiVerboseLog() api.ClientOption { logTraffic := strings.Contains(os.Getenv("DEBUG"), "api") colorize := utils.IsTerminal(os.Stderr) diff --git a/command/testing.go b/command/testing.go index d9599e9b3..0b68a4906 100644 --- a/command/testing.go +++ b/command/testing.go @@ -97,9 +97,6 @@ func initBlankContext(cfg, repo, branch string) { func initFakeHTTP() *httpmock.Registry { http := &httpmock.Registry{} - ensureScopes = func(ctx context.Context, client *api.Client, wantedScopes ...string) (*api.Client, error) { - return client, nil - } apiClientForContext = func(context.Context) (*api.Client, error) { return api.NewClient(api.ReplaceTripper(http)), nil } From 496ed477c571c7955379154ef6c202fa30b38e88 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Jul 2020 17:47:44 -0500 Subject: [PATCH 06/10] linter appeasement --- pkg/cmd/gist/create/create_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 958d09355..048bdda1d 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -268,7 +268,7 @@ func Test_CreateRun_reauth(t *testing.T) { StatusCode: 404, Request: req, Header: map[string][]string{ - "X-Oauth-Scopes": []string{"coolScope"}, + "X-Oauth-Scopes": {"coolScope"}, }, Body: ioutil.NopCloser(bytes.NewBufferString("oh no")), }, nil From 41f7b054e649b73b12453161e46251ee6be6f6ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jul 2020 14:18:18 +0200 Subject: [PATCH 07/10] Bump golangci-lint to v1.29.0 Trying to address https://github.com/cli/cli/pull/1406/checks?check_run_id=896316941 which I cannot reproduce locally on macOS --- .github/workflows/lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 39f048881..ca2f61ae9 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -29,7 +29,7 @@ jobs: go mod verify go mod download - LINT_VERSION=1.27.0 + LINT_VERSION=1.29.0 curl -fsSL https://github.com/golangci/golangci-lint/releases/download/v${LINT_VERSION}/golangci-lint-${LINT_VERSION}-linux-amd64.tar.gz | \ tar xz --strip-components 1 --wildcards \*/golangci-lint mkdir -p bin && mv golangci-lint bin/ From b195075644f6a7308cafda605ec333b28c7f8228 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jul 2020 14:50:23 +0200 Subject: [PATCH 08/10] Tweak approach to iostreams in tests - avoid typecast to `*bytes.Buffer` - assert that stderr is empty --- pkg/cmd/gist/create/create_test.go | 31 +++++++++++++----------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 048bdda1d..17bcf3773 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -140,21 +140,11 @@ func TestNewCmdCreate(t *testing.T) { } } -func testIOWithStdin(stdinContent string) *iostreams.IOStreams { - tio, stdin, _, _ := iostreams.Test() - stdin.WriteString(stdinContent) - return tio -} - -func testIO() *iostreams.IOStreams { - tio, _, _, _ := iostreams.Test() - return tio -} - func Test_createRun(t *testing.T) { tests := []struct { name string opts *CreateOptions + stdin string wantOut string wantParams map[string]interface{} wantErr bool @@ -162,7 +152,6 @@ func Test_createRun(t *testing.T) { { name: "public", opts: &CreateOptions{ - IO: testIO(), Public: true, Filenames: []string{fixtureFile}, }, @@ -180,7 +169,6 @@ func Test_createRun(t *testing.T) { { name: "with description", opts: &CreateOptions{ - IO: testIO(), Description: "an incredibly interesting gist", Filenames: []string{fixtureFile}, }, @@ -198,9 +186,9 @@ func Test_createRun(t *testing.T) { { name: "multiple files", opts: &CreateOptions{ - IO: testIOWithStdin("cool stdin content"), Filenames: []string{fixtureFile, "-"}, }, + stdin: "cool stdin content", wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", wantErr: false, wantParams: map[string]interface{}{ @@ -217,9 +205,9 @@ func Test_createRun(t *testing.T) { { name: "stdin arg", opts: &CreateOptions{ - IO: testIOWithStdin("cool stdin content"), Filenames: []string{"-"}, }, + stdin: "cool stdin content", wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", wantErr: false, wantParams: map[string]interface{}{ @@ -241,10 +229,14 @@ func Test_createRun(t *testing.T) { mockClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } - tt.opts.HttpClient = mockClient + io, stdin, stdout, stderr := iostreams.Test() + tt.opts.IO = io + t.Run(tt.name, func(t *testing.T) { + stdin.WriteString(tt.stdin) + if err := createRun(tt.opts); (err != nil) != tt.wantErr { t.Errorf("createRun() error = %v, wantErr %v", err, tt.wantErr) } @@ -254,7 +246,8 @@ func Test_createRun(t *testing.T) { if err != nil { t.Fatalf("error decoding JSON: %v", err) } - assert.Equal(t, tt.wantOut, tt.opts.IO.Out.(*bytes.Buffer).String()) + assert.Equal(t, tt.wantOut, stdout.String()) + assert.Equal(t, "", stderr.String()) assert.Equal(t, tt.wantParams, reqBody) reg.Verify(t) }) @@ -278,8 +271,10 @@ func Test_CreateRun_reauth(t *testing.T) { return &http.Client{Transport: reg}, nil } + io, _, _, _ := iostreams.Test() + opts := &CreateOptions{ - IO: testIO(), + IO: io, HttpClient: mockClient, Filenames: []string{fixtureFile}, } From 16739c30448daa2c107be4f8dd71e456c27bba3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jul 2020 14:59:35 +0200 Subject: [PATCH 09/10] Whoops, `gist create` stderr is not empty --- pkg/cmd/gist/create/create_test.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 17bcf3773..f5510501e 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -146,6 +146,7 @@ func Test_createRun(t *testing.T) { opts *CreateOptions stdin string wantOut string + wantStderr string wantParams map[string]interface{} wantErr bool }{ @@ -155,8 +156,9 @@ func Test_createRun(t *testing.T) { Public: true, Filenames: []string{fixtureFile}, }, - wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", - wantErr: false, + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantStderr: "- Creating gist...\n✓ Created gist\n", + wantErr: false, wantParams: map[string]interface{}{ "public": true, "files": map[string]interface{}{ @@ -172,8 +174,9 @@ func Test_createRun(t *testing.T) { Description: "an incredibly interesting gist", Filenames: []string{fixtureFile}, }, - wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", - wantErr: false, + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantStderr: "- Creating gist...\n✓ Created gist\n", + wantErr: false, wantParams: map[string]interface{}{ "description": "an incredibly interesting gist", "files": map[string]interface{}{ @@ -188,9 +191,10 @@ func Test_createRun(t *testing.T) { opts: &CreateOptions{ Filenames: []string{fixtureFile, "-"}, }, - stdin: "cool stdin content", - wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", - wantErr: false, + stdin: "cool stdin content", + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantStderr: "- Creating gist...\n✓ Created gist\n", + wantErr: false, wantParams: map[string]interface{}{ "files": map[string]interface{}{ "gistCreate.json": map[string]interface{}{ @@ -207,9 +211,10 @@ func Test_createRun(t *testing.T) { opts: &CreateOptions{ Filenames: []string{"-"}, }, - stdin: "cool stdin content", - wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", - wantErr: false, + stdin: "cool stdin content", + wantOut: "https://gist.github.com/aa5a315d61ae9438b18d\n", + wantStderr: "- Creating gist...\n✓ Created gist\n", + wantErr: false, wantParams: map[string]interface{}{ "files": map[string]interface{}{ "gistfile0.txt": map[string]interface{}{ @@ -247,7 +252,7 @@ func Test_createRun(t *testing.T) { t.Fatalf("error decoding JSON: %v", err) } assert.Equal(t, tt.wantOut, stdout.String()) - assert.Equal(t, "", stderr.String()) + assert.Equal(t, tt.wantStderr, stderr.String()) assert.Equal(t, tt.wantParams, reqBody) reg.Verify(t) }) From 05419e46f02e49e2e7873978a8ac0ea97bb6d9b6 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Jul 2020 10:36:12 -0500 Subject: [PATCH 10/10] put gist fixture file into gist package --- pkg/cmd/gist/create/create_test.go | 8 ++++---- test/fixtures/gistCreate.json => pkg/cmd/gist/fixture.txt | 0 2 files changed, 4 insertions(+), 4 deletions(-) rename test/fixtures/gistCreate.json => pkg/cmd/gist/fixture.txt (100%) diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index f5510501e..686e543ed 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -16,7 +16,7 @@ import ( ) const ( - fixtureFile = "../../../../test/fixtures/gistCreate.json" + fixtureFile = "../fixture.txt" ) func Test_processFiles(t *testing.T) { @@ -162,7 +162,7 @@ func Test_createRun(t *testing.T) { wantParams: map[string]interface{}{ "public": true, "files": map[string]interface{}{ - "gistCreate.json": map[string]interface{}{ + "fixture.txt": map[string]interface{}{ "content": "{}", }, }, @@ -180,7 +180,7 @@ func Test_createRun(t *testing.T) { wantParams: map[string]interface{}{ "description": "an incredibly interesting gist", "files": map[string]interface{}{ - "gistCreate.json": map[string]interface{}{ + "fixture.txt": map[string]interface{}{ "content": "{}", }, }, @@ -197,7 +197,7 @@ func Test_createRun(t *testing.T) { wantErr: false, wantParams: map[string]interface{}{ "files": map[string]interface{}{ - "gistCreate.json": map[string]interface{}{ + "fixture.txt": map[string]interface{}{ "content": "{}", }, "gistfile1.txt": map[string]interface{}{ diff --git a/test/fixtures/gistCreate.json b/pkg/cmd/gist/fixture.txt similarity index 100% rename from test/fixtures/gistCreate.json rename to pkg/cmd/gist/fixture.txt