From 1f7464a05d5c20738b433710cadaf576ffb969c4 Mon Sep 17 00:00:00 2001 From: Lucas Melin Date: Sat, 16 Mar 2024 23:05:34 -0400 Subject: [PATCH 1/3] feat: implement `pr revert` --- api/queries_pr.go | 22 +++++- pkg/cmd/pr/pr.go | 2 + pkg/cmd/pr/revert/revert.go | 122 +++++++++++++++++++++++++++++++ pkg/cmd/pr/revert/revert_test.go | 120 ++++++++++++++++++++++++++++++ 4 files changed, 263 insertions(+), 3 deletions(-) create mode 100644 pkg/cmd/pr/revert/revert.go create mode 100644 pkg/cmd/pr/revert/revert_test.go diff --git a/api/queries_pr.go b/api/queries_pr.go index 36f3c2eef03..a31287206f0 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -480,8 +480,8 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS func (pr *PullRequest) DisplayableReviews() PullRequestReviews { published := []PullRequestReview{} for _, prr := range pr.Reviews.Nodes { - //Dont display pending reviews - //Dont display commenting reviews without top level comment body + // Dont display pending reviews + // Dont display commenting reviews without top level comment body if prr.State != "PENDING" && !(prr.State == "COMMENTED" && prr.Body == "") { published = append(published, prr) } @@ -561,7 +561,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter reviewParams["teamIds"] = ids } - //TODO: How much work to extract this into own method and use for create and edit? + // TODO: How much work to extract this into own method and use for create and edit? if len(reviewParams) > 0 { reviewQuery := ` mutation PullRequestCreateRequestReviews($input: RequestReviewsInput!) { @@ -674,6 +674,22 @@ func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) er return client.Mutate(repo.RepoHost(), "PullRequestReadyForReview", &mutation, variables) } +func PullRequestRevert(client *Client, repo ghrepo.Interface, params githubv4.RevertPullRequestInput) error { + var mutation struct { + RevertPullRequest struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"revertPullRequest(input: $input)"` + } + + variables := map[string]interface{}{ + "input": params, + } + + return client.Mutate(repo.RepoHost(), "PullRequestRevert", &mutation, variables) +} + func ConvertPullRequestToDraft(client *Client, repo ghrepo.Interface, pr *PullRequest) error { var mutation struct { ConvertPullRequestToDraft struct { diff --git a/pkg/cmd/pr/pr.go b/pkg/cmd/pr/pr.go index 401177755cd..ac558d913a0 100644 --- a/pkg/cmd/pr/pr.go +++ b/pkg/cmd/pr/pr.go @@ -14,6 +14,7 @@ import ( cmdMerge "github.com/cli/cli/v2/pkg/cmd/pr/merge" cmdReady "github.com/cli/cli/v2/pkg/cmd/pr/ready" cmdReopen "github.com/cli/cli/v2/pkg/cmd/pr/reopen" + cmdRevert "github.com/cli/cli/v2/pkg/cmd/pr/revert" cmdReview "github.com/cli/cli/v2/pkg/cmd/pr/review" cmdStatus "github.com/cli/cli/v2/pkg/cmd/pr/status" cmdView "github.com/cli/cli/v2/pkg/cmd/pr/view" @@ -61,6 +62,7 @@ func NewCmdPR(f *cmdutil.Factory) *cobra.Command { cmdComment.NewCmdComment(f, nil), cmdClose.NewCmdClose(f, nil), cmdReopen.NewCmdReopen(f, nil), + cmdRevert.NewCmdRevert(f, nil), cmdEdit.NewCmdEdit(f, nil), cmdLock.NewCmdLock(f, cmd.Name(), nil), cmdLock.NewCmdUnlock(f, cmd.Name(), nil), diff --git a/pkg/cmd/pr/revert/revert.go b/pkg/cmd/pr/revert/revert.go new file mode 100644 index 00000000000..7429f3436e4 --- /dev/null +++ b/pkg/cmd/pr/revert/revert.go @@ -0,0 +1,122 @@ +package revert + +import ( + "fmt" + "net/http" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/pr/shared" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/shurcooL/githubv4" + "github.com/spf13/cobra" +) + +type RevertOptions struct { + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + + Finder shared.PRFinder + + SelectorArg string + + Body string + BodySet bool + Title string + IsDraft bool +} + +func NewCmdRevert(f *cmdutil.Factory, runF func(*RevertOptions) error) *cobra.Command { + opts := &RevertOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + } + + var bodyFile string + + cmd := &cobra.Command{ + Use: "revert { | | }", + Short: "Revert a pull request", + Args: cmdutil.ExactArgs(1, "cannot revert pull request: number, url, or branch required"), + RunE: func(cmd *cobra.Command, args []string) error { + opts.Finder = shared.NewFinder(f) + + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + bodyProvided := cmd.Flags().Changed("body") + bodyFileProvided := bodyFile != "" + + if err := cmdutil.MutuallyExclusive( + "specify only one of `--body` or `--body-file`", + bodyProvided, + bodyFileProvided, + ); err != nil { + return err + } + + if bodyProvided || bodyFileProvided { + opts.BodySet = true + if bodyFileProvided { + b, err := cmdutil.ReadFile(bodyFile, opts.IO.In) + if err != nil { + return err + } + opts.Body = string(b) + } + } + + if runF != nil { + return runF(opts) + } + return revertRun(opts) + }, + } + + cmd.Flags().BoolVarP(&opts.IsDraft, "draft", "d", false, "Mark revert pull request as a draft") + cmd.Flags().StringVarP(&opts.Title, "title", "t", "", "Title for the revert pull request") + cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Body for the revert pull request") + cmd.Flags().StringVarP(&bodyFile, "body-file", "F", "", "Read body text from `file` (use \"-\" to read from standard input)") + return cmd +} + +func revertRun(opts *RevertOptions) error { + cs := opts.IO.ColorScheme() + + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, + Fields: []string{"id", "number", "state", "title"}, + } + pr, baseRepo, err := opts.Finder.Find(findOptions) + if err != nil { + return err + } + if pr.State != "MERGED" { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request %s#%d (%s) can't be reverted because it has not been merged\n", cs.FailureIcon(), ghrepo.FullName(baseRepo), pr.Number, pr.Title) + return cmdutil.SilentError + } + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + params := githubv4.RevertPullRequestInput{ + PullRequestID: pr.ID, + Title: githubv4.NewString(githubv4.String(opts.Title)), + Body: githubv4.NewString(githubv4.String(opts.Body)), + Draft: githubv4.NewBoolean(githubv4.Boolean(opts.IsDraft)), + } + + err = api.PullRequestRevert(apiClient, baseRepo, params) + if err != nil { + return fmt.Errorf("API call failed: %w", err) + } + + fmt.Fprintf(opts.IO.ErrOut, "%s Created revert PR for pull request %s#%d (%s)\n", cs.SuccessIconWithColor(cs.Green), ghrepo.FullName(baseRepo), pr.Number, pr.Title) + + return nil +} diff --git a/pkg/cmd/pr/revert/revert_test.go b/pkg/cmd/pr/revert/revert_test.go new file mode 100644 index 00000000000..edd0a859541 --- /dev/null +++ b/pkg/cmd/pr/revert/revert_test.go @@ -0,0 +1,120 @@ +package revert + +import ( + "bytes" + "io" + "net/http" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/pr/shared" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/test" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(isTTY) + ios.SetStdinTTY(isTTY) + ios.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + } + + cmd := NewCmdRevert(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func TestPRRevert(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "SOME-ID", + Number: 123, + State: "MERGED", + Title: "The title of the PR", + }, ghrepo.New("OWNER", "REPO")) + + http.Register( + httpmock.GraphQL(`mutation PullRequestRevert\b`), + httpmock.GraphQLMutation(`{"id": "SOME-ID"}`, + func(inputs map[string]interface{}) { + assert.Equal(t, inputs["pullRequestId"], "SOME-ID") + }), + ) + + output, err := runCommand(http, true, "123") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Created revert PR for pull request OWNER/REPO#123 (The title of the PR)\n", output.Stderr()) +} + +func TestPRRevert_notRevertable(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "SOME-ID", + Number: 123, + State: "OPEN", + Title: "The title of the PR", + }, ghrepo.New("OWNER", "REPO")) + + output, err := runCommand(http, true, "123") + assert.Error(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "X Pull request OWNER/REPO#123 (The title of the PR) can't be reverted because it has not been merged\n", output.Stderr()) +} + +func TestPRRevert_withAllOpts(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "THE-ID", + Number: 123, + State: "MERGED", + Title: "The title of the PR", + }, ghrepo.New("OWNER", "REPO")) + + http.Register( + httpmock.GraphQL(`mutation PullRequestRevert\b`), + httpmock.GraphQLMutation(`{"id": "THE-ID" }`, + func(inputs map[string]interface{}) { + assert.Equal(t, inputs["pullRequestId"], "THE-ID") + assert.Equal(t, inputs["title"], "Revert PR title") + assert.Equal(t, inputs["body"], "Revert PR body") + assert.Equal(t, inputs["draft"], true) + }), + ) + + output, err := runCommand(http, true, "123 --title 'Revert PR title' --body 'Revert PR body' --draft") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Created revert PR for pull request OWNER/REPO#123 (The title of the PR)\n", output.Stderr()) +} From 8e2764063f4fbfe0a2884226e33e61f9976446df Mon Sep 17 00:00:00 2001 From: Lucas Melin Date: Sat, 16 Mar 2024 23:20:43 -0400 Subject: [PATCH 2/3] Undo autoformat changes --- api/queries_pr.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index a31287206f0..c6e7a1cdeed 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -480,8 +480,8 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS func (pr *PullRequest) DisplayableReviews() PullRequestReviews { published := []PullRequestReview{} for _, prr := range pr.Reviews.Nodes { - // Dont display pending reviews - // Dont display commenting reviews without top level comment body + //Dont display pending reviews + //Dont display commenting reviews without top level comment body if prr.State != "PENDING" && !(prr.State == "COMMENTED" && prr.Body == "") { published = append(published, prr) } @@ -561,7 +561,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter reviewParams["teamIds"] = ids } - // TODO: How much work to extract this into own method and use for create and edit? + //TODO: How much work to extract this into own method and use for create and edit? if len(reviewParams) > 0 { reviewQuery := ` mutation PullRequestCreateRequestReviews($input: RequestReviewsInput!) { From cbfc18376bbe6ef22f85c0d2fb6b9c99e180b251 Mon Sep 17 00:00:00 2001 From: Lucas Melin Date: Sun, 17 Mar 2024 09:27:45 -0400 Subject: [PATCH 3/3] feat: include revert PR info in message output --- api/queries_pr.go | 11 ++++++++--- pkg/cmd/pr/revert/revert.go | 14 ++++++++++++-- pkg/cmd/pr/revert/revert_test.go | 28 ++++++++++++++++++++++------ 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index c6e7a1cdeed..a265e707b08 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -674,20 +674,25 @@ func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) er return client.Mutate(repo.RepoHost(), "PullRequestReadyForReview", &mutation, variables) } -func PullRequestRevert(client *Client, repo ghrepo.Interface, params githubv4.RevertPullRequestInput) error { +func PullRequestRevert(client *Client, repo ghrepo.Interface, params githubv4.RevertPullRequestInput) (*PullRequest, error) { var mutation struct { RevertPullRequest struct { PullRequest struct { ID githubv4.ID } + RevertPullRequest PullRequest } `graphql:"revertPullRequest(input: $input)"` } variables := map[string]interface{}{ "input": params, } - - return client.Mutate(repo.RepoHost(), "PullRequestRevert", &mutation, variables) + err := client.Mutate(repo.RepoHost(), "PullRequestRevert", &mutation, variables) + if err != nil { + return nil, err + } + revertPR := &mutation.RevertPullRequest.RevertPullRequest + return revertPR, err } func ConvertPullRequestToDraft(client *Client, repo ghrepo.Interface, pr *PullRequest) error { diff --git a/pkg/cmd/pr/revert/revert.go b/pkg/cmd/pr/revert/revert.go index 7429f3436e4..72ce534902b 100644 --- a/pkg/cmd/pr/revert/revert.go +++ b/pkg/cmd/pr/revert/revert.go @@ -111,12 +111,22 @@ func revertRun(opts *RevertOptions) error { Draft: githubv4.NewBoolean(githubv4.Boolean(opts.IsDraft)), } - err = api.PullRequestRevert(apiClient, baseRepo, params) + revertPR, err := api.PullRequestRevert(apiClient, baseRepo, params) if err != nil { return fmt.Errorf("API call failed: %w", err) } - fmt.Fprintf(opts.IO.ErrOut, "%s Created revert PR for pull request %s#%d (%s)\n", cs.SuccessIconWithColor(cs.Green), ghrepo.FullName(baseRepo), pr.Number, pr.Title) + fmt.Fprintf( + opts.IO.ErrOut, + "%s Created pull request %s#%d (%s) that reverts %s#%d (%s)\n", + cs.SuccessIconWithColor(cs.Green), + ghrepo.FullName(baseRepo), + revertPR.Number, + revertPR.Title, + ghrepo.FullName(baseRepo), + pr.Number, + pr.Title, + ) return nil } diff --git a/pkg/cmd/pr/revert/revert_test.go b/pkg/cmd/pr/revert/revert_test.go index edd0a859541..2833f36f896 100644 --- a/pkg/cmd/pr/revert/revert_test.go +++ b/pkg/cmd/pr/revert/revert_test.go @@ -62,7 +62,15 @@ func TestPRRevert(t *testing.T) { http.Register( httpmock.GraphQL(`mutation PullRequestRevert\b`), - httpmock.GraphQLMutation(`{"id": "SOME-ID"}`, + httpmock.GraphQLMutation(` + { "data": { "revertPullRequest": { "pullRequest": { + "ID": "SOME-ID" + }, "revertPullRequest": { + "ID": "NEW-ID", + "Title": "Revert PR title", + "Number": 456 + } } } } + `, func(inputs map[string]interface{}) { assert.Equal(t, inputs["pullRequestId"], "SOME-ID") }), @@ -71,7 +79,7 @@ func TestPRRevert(t *testing.T) { output, err := runCommand(http, true, "123") assert.NoError(t, err) assert.Equal(t, "", output.String()) - assert.Equal(t, "✓ Created revert PR for pull request OWNER/REPO#123 (The title of the PR)\n", output.Stderr()) + assert.Equal(t, "✓ Created pull request OWNER/REPO#456 (Revert PR title) that reverts OWNER/REPO#123 (The title of the PR)\n", output.Stderr()) } func TestPRRevert_notRevertable(t *testing.T) { @@ -96,7 +104,7 @@ func TestPRRevert_withAllOpts(t *testing.T) { defer http.Verify(t) shared.RunCommandFinder("123", &api.PullRequest{ - ID: "THE-ID", + ID: "SOME-ID", Number: 123, State: "MERGED", Title: "The title of the PR", @@ -104,9 +112,17 @@ func TestPRRevert_withAllOpts(t *testing.T) { http.Register( httpmock.GraphQL(`mutation PullRequestRevert\b`), - httpmock.GraphQLMutation(`{"id": "THE-ID" }`, + httpmock.GraphQLMutation(` + { "data": { "revertPullRequest": { "pullRequest": { + "ID": "SOME-ID" + }, "revertPullRequest": { + "ID": "NEW-ID", + "Title": "Revert PR title", + "Number": 456 + } } } } + `, func(inputs map[string]interface{}) { - assert.Equal(t, inputs["pullRequestId"], "THE-ID") + assert.Equal(t, inputs["pullRequestId"], "SOME-ID") assert.Equal(t, inputs["title"], "Revert PR title") assert.Equal(t, inputs["body"], "Revert PR body") assert.Equal(t, inputs["draft"], true) @@ -116,5 +132,5 @@ func TestPRRevert_withAllOpts(t *testing.T) { output, err := runCommand(http, true, "123 --title 'Revert PR title' --body 'Revert PR body' --draft") assert.NoError(t, err) assert.Equal(t, "", output.String()) - assert.Equal(t, "✓ Created revert PR for pull request OWNER/REPO#123 (The title of the PR)\n", output.Stderr()) + assert.Equal(t, "✓ Created pull request OWNER/REPO#456 (Revert PR title) that reverts OWNER/REPO#123 (The title of the PR)\n", output.Stderr()) }