diff --git a/.github/PULL_REQUEST_TEMPLATE/bug_fix.md b/.github/PULL_REQUEST_TEMPLATE/bug_fix.md index 86e73d76a..ca33dd34c 100644 --- a/.github/PULL_REQUEST_TEMPLATE/bug_fix.md +++ b/.github/PULL_REQUEST_TEMPLATE/bug_fix.md @@ -1,7 +1,13 @@ +--- +name: "\U0001F41B Bug fix" +about: Fix a bug in GitHub CLI + +--- + ## Summary diff --git a/api/client.go b/api/client.go index b8c6857fb..1c8c1eaa2 100644 --- a/api/client.go +++ b/api/client.go @@ -35,7 +35,11 @@ func NewClient(opts ...ClientOption) *Client { func AddHeader(name, value string) ClientOption { return func(tr http.RoundTripper) http.RoundTripper { return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { - req.Header.Add(name, value) + // prevent the token from leaking to non-GitHub hosts + // TODO: GHE support + if !strings.EqualFold(name, "Authorization") || strings.HasSuffix(req.URL.Hostname(), ".github.com") { + req.Header.Add(name, value) + } return tr.RoundTrip(req) }} } @@ -45,7 +49,11 @@ func AddHeader(name, value string) ClientOption { func AddHeaderFunc(name string, value func() string) ClientOption { return func(tr http.RoundTripper) http.RoundTripper { return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { - req.Header.Add(name, value()) + // prevent the token from leaking to non-GitHub hosts + // TODO: GHE support + if !strings.EqualFold(name, "Authorization") || strings.HasSuffix(req.URL.Hostname(), ".github.com") { + req.Header.Add(name, value()) + } return tr.RoundTrip(req) }} } diff --git a/command/help.go b/command/help.go new file mode 100644 index 000000000..ad3561af2 --- /dev/null +++ b/command/help.go @@ -0,0 +1,119 @@ +package command + +import ( + "fmt" + "strings" + + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +func rootHelpFunc(command *cobra.Command, args []string) { + // Display helpful error message in case subcommand name was mistyped. + // This matches Cobra's behavior for root command, which Cobra + // confusingly doesn't apply to nested commands. + if command != RootCmd { + if command.Parent() == RootCmd && len(args) >= 2 { + if command.SuggestionsMinimumDistance <= 0 { + command.SuggestionsMinimumDistance = 2 + } + candidates := command.SuggestionsFor(args[1]) + + errOut := command.OutOrStderr() + fmt.Fprintf(errOut, "unknown command %q for %q\n", args[1], "gh "+args[0]) + + if len(candidates) > 0 { + fmt.Fprint(errOut, "\nDid you mean this?\n") + for _, c := range candidates { + fmt.Fprintf(errOut, "\t%s\n", c) + } + fmt.Fprint(errOut, "\n") + } + + oldOut := command.OutOrStdout() + command.SetOut(errOut) + defer command.SetOut(oldOut) + } + } + + coreCommands := []string{} + additionalCommands := []string{} + for _, c := range command.Commands() { + if c.Short == "" { + continue + } + if c.Hidden { + continue + } + + s := rpad(c.Name()+":", c.NamePadding()) + c.Short + if _, ok := c.Annotations["IsCore"]; ok { + coreCommands = append(coreCommands, s) + } else { + additionalCommands = append(additionalCommands, s) + } + } + + // If there are no core commands, assume everything is a core command + if len(coreCommands) == 0 { + coreCommands = additionalCommands + additionalCommands = []string{} + } + + type helpEntry struct { + Title string + Body string + } + + helpEntries := []helpEntry{} + if command.Long != "" { + helpEntries = append(helpEntries, helpEntry{"", command.Long}) + } else if command.Short != "" { + helpEntries = append(helpEntries, helpEntry{"", command.Short}) + } + helpEntries = append(helpEntries, helpEntry{"USAGE", command.UseLine()}) + if len(coreCommands) > 0 { + helpEntries = append(helpEntries, helpEntry{"CORE COMMANDS", strings.Join(coreCommands, "\n")}) + } + if len(additionalCommands) > 0 { + helpEntries = append(helpEntries, helpEntry{"ADDITIONAL COMMANDS", strings.Join(additionalCommands, "\n")}) + } + if command.HasLocalFlags() { + helpEntries = append(helpEntries, helpEntry{"FLAGS", strings.TrimRight(command.LocalFlags().FlagUsages(), "\n")}) + } + if _, ok := command.Annotations["help:arguments"]; ok { + helpEntries = append(helpEntries, helpEntry{"ARGUMENTS", command.Annotations["help:arguments"]}) + } + if command.Example != "" { + helpEntries = append(helpEntries, helpEntry{"EXAMPLES", command.Example}) + } + helpEntries = append(helpEntries, helpEntry{"LEARN MORE", ` +Use "gh --help" for more information about a command. +Read the manual at http://cli.github.com/manual`}) + if _, ok := command.Annotations["help:feedback"]; ok { + helpEntries = append(helpEntries, helpEntry{"FEEDBACK", command.Annotations["help:feedback"]}) + } + + out := colorableOut(command) + for _, e := range helpEntries { + if e.Title != "" { + // If there is a title, add indentation to each line in the body + fmt.Fprintln(out, utils.Bold(e.Title)) + + for _, l := range strings.Split(strings.Trim(e.Body, "\n\r"), "\n") { + l = strings.Trim(l, " \n\r") + fmt.Fprintln(out, " "+l) + } + } else { + // If there is no title print the body as is + fmt.Fprintln(out, e.Body) + } + fmt.Fprintln(out) + } +} + +// rpad adds padding to the right of a string. +func rpad(s string, padding int) string { + template := fmt.Sprintf("%%-%ds ", padding) + return fmt.Sprintf(template, s) +} diff --git a/command/issue.go b/command/issue.go index 305b6f832..d630b340c 100644 --- a/command/issue.go +++ b/command/issue.go @@ -49,13 +49,17 @@ func init() { } var issueCmd = &cobra.Command{ - Use: "issue", + Use: "issue ", Short: "Create and view issues", - Long: `Work with GitHub issues. - -An issue can be supplied as argument in any of the following formats: + Long: `Work with GitHub issues`, + Example: `$ gh issue list +$ gh issue create --fill +$ gh issue view --web`, + Annotations: map[string]string{ + "IsCore": "true", + "help:arguments": `An issue can be supplied as argument in any of the following formats: - by number, e.g. "123"; or -- by URL, e.g. "https://github.com/OWNER/REPO/issues/123".`, +- by URL, e.g. "https://github.com/OWNER/REPO/issues/123".`}, } var issueCreateCmd = &cobra.Command{ Use: "create", @@ -351,11 +355,11 @@ func issueCreate(cmd *cobra.Command, args []string) error { return err } - var templateFiles []string + var nonLegacyTemplateFiles []string if baseOverride == "" { if rootDir, err := git.ToplevelDir(); err == nil { // TODO: figure out how to stub this in tests - templateFiles = githubtemplate.Find(rootDir, "ISSUE_TEMPLATE") + nonLegacyTemplateFiles = githubtemplate.FindNonLegacy(rootDir, "ISSUE_TEMPLATE") } } @@ -399,7 +403,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { if err != nil { return err } - } else if len(templateFiles) > 1 { + } else if len(nonLegacyTemplateFiles) > 1 { openURL += "/choose" } cmd.Printf("Opening %s in your browser.\n", displayURL(openURL)) @@ -418,6 +422,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { action := SubmitAction tb := issueMetadataState{ + Type: issueMetadata, Assignees: assignees, Labels: labelNames, Projects: projectNames, @@ -427,7 +432,14 @@ func issueCreate(cmd *cobra.Command, args []string) error { interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) if interactive { - err := titleBodySurvey(cmd, &tb, apiClient, baseRepo, title, body, defaults{}, templateFiles, false, repo.ViewerCanTriage()) + var legacyTemplateFile *string + if baseOverride == "" { + if rootDir, err := git.ToplevelDir(); err == nil { + // TODO: figure out how to stub this in tests + legacyTemplateFile = githubtemplate.FindLegacy(rootDir, "ISSUE_TEMPLATE") + } + } + err := titleBodySurvey(cmd, &tb, apiClient, baseRepo, title, body, defaults{}, nonLegacyTemplateFiles, legacyTemplateFile, false, repo.ViewerCanTriage()) if err != nil { return fmt.Errorf("could not collect title and/or body: %w", err) } diff --git a/command/pr.go b/command/pr.go index e92fa7cf0..dce006bf0 100644 --- a/command/pr.go +++ b/command/pr.go @@ -46,19 +46,27 @@ func init() { } var prCmd = &cobra.Command{ - Use: "pr", + Use: "pr ", Short: "Create, view, and checkout pull requests", - Long: `Work with GitHub pull requests. - -A pull request can be supplied as argument in any of the following formats: + Long: `Work with GitHub pull requests`, + Example: `$ gh pr checkout 353 +$ gh pr checkout bug-fix-branch +$ gh pr create --fill +$ gh pr view --web`, + Annotations: map[string]string{ + "IsCore": "true", + "help:arguments": `A pull request can be supplied as argument in any of the following formats: - by number, e.g. "123"; - by URL, e.g. "https://github.com/OWNER/REPO/pull/123"; or -- by the name of its head branch, e.g. "patch-1" or "OWNER:patch-1".`, +- by the name of its head branch, e.g. "patch-1" or "OWNER:patch-1".`}, } var prListCmd = &cobra.Command{ Use: "list", Short: "List and filter pull requests in this repository", - RunE: prList, + Example: `$ gh pr list --limit all +$ gh pr list --state closed +$ gh pr list --label “priority 1” “bug”`, + RunE: prList, } var prStatusCmd = &cobra.Command{ Use: "status", diff --git a/command/pr_create.go b/command/pr_create.go index 45ec801d4..608656f4b 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -40,8 +40,8 @@ func computeDefaults(baseRef, headRef string) (defaults, error) { out.Title = utils.Humanize(headRef) body := "" - for _, c := range commits { - body += fmt.Sprintf("- %s\n", c.Title) + for i := len(commits) - 1; i >= 0; i-- { + body += fmt.Sprintf("- %s\n", commits[i].Title) } out.Body = body } @@ -203,6 +203,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } tb := issueMetadataState{ + Type: prMetadata, Reviewers: reviewers, Assignees: assignees, Labels: labelNames, @@ -213,13 +214,14 @@ func prCreate(cmd *cobra.Command, _ []string) error { interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) if !isWeb && !autofill && interactive { - var templateFiles []string + var nonLegacyTemplateFiles []string + var legacyTemplateFile *string if rootDir, err := git.ToplevelDir(); err == nil { // TODO: figure out how to stub this in tests - templateFiles = githubtemplate.Find(rootDir, "PULL_REQUEST_TEMPLATE") + nonLegacyTemplateFiles = githubtemplate.FindNonLegacy(rootDir, "PULL_REQUEST_TEMPLATE") + legacyTemplateFile = githubtemplate.FindLegacy(rootDir, "PULL_REQUEST_TEMPLATE") } - - err := titleBodySurvey(cmd, &tb, client, baseRepo, title, body, defs, templateFiles, true, baseRepo.ViewerCanTriage()) + err := titleBodySurvey(cmd, &tb, client, baseRepo, title, body, defs, nonLegacyTemplateFiles, legacyTemplateFile, true, baseRepo.ViewerCanTriage()) if err != nil { return fmt.Errorf("could not collect title and/or body: %w", err) } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index e13ecc15f..2eb0872c7 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -519,7 +519,7 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - expectedBody := "- commit 0\n- commit 1\n" + expectedBody := "- commit 1\n- commit 0\n" eq(t, reqBody.Variables.Input.RepositoryID, "REPOID") eq(t, reqBody.Variables.Input.Title, "cool bug fixes") diff --git a/command/pr_review.go b/command/pr_review.go index 642c9ac24..37b4e09c0 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -180,7 +180,7 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { } bodyQs := []*survey.Question{ - &survey.Question{ + { Name: "body", Prompt: &surveyext.GhEditor{ BlankAllowed: blankAllowed, diff --git a/command/pr_review_test.go b/command/pr_review_test.go index 612f6c574..2c2673db4 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -213,10 +213,10 @@ func TestPRReview(t *testing.T) { ExpectedBody string } cases := []c{ - c{`pr review --request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, - c{`pr review --approve`, "APPROVE", ""}, - c{`pr review --approve -b"hot damn"`, "APPROVE", "hot damn"}, - c{`pr review --comment --body "i donno"`, "COMMENT", "i donno"}, + {`pr review --request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, + {`pr review --approve`, "APPROVE", ""}, + {`pr review --approve -b"hot damn"`, "APPROVE", "hot damn"}, + {`pr review --comment --body "i donno"`, "COMMENT", "i donno"}, } for _, kase := range cases { diff --git a/command/repo.go b/command/repo.go index b1a57d3a7..d62f0c715 100644 --- a/command/repo.go +++ b/command/repo.go @@ -44,13 +44,19 @@ func init() { } var repoCmd = &cobra.Command{ - Use: "repo", + Use: "repo ", Short: "Create, clone, fork, and view repositories", - Long: `Work with GitHub repositories. - + Long: `Work with GitHub repositories`, + Example: `$ gh repo create +$ gh repo clone cli/cli +$ gh repo view --web +`, + Annotations: map[string]string{ + "IsCore": "true", + "help:arguments": ` A repository can be supplied as an argument in any of the following formats: - "OWNER/REPO" -- by URL, e.g. "https://github.com/OWNER/REPO"`, +- by URL, e.g. "https://github.com/OWNER/REPO"`}, } var repoCloneCmd = &cobra.Command{ @@ -69,9 +75,18 @@ To pass 'git clone' flags, separate them with '--'.`, var repoCreateCmd = &cobra.Command{ Use: "create []", Short: "Create a new repository", - Long: `Create a new GitHub repository. + Long: `Create a new GitHub repository`, + Example: utils.Bold("$ gh repo create") + ` +Will create a repository on your account using the name of your current directory -Use the "ORG/NAME" syntax to create a repository within your organization.`, +` + utils.Bold("$ gh repo create my-project") + ` +Will create a repository on your account using the name 'my-project' + +` + utils.Bold("$ gh repo create cli/my-project") + ` +Will create a repository in the organization 'cli' using the name 'my-project'`, + Annotations: map[string]string{"help:arguments": `A repository can be supplied as an argument in any of the following formats: +- +- by URL, e.g. "https://github.com/OWNER/REPO"`}, RunE: repoCreate, } diff --git a/command/repo_test.go b/command/repo_test.go index ff882d12e..52928c8f4 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -1,7 +1,6 @@ package command import ( - "bytes" "encoding/json" "io/ioutil" "os/exec" @@ -459,11 +458,13 @@ func TestRepoClone(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "parent": null - } } } - `)) + http.Register( + httpmock.GraphQL(`\brepository\(`), + httpmock.StringResponse(` + { "data": { "repository": { + "parent": null + } } } + `)) cs, restore := test.InitCmdStubber() defer restore() @@ -485,14 +486,16 @@ func TestRepoClone(t *testing.T) { func TestRepoClone_hasParent(t *testing.T) { http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "parent": { - "owner": {"login": "hubot"}, - "name": "ORIG" - } - } } } - `)) + http.Register( + httpmock.GraphQL(`\brepository\(`), + httpmock.StringResponse(` + { "data": { "repository": { + "parent": { + "owner": {"login": "hubot"}, + "name": "ORIG" + } + } } } + `)) cs, restore := test.InitCmdStubber() defer restore() @@ -548,18 +551,19 @@ func TestRepoCreate(t *testing.T) { } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "createRepository": { - "repository": { - "id": "REPOID", - "url": "https://github.com/OWNER/REPO", - "name": "REPO", - "owner": { - "login": "OWNER" + http.Register( + httpmock.GraphQL(`\bcreateRepository\(`), + httpmock.StringResponse(` + { "data": { "createRepository": { + "repository": { + "id": "REPOID", + "url": "https://github.com/OWNER/REPO", + "name": "REPO", + "owner": { + "login": "OWNER" + } } - } - } } } - `)) + } } }`)) var seenCmd *exec.Cmd restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -613,22 +617,24 @@ func TestRepoCreate_org(t *testing.T) { } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { "node_id": "ORGID" - } - `)) - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "createRepository": { - "repository": { - "id": "REPOID", - "url": "https://github.com/ORG/REPO", - "name": "REPO", - "owner": { - "login": "ORG" + http.Register( + httpmock.MatchAny, + httpmock.StringResponse(` + { "node_id": "ORGID" + }`)) + http.Register( + httpmock.GraphQL(`\bcreateRepository\(`), + httpmock.StringResponse(` + { "data": { "createRepository": { + "repository": { + "id": "REPOID", + "url": "https://github.com/ORG/REPO", + "name": "REPO", + "owner": { + "login": "ORG" + } } - } - } } } - `)) + } } }`)) var seenCmd *exec.Cmd restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -681,23 +687,25 @@ func TestRepoCreate_orgWithTeam(t *testing.T) { } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { "node_id": "TEAMID", - "organization": { "node_id": "ORGID" } - } - `)) - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "createRepository": { - "repository": { - "id": "REPOID", - "url": "https://github.com/ORG/REPO", - "name": "REPO", - "owner": { - "login": "ORG" + http.Register( + httpmock.MatchAny, + httpmock.StringResponse(` + { "node_id": "TEAMID", + "organization": { "node_id": "ORGID" } + }`)) + http.Register( + httpmock.GraphQL(`\bcreateRepository\(`), + httpmock.StringResponse(` + { "data": { "createRepository": { + "repository": { + "id": "REPOID", + "url": "https://github.com/ORG/REPO", + "name": "REPO", + "owner": { + "login": "ORG" + } } - } - } } } - `)) + } } }`)) var seenCmd *exec.Cmd restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -746,9 +754,10 @@ func TestRepoView_web(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { } - `)) + http.Register( + httpmock.MatchAny, + httpmock.StringResponse(` + { }`)) var seenCmd *exec.Cmd restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -779,9 +788,10 @@ func TestRepoView_web_ownerRepo(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { } - `)) + http.Register( + httpmock.MatchAny, + httpmock.StringResponse(` + { }`)) var seenCmd *exec.Cmd restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -812,9 +822,10 @@ func TestRepoView_web_fullURL(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { } - `)) + http.Register( + httpmock.MatchAny, + httpmock.StringResponse(` + { }`)) var seenCmd *exec.Cmd restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd @@ -841,16 +852,18 @@ func TestRepoView(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register( + httpmock.GraphQL(`\brepository\(`), + httpmock.StringResponse(` { "data": { - "repository": { - "description": "social distancing" - }}} - `)) - http.StubResponse(200, bytes.NewBufferString(` + "repository": { + "description": "social distancing" + } } }`)) + http.Register( + httpmock.MatchAny, + httpmock.StringResponse(` { "name": "readme.md", - "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="} - `)) + "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) output, err := RunCommand("repo view") if err != nil { @@ -869,16 +882,18 @@ func TestRepoView_nonmarkdown_readme(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register( + httpmock.GraphQL(`\brepository\(`), + httpmock.StringResponse(` { "data": { "repository": { - "description": "social distancing" - }}} - `)) - http.StubResponse(200, bytes.NewBufferString(` + "description": "social distancing" + } } }`)) + http.Register( + httpmock.MatchAny, + httpmock.StringResponse(` { "name": "readme.org", - "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="} - `)) + "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) output, err := RunCommand("repo view") if err != nil { @@ -896,8 +911,8 @@ func TestRepoView_blanks(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString("{}")) - http.StubResponse(200, bytes.NewBufferString("{}")) + http.Register(httpmock.MatchAny, httpmock.StringResponse("{}")) + http.Register(httpmock.MatchAny, httpmock.StringResponse("{}")) output, err := RunCommand("repo view") if err != nil { diff --git a/command/root.go b/command/root.go index 05e864eb7..12979ad5f 100644 --- a/command/root.go +++ b/command/root.go @@ -34,7 +34,6 @@ var Version = "DEV" var BuildDate = "" // YYYY-MM-DD var versionOutput = "" -var cobraDefaultHelpFunc func(*cobra.Command, []string) func init() { if Version == "DEV" { @@ -58,9 +57,11 @@ func init() { // TODO: // RootCmd.PersistentFlags().BoolP("verbose", "V", false, "enable verbose output") - cobraDefaultHelpFunc = RootCmd.HelpFunc() RootCmd.SetHelpFunc(rootHelpFunc) + // This will silence the usage func on error + RootCmd.SetUsageFunc(func(_ *cobra.Command) error { return nil }) + RootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { if err == pflag.ErrHelp { return err @@ -95,6 +96,13 @@ var RootCmd = &cobra.Command{ SilenceErrors: true, SilenceUsage: true, + Example: `$ gh issue create +$ gh repo clone +$ gh pr checkout 321`, + Annotations: map[string]string{ + "help:feedback": ` +Fill out our feedback form https://forms.gle/umxd3h31c7aMQFKG7 +Open an issue using “gh issue create -R cli/cli”`}, } var versionCmd = &cobra.Command{ @@ -320,100 +328,6 @@ func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Co return baseRepo, nil } -func rootHelpFunc(command *cobra.Command, args []string) { - if command != RootCmd { - // Display helpful error message in case subcommand name was mistyped. - // This matches Cobra's behavior for root command, which Cobra - // confusingly doesn't apply to nested commands. - if command.Parent() == RootCmd && len(args) >= 2 { - if command.SuggestionsMinimumDistance <= 0 { - command.SuggestionsMinimumDistance = 2 - } - candidates := command.SuggestionsFor(args[1]) - - errOut := command.OutOrStderr() - fmt.Fprintf(errOut, "unknown command %q for %q\n", args[1], "gh "+args[0]) - - if len(candidates) > 0 { - fmt.Fprint(errOut, "\nDid you mean this?\n") - for _, c := range candidates { - fmt.Fprintf(errOut, "\t%s\n", c) - } - fmt.Fprint(errOut, "\n") - } - - oldOut := command.OutOrStdout() - command.SetOut(errOut) - defer command.SetOut(oldOut) - } - cobraDefaultHelpFunc(command, args) - return - } - - type helpEntry struct { - Title string - Body string - } - - coreCommandNames := []string{"issue", "pr", "repo"} - var coreCommands []string - var additionalCommands []string - for _, c := range command.Commands() { - if c.Short == "" { - continue - } - s := " " + rpad(c.Name()+":", c.NamePadding()) + c.Short - if includes(coreCommandNames, c.Name()) { - coreCommands = append(coreCommands, s) - } else if !c.Hidden { - additionalCommands = append(additionalCommands, s) - } - } - - helpEntries := []helpEntry{ - { - "", - command.Long}, - {"USAGE", command.Use}, - {"CORE COMMANDS", strings.Join(coreCommands, "\n")}, - {"ADDITIONAL COMMANDS", strings.Join(additionalCommands, "\n")}, - {"FLAGS", strings.TrimRight(command.LocalFlags().FlagUsages(), "\n")}, - {"EXAMPLES", ` - $ gh issue create - $ gh repo clone - $ gh pr checkout 321`}, - {"LEARN MORE", ` - Use "gh --help" for more information about a command. - Read the manual at http://cli.github.com/manual`}, - {"FEEDBACK", ` - Fill out our feedback form https://forms.gle/umxd3h31c7aMQFKG7 - Open an issue using “gh issue create -R cli/cli”`}, - } - - out := colorableOut(command) - for _, e := range helpEntries { - if e.Title != "" { - fmt.Fprintln(out, utils.Bold(e.Title)) - } - fmt.Fprintln(out, strings.TrimLeft(e.Body, "\n")+"\n") - } -} - -// rpad adds padding to the right of a string. -func rpad(s string, padding int) string { - template := fmt.Sprintf("%%-%ds ", padding) - return fmt.Sprintf(template, s) -} - -func includes(a []string, s string) bool { - for _, x := range a { - if x == s { - return true - } - } - return false -} - func formatRemoteURL(cmd *cobra.Command, fullRepoName string) string { ctx := contextForCommand(cmd) diff --git a/command/title_body_survey.go b/command/title_body_survey.go index 2b8bdda3a..d6fe11bbf 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -13,8 +13,16 @@ import ( ) type Action int +type metadataStateType int + +const ( + issueMetadata metadataStateType = iota + prMetadata +) type issueMetadataState struct { + Type metadataStateType + Body string Title string Action Action @@ -99,35 +107,46 @@ func confirmSubmission(allowPreview bool, allowMetadata bool) (Action, error) { } } -func selectTemplate(templatePaths []string) (string, error) { +func selectTemplate(nonLegacyTemplatePaths []string, legacyTemplatePath *string, metadataType metadataStateType) (string, error) { templateResponse := struct { Index int }{} - if len(templatePaths) > 1 { - templateNames := make([]string, 0, len(templatePaths)) - for _, p := range templatePaths { - templateNames = append(templateNames, githubtemplate.ExtractName(p)) - } - - selectQs := []*survey.Question{ - { - Name: "index", - Prompt: &survey.Select{ - Message: "Choose a template", - Options: templateNames, - }, - }, - } - if err := SurveyAsk(selectQs, &templateResponse); err != nil { - return "", fmt.Errorf("could not prompt: %w", err) - } + templateNames := make([]string, 0, len(nonLegacyTemplatePaths)) + for _, p := range nonLegacyTemplatePaths { + templateNames = append(templateNames, githubtemplate.ExtractName(p)) + } + if metadataType == issueMetadata { + templateNames = append(templateNames, "Open a blank issue") + } else if metadataType == prMetadata { + templateNames = append(templateNames, "Open a blank pull request") } - templateContents := githubtemplate.ExtractContents(templatePaths[templateResponse.Index]) + selectQs := []*survey.Question{ + { + Name: "index", + Prompt: &survey.Select{ + Message: "Choose a template", + Options: templateNames, + }, + }, + } + if err := SurveyAsk(selectQs, &templateResponse); err != nil { + return "", fmt.Errorf("could not prompt: %w", err) + } + + if templateResponse.Index == len(nonLegacyTemplatePaths) { // the user has selected the blank template + if legacyTemplatePath != nil { + templateContents := githubtemplate.ExtractContents(*legacyTemplatePath) + return string(templateContents), nil + } else { + return "", nil + } + } + templateContents := githubtemplate.ExtractContents(nonLegacyTemplatePaths[templateResponse.Index]) return string(templateContents), nil } -func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClient *api.Client, repo ghrepo.Interface, providedTitle, providedBody string, defs defaults, templatePaths []string, allowReviewers, allowMetadata bool) error { +func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClient *api.Client, repo ghrepo.Interface, providedTitle, providedBody string, defs defaults, nonLegacyTemplatePaths []string, legacyTemplatePath *string, allowReviewers, allowMetadata bool) error { editorCommand, err := determineEditor(cmd) if err != nil { return err @@ -137,13 +156,15 @@ func titleBodySurvey(cmd *cobra.Command, issueState *issueMetadataState, apiClie templateContents := "" if providedBody == "" { - if len(templatePaths) > 0 { + if len(nonLegacyTemplatePaths) > 0 { var err error - templateContents, err = selectTemplate(templatePaths) + templateContents, err = selectTemplate(nonLegacyTemplatePaths, legacyTemplatePath, issueState.Type) if err != nil { return err } issueState.Body = templateContents + } else if legacyTemplatePath != nil { + issueState.Body = string(githubtemplate.ExtractContents(*legacyTemplatePath)) } else { issueState.Body = defs.Body } diff --git a/docs/triage.md b/docs/triage.md new file mode 100644 index 000000000..95e1bf852 --- /dev/null +++ b/docs/triage.md @@ -0,0 +1,62 @@ +# Triage role + +As we get more issues and pull requests opened on the GitHub CLI, we've decided on a weekly rotation +triage role. The initial expectation is that the person in the role for the week spends no more than +1-2 hours a day on this work; we can refine that as needed. + +# Incoming issues + +just imagine a flowchart + +- can this be closed outright? + - e.g. spam/junk + - close without comment +- do we not want to do it? + - e.g. have already discussed not wanting to do or duplicate issue + - comment acknowledging receipt + - close +- do we want to do it? + - e.g. bugs or things we have discussed before + - comment acknowledging it + - label appropriately (examples include `enhancement` or `bug`) + - add to project TODO column if appropriate, otherwise just leave it labeled +- is it intriguing but needs discussion? + - label `needs-design` if design input is needed, ping + - label `needs-investigation` if engineering research is required before action can be taken + - ping engineers if eng needed + - ping product if it's about future directions/roadamp/big changes +- does it need more info from the issue author? + - ask the user for that + - add `needs-user-input` label +- is it a user asking for help and you have all the info you need to help? + - try and help + +# Incoming PRs + +just imagine a flowchart + +- can it be closed outright? + - ie spam/junk +- do we not want to do it? + - ie have already discussed not wanting to do, duplicate issue + - comment acknowledging receipt + - close +- is it intriguing but needs discussion? + - request an issue + - close +- is it something we want to include? + - add `community` label + - add to `needs review` column + +# Weekly PR audit + +In the interest of not letting our open PR list get out of hand (20+ total PRs _or_ multiple PRs +over a few months old), try to audit open PRs each week with the goal of getting them merged and/or +closed. It's likely too much work to deal with every PR, but even getting a few closer to done is +helpful. + +For each PR, ask: + +- is this too stale? close with comment +- is this really close but author is absent? push commits to finish, request review +- is this waiting on triage? go through the PR triage flow diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 4158ff8c1..b4a8dbdff 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -1,16 +1,21 @@ package api import ( + "bytes" + "encoding/json" "fmt" "io" "io/ioutil" "net/http" "os" + "regexp" + "sort" "strconv" "strings" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/jsoncolor" "github.com/spf13/cobra" ) @@ -20,6 +25,7 @@ type ApiOptions struct { RequestMethod string RequestMethodPassed bool RequestPath string + RequestInputFile string MagicFields []string RawFields []string RequestHeaders []string @@ -55,6 +61,10 @@ on the format of the value: appropriate JSON types; - if the value starts with "@", the rest of the value is interpreted as a filename to read the value from. Pass "-" to read from standard input. + +Raw request body may be passed from the outside via a file specified by '--input'. +Pass "-" to read from standard input. In this mode, parameters specified via +'--field' flags are serialized into URL query parameters. `, Args: cobra.ExactArgs(1), RunE: func(c *cobra.Command, args []string) error { @@ -73,6 +83,7 @@ on the format of the value: cmd.Flags().StringArrayVarP(&opts.RawFields, "raw-field", "f", nil, "Add a string parameter") cmd.Flags().StringArrayVarP(&opts.RequestHeaders, "header", "H", nil, "Add an additional HTTP request header") cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response headers in the output") + cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The file to use as body for the HTTP request") return cmd } @@ -83,45 +94,102 @@ func apiRun(opts *ApiOptions) error { } method := opts.RequestMethod - if len(params) > 0 && !opts.RequestMethodPassed { + requestPath := opts.RequestPath + requestHeaders := opts.RequestHeaders + var requestBody interface{} = params + + if !opts.RequestMethodPassed && (len(params) > 0 || opts.RequestInputFile != "") { method = "POST" } + if opts.RequestInputFile != "" { + file, size, err := openUserFile(opts.RequestInputFile, opts.IO.In) + if err != nil { + return err + } + defer file.Close() + requestPath = addQuery(requestPath, params) + requestBody = file + if size >= 0 { + requestHeaders = append([]string{fmt.Sprintf("Content-Length: %d", size)}, requestHeaders...) + } + } + httpClient, err := opts.HttpClient() if err != nil { return err } - resp, err := httpRequest(httpClient, method, opts.RequestPath, params, opts.RequestHeaders) + resp, err := httpRequest(httpClient, method, requestPath, requestBody, requestHeaders) if err != nil { return err } if opts.ShowResponseHeaders { - for name, vals := range resp.Header { - fmt.Fprintf(opts.IO.Out, "%s: %s\r\n", name, strings.Join(vals, ", ")) - } + fmt.Fprintln(opts.IO.Out, resp.Proto, resp.Status) + printHeaders(opts.IO.Out, resp.Header, opts.IO.ColorEnabled()) fmt.Fprint(opts.IO.Out, "\r\n") } if resp.StatusCode == 204 { return nil } + var responseBody io.Reader = resp.Body defer resp.Body.Close() - _, err = io.Copy(opts.IO.Out, resp.Body) - if err != nil { - return err + isJSON, _ := regexp.MatchString(`[/+]json(;|$)`, resp.Header.Get("Content-Type")) + + var serverError string + if isJSON && (opts.RequestPath == "graphql" || resp.StatusCode >= 400) { + responseBody, serverError, err = parseErrorResponse(responseBody, resp.StatusCode) + if err != nil { + return err + } } - // TODO: detect GraphQL errors - if resp.StatusCode > 299 { + if isJSON && opts.IO.ColorEnabled() { + err = jsoncolor.Write(opts.IO.Out, responseBody, " ") + if err != nil { + return err + } + } else { + _, err = io.Copy(opts.IO.Out, responseBody) + if err != nil { + return err + } + } + + if serverError != "" { + fmt.Fprintf(opts.IO.ErrOut, "gh: %s\n", serverError) + return cmdutil.SilentError + } else if resp.StatusCode > 299 { + fmt.Fprintf(opts.IO.ErrOut, "gh: HTTP %d\n", resp.StatusCode) return cmdutil.SilentError } return nil } +func printHeaders(w io.Writer, headers http.Header, colorize bool) { + var names []string + for name := range headers { + if name == "Status" { + continue + } + names = append(names, name) + } + sort.Strings(names) + + var headerColor, headerColorReset string + if colorize { + headerColor = "\x1b[1;34m" // bright blue + headerColorReset = "\x1b[m" + } + for _, name := range names { + fmt.Fprintf(w, "%s%s%s: %s\r\n", headerColor, name, headerColorReset, strings.Join(headers[name], ", ")) + } +} + func parseFields(opts *ApiOptions) (map[string]interface{}, error) { params := make(map[string]interface{}) for _, f := range opts.RawFields { @@ -188,3 +256,52 @@ func readUserFile(fn string, stdin io.ReadCloser) ([]byte, error) { defer r.Close() return ioutil.ReadAll(r) } + +func openUserFile(fn string, stdin io.ReadCloser) (io.ReadCloser, int64, error) { + if fn == "-" { + return stdin, -1, nil + } + + r, err := os.Open(fn) + if err != nil { + return r, -1, err + } + + s, err := os.Stat(fn) + if err != nil { + return r, -1, err + } + + return r, s.Size(), nil +} + +func parseErrorResponse(r io.Reader, statusCode int) (io.Reader, string, error) { + bodyCopy := &bytes.Buffer{} + b, err := ioutil.ReadAll(io.TeeReader(r, bodyCopy)) + if err != nil { + return r, "", err + } + + var parsedBody struct { + Message string + Errors []struct { + Message string + } + } + err = json.Unmarshal(b, &parsedBody) + if err != nil { + return r, "", err + } + + if parsedBody.Message != "" { + return bodyCopy, fmt.Sprintf("%s (HTTP %d)", parsedBody.Message, statusCode), nil + } else if len(parsedBody.Errors) > 0 { + msgs := make([]string, len(parsedBody.Errors)) + for i, e := range parsedBody.Errors { + msgs[i] = e.Message + } + return bodyCopy, strings.Join(msgs, "\n"), nil + } + + return bodyCopy, "", nil +} diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 8149f5906..100c1257e 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -31,6 +31,7 @@ func Test_NewCmdApi(t *testing.T) { RequestMethod: "GET", RequestMethodPassed: false, RequestPath: "graphql", + RequestInputFile: "", RawFields: []string(nil), MagicFields: []string(nil), RequestHeaders: []string(nil), @@ -45,6 +46,7 @@ func Test_NewCmdApi(t *testing.T) { RequestMethod: "DELETE", RequestMethodPassed: true, RequestPath: "repos/octocat/Spoon-Knife", + RequestInputFile: "", RawFields: []string(nil), MagicFields: []string(nil), RequestHeaders: []string(nil), @@ -59,6 +61,7 @@ func Test_NewCmdApi(t *testing.T) { RequestMethod: "GET", RequestMethodPassed: false, RequestPath: "graphql", + RequestInputFile: "", RawFields: []string{"query=QUERY"}, MagicFields: []string{"body=@file.txt"}, RequestHeaders: []string(nil), @@ -73,6 +76,7 @@ func Test_NewCmdApi(t *testing.T) { RequestMethod: "GET", RequestMethodPassed: false, RequestPath: "user", + RequestInputFile: "", RawFields: []string(nil), MagicFields: []string(nil), RequestHeaders: []string{"accept: text/plain"}, @@ -80,6 +84,21 @@ func Test_NewCmdApi(t *testing.T) { }, wantsErr: false, }, + { + name: "with request body from file", + cli: "user --input myfile", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "user", + RequestInputFile: "myfile", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + }, + wantsErr: false, + }, { name: "no arguments", cli: "", @@ -92,6 +111,7 @@ func Test_NewCmdApi(t *testing.T) { assert.Equal(t, tt.wants.RequestMethod, o.RequestMethod) assert.Equal(t, tt.wants.RequestMethodPassed, o.RequestMethodPassed) assert.Equal(t, tt.wants.RequestPath, o.RequestPath) + assert.Equal(t, tt.wants.RequestInputFile, o.RequestInputFile) assert.Equal(t, tt.wants.RawFields, o.RawFields) assert.Equal(t, tt.wants.MagicFields, o.MagicFields) assert.Equal(t, tt.wants.RequestHeaders, o.RequestHeaders) @@ -140,12 +160,14 @@ func Test_apiRun(t *testing.T) { ShowResponseHeaders: true, }, httpResponse: &http.Response{ + Proto: "HTTP/1.1", + Status: "200 Okey-dokey", StatusCode: 200, Body: ioutil.NopCloser(bytes.NewBufferString(`body`)), Header: http.Header{"Content-Type": []string{"text/plain"}}, }, err: nil, - stdout: "Content-Type: text/plain\r\n\r\nbody", + stdout: "HTTP/1.1 200 Okey-dokey\nContent-Type: text/plain\r\n\r\nbody", stderr: ``, }, { @@ -158,6 +180,31 @@ func Test_apiRun(t *testing.T) { stdout: ``, stderr: ``, }, + { + name: "REST error", + httpResponse: &http.Response{ + StatusCode: 400, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"message": "THIS IS FINE"}`)), + Header: http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}, + }, + err: cmdutil.SilentError, + stdout: `{"message": "THIS IS FINE"}`, + stderr: "gh: THIS IS FINE (HTTP 400)\n", + }, + { + name: "GraphQL error", + options: ApiOptions{ + RequestPath: "graphql", + }, + httpResponse: &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"errors": [{"message":"AGAIN"}, {"message":"FINE"}]}`)), + Header: http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}, + }, + err: cmdutil.SilentError, + stdout: `{"errors": [{"message":"AGAIN"}, {"message":"FINE"}]}`, + stderr: "gh: AGAIN\nFINE\n", + }, { name: "failure", httpResponse: &http.Response{ @@ -166,7 +213,7 @@ func Test_apiRun(t *testing.T) { }, err: cmdutil.SilentError, stdout: `gateway timeout`, - stderr: ``, + stderr: "gh: HTTP 502\n", }, } @@ -199,6 +246,81 @@ func Test_apiRun(t *testing.T) { } } +func Test_apiRun_inputFile(t *testing.T) { + tests := []struct { + name string + inputFile string + inputContents []byte + + contentLength int64 + expectedContents []byte + }{ + { + name: "stdin", + inputFile: "-", + inputContents: []byte("I WORK OUT"), + contentLength: 0, + }, + { + name: "from file", + inputFile: "gh-test-file", + inputContents: []byte("I WORK OUT"), + contentLength: 10, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, stdin, _, _ := iostreams.Test() + resp := &http.Response{StatusCode: 204} + + inputFile := tt.inputFile + if tt.inputFile == "-" { + _, _ = stdin.Write(tt.inputContents) + } else { + f, err := ioutil.TempFile("", tt.inputFile) + if err != nil { + t.Fatal(err) + } + _, _ = f.Write(tt.inputContents) + f.Close() + t.Cleanup(func() { os.Remove(f.Name()) }) + inputFile = f.Name() + } + + var bodyBytes []byte + options := ApiOptions{ + RequestPath: "hello", + RequestInputFile: inputFile, + RawFields: []string{"a=b", "c=d"}, + + IO: io, + HttpClient: func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + var err error + if bodyBytes, err = ioutil.ReadAll(req.Body); err != nil { + return nil, err + } + resp.Request = req + return resp, nil + } + return &http.Client{Transport: tr}, nil + }, + } + + err := apiRun(&options) + if err != nil { + t.Errorf("got error %v", err) + } + + assert.Equal(t, "POST", resp.Request.Method) + assert.Equal(t, "/hello?a=b&c=d", resp.Request.URL.RequestURI()) + assert.Equal(t, tt.contentLength, resp.Request.ContentLength) + assert.Equal(t, "", resp.Request.Header.Get("Content-Type")) + assert.Equal(t, tt.inputContents, bodyBytes) + }) + } +} + func Test_parseFields(t *testing.T) { io, stdin, _, _ := iostreams.Test() fmt.Fprint(stdin, "pasted contents") @@ -305,3 +427,27 @@ func Test_magicFieldValue(t *testing.T) { }) } } + +func Test_openUserFile(t *testing.T) { + f, err := ioutil.TempFile("", "gh-test") + if err != nil { + t.Fatal(err) + } + fmt.Fprint(f, "file contents") + f.Close() + t.Cleanup(func() { os.Remove(f.Name()) }) + + file, length, err := openUserFile(f.Name(), nil) + if err != nil { + t.Fatal(err) + } + defer file.Close() + + fb, err := ioutil.ReadAll(file) + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, int64(13), length) + assert.Equal(t, "file contents", string(fb)) +} diff --git a/pkg/cmd/api/http.go b/pkg/cmd/api/http.go index 812b95af4..e393bb593 100644 --- a/pkg/cmd/api/http.go +++ b/pkg/cmd/api/http.go @@ -7,12 +7,19 @@ import ( "io" "net/http" "net/url" + "strconv" "strings" ) func httpRequest(client *http.Client, method string, p string, params interface{}, headers []string) (*http.Response, error) { + var requestURL string // TODO: GHE support - url := "https://api.github.com/" + p + if strings.Contains(p, "://") { + requestURL = p + } else { + requestURL = "https://api.github.com/" + p + } + var body io.Reader var bodyIsJSON bool isGraphQL := p == "graphql" @@ -20,7 +27,7 @@ func httpRequest(client *http.Client, method string, p string, params interface{ switch pp := params.(type) { case map[string]interface{}: if strings.EqualFold(method, "GET") { - url = addQuery(url, pp) + requestURL = addQuery(requestURL, pp) } else { for key, value := range pp { switch vv := value.(type) { @@ -46,7 +53,7 @@ func httpRequest(client *http.Client, method string, p string, params interface{ return nil, fmt.Errorf("unrecognized parameters type: %v", params) } - req, err := http.NewRequest(method, url, body) + req, err := http.NewRequest(method, requestURL, body) if err != nil { return nil, err } @@ -56,7 +63,16 @@ func httpRequest(client *http.Client, method string, p string, params interface{ if idx == -1 { return nil, fmt.Errorf("header %q requires a value separated by ':'", h) } - req.Header.Add(h[0:idx], strings.TrimSpace(h[idx+1:])) + name, value := h[0:idx], strings.TrimSpace(h[idx+1:]) + if strings.EqualFold(name, "Content-Length") { + length, err := strconv.ParseInt(value, 10, 0) + if err != nil { + return nil, err + } + req.ContentLength = length + } else { + req.Header.Add(name, value) + } } if bodyIsJSON && req.Header.Get("Content-Type") == "" { req.Header.Set("Content-Type", "application/json; charset=utf-8") diff --git a/pkg/githubtemplate/github_template.go b/pkg/githubtemplate/github_template.go index 89cceda4c..e14ad04c8 100644 --- a/pkg/githubtemplate/github_template.go +++ b/pkg/githubtemplate/github_template.go @@ -10,8 +10,8 @@ import ( "gopkg.in/yaml.v3" ) -// Find returns the list of template file paths -func Find(rootDir string, name string) []string { +// FindNonLegacy returns the list of template file paths from the template folder (according to the "upgraded multiple template builder") +func FindNonLegacy(rootDir string, name string) []string { results := []string{} // https://help.github.com/en/github/building-a-strong-community/creating-a-pull-request-template-for-your-repository @@ -46,21 +46,34 @@ mainLoop: break } } + } + sort.Strings(results) + return results +} + +// FindLegacy returns the file path of the default(legacy) template +func FindLegacy(rootDir string, name string) *string { + // https://help.github.com/en/github/building-a-strong-community/creating-a-pull-request-template-for-your-repository + candidateDirs := []string{ + path.Join(rootDir, ".github"), + rootDir, + path.Join(rootDir, "docs"), + } + for _, dir := range candidateDirs { + files, err := ioutil.ReadDir(dir) + if err != nil { + continue + } // detect a single template file for _, file := range files { if strings.EqualFold(file.Name(), name+".md") { - results = append(results, path.Join(dir, file.Name())) - break + result := path.Join(dir, file.Name()) + return &result } } - if len(results) > 0 { - break - } } - - sort.Strings(results) - return results + return nil } // ExtractName returns the name of the template from YAML front-matter diff --git a/pkg/githubtemplate/github_template_test.go b/pkg/githubtemplate/github_template_test.go index 839e215b3..ee5dfb625 100644 --- a/pkg/githubtemplate/github_template_test.go +++ b/pkg/githubtemplate/github_template_test.go @@ -8,7 +8,7 @@ import ( "testing" ) -func TestFind(t *testing.T) { +func TestFindNonLegacy(t *testing.T) { tmpdir, err := ioutil.TempDir("", "gh-cli") if err != nil { t.Fatal(err) @@ -25,52 +25,69 @@ func TestFind(t *testing.T) { want []string }{ { - name: "Template in root", + name: "Legacy templates ignored", prepare: []string{ "README.md", "ISSUE_TEMPLATE", "issue_template.md", "issue_template.txt", "pull_request_template.md", - }, - args: args{ - rootDir: tmpdir, - name: "ISSUE_TEMPLATE", - }, - want: []string{ - path.Join(tmpdir, "issue_template.md"), - }, - }, - { - name: "Template in .github takes precedence", - prepare: []string{ - "ISSUE_TEMPLATE.md", ".github/issue_template.md", - }, - args: args{ - rootDir: tmpdir, - name: "ISSUE_TEMPLATE", - }, - want: []string{ - path.Join(tmpdir, ".github/issue_template.md"), - }, - }, - { - name: "Template in docs", - prepare: []string{ - "README.md", "docs/issue_template.md", }, args: args{ rootDir: tmpdir, name: "ISSUE_TEMPLATE", }, + want: []string{}, + }, + { + name: "Template folder in .github takes precedence", + prepare: []string{ + "ISSUE_TEMPLATE.md", + "docs/ISSUE_TEMPLATE/abc.md", + "ISSUE_TEMPLATE/abc.md", + ".github/ISSUE_TEMPLATE/abc.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, want: []string{ - path.Join(tmpdir, "docs/issue_template.md"), + path.Join(tmpdir, ".github/ISSUE_TEMPLATE/abc.md"), }, }, { - name: "Multiple templates", + name: "Template folder in root", + prepare: []string{ + "ISSUE_TEMPLATE.md", + "docs/ISSUE_TEMPLATE/abc.md", + "ISSUE_TEMPLATE/abc.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, + want: []string{ + path.Join(tmpdir, "ISSUE_TEMPLATE/abc.md"), + }, + }, + { + name: "Template folder in docs", + prepare: []string{ + "ISSUE_TEMPLATE.md", + "docs/ISSUE_TEMPLATE/abc.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, + want: []string{ + path.Join(tmpdir, "docs/ISSUE_TEMPLATE/abc.md"), + }, + }, + { + name: "Multiple templates in template folder", prepare: []string{ ".github/ISSUE_TEMPLATE/nope.md", ".github/PULL_REQUEST_TEMPLATE.md", @@ -90,18 +107,17 @@ func TestFind(t *testing.T) { }, }, { - name: "Empty multiple templates directory", + name: "Empty template directories", prepare: []string{ - ".github/issue_template.md", - ".github/issue_template/.keep", + ".github/ISSUE_TEMPLATE/.keep", + ".docs/ISSUE_TEMPLATE/.keep", + "ISSUE_TEMPLATE/.keep", }, args: args{ rootDir: tmpdir, name: "ISSUE_TEMPLATE", }, - want: []string{ - path.Join(tmpdir, ".github/issue_template.md"), - }, + want: []string{}, }, } for _, tt := range tests { @@ -116,7 +132,99 @@ func TestFind(t *testing.T) { file.Close() } - if got := Find(tt.args.rootDir, tt.args.name); !reflect.DeepEqual(got, tt.want) { + if got := FindNonLegacy(tt.args.rootDir, tt.args.name); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Find() = %v, want %v", got, tt.want) + } + }) + os.RemoveAll(tmpdir) + } +} + +func TestFindLegacy(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "gh-cli") + if err != nil { + t.Fatal(err) + } + + type args struct { + rootDir string + name string + } + tests := []struct { + name string + prepare []string + args args + want string + }{ + { + name: "Template in root", + prepare: []string{ + "README.md", + "ISSUE_TEMPLATE", + "issue_template.md", + "issue_template.txt", + "pull_request_template.md", + "docs/issue_template.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, + want: path.Join(tmpdir, "issue_template.md"), + }, + { + name: "Template in .github takes precedence", + prepare: []string{ + "ISSUE_TEMPLATE.md", + ".github/issue_template.md", + "docs/issue_template.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, + want: path.Join(tmpdir, ".github/issue_template.md"), + }, + { + name: "Template in docs", + prepare: []string{ + "README.md", + "docs/issue_template.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, + want: path.Join(tmpdir, "docs/issue_template.md"), + }, + { + name: "Non legacy templates ignored", + prepare: []string{ + ".github/PULL_REQUEST_TEMPLATE/abc.md", + "PULL_REQUEST_TEMPLATE/abc.md", + "docs/PULL_REQUEST_TEMPLATE/abc.md", + ".github/PULL_REQUEST_TEMPLATE.md", + }, + args: args{ + rootDir: tmpdir, + name: "PuLl_ReQuEsT_TeMpLaTe", + }, + want: path.Join(tmpdir, ".github/PULL_REQUEST_TEMPLATE.md"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, p := range tt.prepare { + fp := path.Join(tmpdir, p) + _ = os.MkdirAll(path.Dir(fp), 0700) + file, err := os.Create(fp) + if err != nil { + t.Fatal(err) + } + file.Close() + } + + if got := FindLegacy(tt.args.rootDir, tt.args.name); *got != tt.want { t.Errorf("Find() = %v, want %v", got, tt.want) } }) diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 028b11264..8fd213e9f 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -5,19 +5,36 @@ import ( "io" "io/ioutil" "os" + + "github.com/mattn/go-colorable" + "github.com/mattn/go-isatty" ) type IOStreams struct { In io.ReadCloser Out io.Writer ErrOut io.Writer + + colorEnabled bool +} + +func (s *IOStreams) ColorEnabled() bool { + return s.colorEnabled } func System() *IOStreams { + var out io.Writer = os.Stdout + var colorEnabled bool + if os.Getenv("NO_COLOR") == "" && isTerminal(os.Stdout) { + out = colorable.NewColorable(os.Stdout) + colorEnabled = true + } + return &IOStreams{ - In: os.Stdin, - Out: os.Stdout, - ErrOut: os.Stderr, + In: os.Stdin, + Out: out, + ErrOut: os.Stderr, + colorEnabled: colorEnabled, } } @@ -31,3 +48,7 @@ func Test() (*IOStreams, *bytes.Buffer, *bytes.Buffer, *bytes.Buffer) { ErrOut: errOut, }, in, out, errOut } + +func isTerminal(f *os.File) bool { + return isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd()) +} diff --git a/pkg/jsoncolor/jsoncolor.go b/pkg/jsoncolor/jsoncolor.go new file mode 100644 index 000000000..cf3bd064c --- /dev/null +++ b/pkg/jsoncolor/jsoncolor.go @@ -0,0 +1,96 @@ +package jsoncolor + +import ( + "encoding/json" + "fmt" + "io" + "strings" +) + +const ( + colorDelim = "1;38" // bright white + colorKey = "1;34" // bright blue + colorNull = "1;30" // gray + colorString = "32" // green + colorBool = "33" // yellow +) + +// Write colorized JSON output parsed from reader +func Write(w io.Writer, r io.Reader, indent string) error { + dec := json.NewDecoder(r) + dec.UseNumber() + + var idx int + var stack []json.Delim + + for { + t, err := dec.Token() + if err == io.EOF { + break + } + if err != nil { + return err + } + + switch tt := t.(type) { + case json.Delim: + switch tt { + case '{', '[': + stack = append(stack, tt) + idx = 0 + fmt.Fprintf(w, "\x1b[%sm%s\x1b[m", colorDelim, tt) + if dec.More() { + fmt.Fprint(w, "\n", strings.Repeat(indent, len(stack))) + } + continue + case '}', ']': + stack = stack[:len(stack)-1] + idx = 0 + fmt.Fprintf(w, "\x1b[%sm%s\x1b[m", colorDelim, tt) + } + default: + b, err := json.Marshal(tt) + if err != nil { + return err + } + + isKey := len(stack) > 0 && stack[len(stack)-1] == '{' && idx%2 == 0 + idx++ + + var color string + if isKey { + color = colorKey + } else if tt == nil { + color = colorNull + } else { + switch t.(type) { + case string: + color = colorString + case bool: + color = colorBool + } + } + + if color == "" { + _, _ = w.Write(b) + } else { + fmt.Fprintf(w, "\x1b[%sm%s\x1b[m", color, b) + } + + if isKey { + fmt.Fprintf(w, "\x1b[%sm:\x1b[m ", colorDelim) + continue + } + } + + if dec.More() { + fmt.Fprintf(w, "\x1b[%sm,\x1b[m\n%s", colorDelim, strings.Repeat(indent, len(stack))) + } else if len(stack) > 0 { + fmt.Fprint(w, "\n", strings.Repeat(indent, len(stack)-1)) + } else { + fmt.Fprint(w, "\n") + } + } + + return nil +} diff --git a/pkg/jsoncolor/jsoncolor_test.go b/pkg/jsoncolor/jsoncolor_test.go new file mode 100644 index 000000000..9e2eda343 --- /dev/null +++ b/pkg/jsoncolor/jsoncolor_test.go @@ -0,0 +1,83 @@ +package jsoncolor + +import ( + "bytes" + "io" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestWrite(t *testing.T) { + type args struct { + r io.Reader + indent string + } + tests := []struct { + name string + args args + wantW string + wantErr bool + }{ + { + name: "blank", + args: args{ + r: bytes.NewBufferString(``), + indent: "", + }, + wantW: "", + wantErr: false, + }, + { + name: "empty object", + args: args{ + r: bytes.NewBufferString(`{}`), + indent: "", + }, + wantW: "\x1b[1;38m{\x1b[m\x1b[1;38m}\x1b[m\n", + wantErr: false, + }, + { + name: "nested object", + args: args{ + r: bytes.NewBufferString(`{"hash":{"a":1,"b":2},"array":[3,4]}`), + indent: "\t", + }, + wantW: "\x1b[1;38m{\x1b[m\n\t\x1b[1;34m\"hash\"\x1b[m\x1b[1;38m:\x1b[m " + + "\x1b[1;38m{\x1b[m\n\t\t\x1b[1;34m\"a\"\x1b[m\x1b[1;38m:\x1b[m 1\x1b[1;38m,\x1b[m\n\t\t\x1b[1;34m\"b\"\x1b[m\x1b[1;38m:\x1b[m 2\n\t\x1b[1;38m}\x1b[m\x1b[1;38m,\x1b[m" + + "\n\t\x1b[1;34m\"array\"\x1b[m\x1b[1;38m:\x1b[m \x1b[1;38m[\x1b[m\n\t\t3\x1b[1;38m,\x1b[m\n\t\t4\n\t\x1b[1;38m]\x1b[m\n\x1b[1;38m}\x1b[m\n", + wantErr: false, + }, + { + name: "string", + args: args{ + r: bytes.NewBufferString(`"foo"`), + indent: "", + }, + wantW: "\x1b[32m\"foo\"\x1b[m\n", + wantErr: false, + }, + { + name: "error", + args: args{ + r: bytes.NewBufferString(`{{`), + indent: "", + }, + wantW: "\x1b[1;38m{\x1b[m\n", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := &bytes.Buffer{} + if err := Write(w, tt.args.r, tt.args.indent); (err != nil) != tt.wantErr { + t.Errorf("Write() error = %v, wantErr %v", err, tt.wantErr) + return + } + diff := cmp.Diff(tt.wantW, w.String()) + if diff != "" { + t.Error(diff) + } + }) + } +} diff --git a/utils/color.go b/utils/color.go index b2b2add21..4940fe26b 100644 --- a/utils/color.go +++ b/utils/color.go @@ -9,10 +9,20 @@ import ( "github.com/mgutz/ansi" ) -var _isColorEnabled = true -var _isStdoutTerminal = false -var checkedTerminal = false -var checkedNoColor = false +var ( + _isColorEnabled bool = true + _isStdoutTerminal, checkedTerminal, checkedNoColor bool + + // Outputs ANSI color if stdout is a tty + Magenta = makeColorFunc("magenta") + Cyan = makeColorFunc("cyan") + Red = makeColorFunc("red") + Yellow = makeColorFunc("yellow") + Blue = makeColorFunc("blue") + Green = makeColorFunc("green") + Gray = makeColorFunc("black+h") + Bold = makeColorFunc("default+b") +) func isStdoutTerminal() bool { if !checkedTerminal { @@ -49,27 +59,3 @@ func isColorEnabled() bool { } return _isColorEnabled } - -// Magenta outputs ANSI color if stdout is a tty -var Magenta = makeColorFunc("magenta") - -// Cyan outputs ANSI color if stdout is a tty -var Cyan = makeColorFunc("cyan") - -// Red outputs ANSI color if stdout is a tty -var Red = makeColorFunc("red") - -// Yellow outputs ANSI color if stdout is a tty -var Yellow = makeColorFunc("yellow") - -// Blue outputs ANSI color if stdout is a tty -var Blue = makeColorFunc("blue") - -// Green outputs ANSI color if stdout is a tty -var Green = makeColorFunc("green") - -// Gray outputs ANSI color if stdout is a tty -var Gray = makeColorFunc("black+h") - -// Bold outputs ANSI color if stdout is a tty -var Bold = makeColorFunc("default+b")