-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat: implement gh pr revert
#8826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
1f7464a
8e27640
cbfc183
e69a4be
67fd075
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,132 @@ | ||||||||||||||||||
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 {<number> | <url> | <branch>}", | ||||||||||||||||||
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)), | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Body field should only be set when opts.BodySet is true. Currently, an empty string will be passed to the API when no body is provided, which may override GitHub's default revert body generation.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||
|
||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Title field should only be set when opts.Title is not empty. Currently, an empty string will be passed to the API when no title is provided, which may override GitHub's default revert title generation.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||
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 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 | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
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{ | ||
Check failure on line 56 in pkg/cmd/pr/revert/revert_test.go
|
||
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(` | ||
{ "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") | ||
}), | ||
) | ||
|
||
output, err := runCommand(http, true, "123") | ||
assert.NoError(t, err) | ||
assert.Equal(t, "", output.String()) | ||
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) { | ||
http := &httpmock.Registry{} | ||
defer http.Verify(t) | ||
|
||
shared.RunCommandFinder("123", &api.PullRequest{ | ||
Check failure on line 89 in pkg/cmd/pr/revert/revert_test.go
|
||
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{ | ||
Check failure on line 106 in pkg/cmd/pr/revert/revert_test.go
|
||
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(` | ||
{ "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") | ||
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 pull request OWNER/REPO#456 (Revert PR title) that reverts OWNER/REPO#123 (The title of the PR)\n", output.Stderr()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The err variable will always be nil at this point since it was already checked and returned on line 737. This should return
revertPR, nil
instead.Copilot uses AI. Check for mistakes.